dnn: download_models script improvements#1084
Conversation
1f267fb to
eb15035
Compare
opencv-alalek
left a comment
There was a problem hiding this comment.
--cachedirectory will be tried before downloading from internet
Need to ensure that this doesn't double storage size requirements.
|
@opencv-alalek , what do you mean? Cache can be located on a mounted network drive or can contain only several models which have stale links. The script will not populate the cache, it is read-only. To create the cache one should separately run the script or copy required files manually. Should I add a "read-only" mention to the parameter description? |
| Model( | ||
| name='GSOC2016-GOTURN', # https://github.com/opencv/opencv_contrib/issues/941 | ||
| downloader=GDrive('1j4UTqVE4EGaUFiK7a5I_CYX7twO9c5br'), | ||
| gdrive='1j4UTqVE4EGaUFiK7a5I_CYX7twO9c5br', |
There was a problem hiding this comment.
I would prefer to keep class-based implementation instead of expanding list of fields.
There was a problem hiding this comment.
At the current script state extracting this specific part of functionality to separate class hierarchy seemed unnecessary to me. By rewriting it as a function we've reduced code duplication (actual downloading). In the future we can extract every bit of functionality as a separate class (downloaders, hash checkers, etc.) to improve extensibility, but it does not seem to be particularly useful at this moment because script is still very simple.
There was a problem hiding this comment.
Using of GOD all-in-one classes should be considered as a bad practice.
Use OOP practices and add proper inheritance / dependency injection to avoid code duplication.
testdata/dnn/download_models.py
Outdated
| parser = argparse.ArgumentParser(description="Download test models for OpenCV library") | ||
| parser.add_argument("-d", "--dst", "--destination", help="Destination folder", default=Path.cwd()) | ||
| parser.add_argument("-l", "--list", action="store_true", help="List models") | ||
| parser.add_argument("-c", "--cache", help="Cache directory containing pre-downloaded models (read-only)") |
There was a problem hiding this comment.
This doesn't behave like a classic cache, so this confuses.
It looks like a "reference" helper storage, see "git clone --help".
There was a problem hiding this comment.
I've added "--reference" option alias. I believe read-only cache is a common enough thing and should be more intuitive than "git reference" for average user.
There was a problem hiding this comment.
I've updated the option, it is called -r, --ref, --reference now.
Added new features:
--cachedirectory will be tried before downloading from internet--dstchange destination directory--listlist all model names--cleanupremove alltar.gzfiles afterwardspathlib.Pathfor most paths.I have doubts regarding filter arguments: should we keep them positional arguments or change to keyword arguments (e.g.
--filter)?Examples:
Note: also updated my p2p model distribution project - https://github.com/mshabunin/opencv_models_p2p