b-it-bots / mas_perception

2 stars 13 forks source link

[refactor] change dynamic config param name #35

Closed minhnh closed 5 years ago

minhnh commented 5 years ago

The parameter name change is requested by #33. I thought it would be a good point to add some unit tests to the cloud processing functionalities. Following are a few issues that arose during the process that may be worth documenting somewhere:

minhnh commented 5 years ago

@alex-mitrevski @argenos I guess since mas_domestic_datasets is private, cloning it would be a bit tricky with Travis. How should this be resolved? I assume some SSH key would be required in the .travis.yml file, and SSH clone in the mas-perception.rosinstall?

argenos commented 5 years ago

I don't have time right now to take a look at this, so here are a few hints in case you want to work on it:

minhnh commented 5 years ago
argenos commented 5 years ago

The “downloading” will happen anyways, except you will be pulling from Docker hub instead of using git lfs

minhnh commented 5 years ago

seems like there're some hacky ways to cache the docker image: https://github.com/travis-ci/travis-ci/issues/5358#issuecomment-248915326, but indeed caching docker image is not yet officially supported

argenos commented 5 years ago

I still don't think it makes sense to add the models to the docker image. Any image that builds upon this one will also contain the tests, whether we use them or not. Since this is just for testing, it makes more sense to add this as a volume. Loading other models would then also be easier to "override", e.g. switching a real model vs. the testing one.

minhnh commented 5 years ago

@argenos I am not sure how to do volumes with the current Travis-Docker setup that we have. Can you walk me through how to do this at some point next week?

minhnh commented 5 years ago

Ignoring themds_home_setups dependency for now to avoid Travis error, and because we are not actually running tests during builds. @sthoduka @alex-mitrevski @mhwasil please approve so I can merge.