Add optional destination folder in download_models CLI command

[vasudev-sharma]

This PR aims to add an optional flag -p / --path and resolves some of the issued while using download_models CLI command to retrieve the models (SEM and TEM) stored on the OSF in a specific destination folder.

Usage
download_models -p destination_folder

Modifications:

1. Using CLI, user is able to download the models in a specific destination folder.
2. Fixes the bug which is caused when one tries download models in the working directory.
3. Fix bug which is encountered when user tries to download model by calling downlaod_models cli command in directory different from root directory

Testing Instructions

1. First, check-p/--pathflag works in terminal.
   -Go to terminal, and run download_models -p destination_folder_path. (Note path to destination folder path
   could either be absolute or relative)
   - SEM and TEM models would be saved under the given destination folder.
2. Second, go to the working directory and set the destination folder to the working directory.
   - Assume the working directory to be axondeepseg, run the cli command download_models
   by setting the -p flag to be current working directory, that is,
   download_models -p /Users/vasudevsharma/VScode/Projects/axondeepseg
   - You will note that this fails in the latest master branch.
3. Third, check whether additional directories are created when download_models cli command is executed in a directory different from root directory.
   - To test this error, follow this comment in both the master branch and vs74/download_model_cli_download_folder branch of this PR.

Resolves #323, Resolves #389, Resolves #394

[vasudev-sharma]

@mathieuboudreau or @mariehbourget could either of you review this PR?

[mathieuboudreau]

Sure, I'll try to get to it later tonight

[mathieuboudreau]

Just having a quick look, @vs74 the edits & logic seem a bit complicated to not have written tests for them. I recommend writing unit tests for each of the cases so that you can define the expected behaviour clearly. Beyond making this feature more stable moving forward if someone else adds more changes, it will also help me with the current review.

There is already a file for unit tests of this module, here it is: https://github.com/neuropoly/axondeepseg/blob/master/test/models/test_download_models.py. Your tests will have to be a bit more expanded than the current tests, which are only for the simplest use case we previously defined the behaviour for.

[vasudev-sharma]

One question, while using the CLI command to download the models, should we create destination folder if the destination folder is non existent?

[vasudev-sharma]

@mathieuboudreau , I've added some unit tests for the changes introduced in download_model.py script. Feel free to review them. Also, I haven't included the unit test for the issue #394, since I think it's not required becasue I fixed the harcoded paths in commit (cc83bed). Let me know what you think about it

[mathieuboudreau]

One question, while using the CLI command to download the models, should we create destination folder if the destination folder is non existent?

Yup

[mathieuboudreau]

According to the test coverage analysis, these two lines are missed by the tests. @vs74 could you add an additional test that deals with this case?

[vasudev-sharma]

Well, the thing is that when destination is None, the models get saved in the deafult AxonDeepSeg/models directory. Adding another test to cover these lines, would add an extra overhead in Travis CI testing since these lines would be executed twice: first during pip install -e . and second during testing, which would overwrite the existing models stored in default directory, AxonDeepSeg/models/. Do you think it's a good idea to do that?

[vasudev-sharma]

We will deal with this once ivadomed-ads integration is completed. Adding this PR in draft mode so as to avoid confusion with the new PR's.

[mathieuboudreau]

Stale PR, closing.