Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[utils]: Unify the scripts for downloading database and caffe2/onnx models. #2875

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@ZchiPitt
Copy link
Contributor

commented May 9, 2019

Description:
Merge utils/download_caffe2_models.sh and utils/download_onnx_models.sh into utils/download_test_db.py.

  • Add two functions: download_onnx_models() and download_caffe2_models()
  • All functionalities for the previous two scripts are in these functions.
  • Also modify the argument parser such that the downloading of onnx/caffe2 models will be triggered by passing the parameters -c/-o.

[Testing]:
python download_datasets_and_models.py -D -C -O

  • Sanity checked by removing all downloaded files and re-run this script. Verified that tests/images/run.sh still works, with no breaks.
  • Also tested by removing some downloaded files and re-run the script. The script skipped all those that have been downloaded and extracted, and just worked on the missing ones. Also verified with run.sh.
  • Tested with -P flag that specifies the directory that generated files will go to. And all files did show up in that directory, lol.

[Optional Fixes #issue]
#2774

@ZchiPitt ZchiPitt requested a review from nickgg May 9, 2019

@ZchiPitt ZchiPitt requested review from nadavrot and opti-mix May 9, 2019

@SplitInfinity
Copy link
Contributor

left a comment

I think the following would also be useful:

  1. Allow the user to specify a subset of C2/ONNX models to download
  2. A command-line option to specify the output directory
Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated
@nickgg
Copy link
Contributor

left a comment

Functionally looks good, nice job - just some style / organisation issues.

Can you add a test plan, e.g: sanity checked by deleting all models, running the script and verify run.sh still works.

Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated
@opti-mix

This comment has been minimized.

Copy link
Contributor

commented May 9, 2019

Can you add a test plan, e.g: sanity checked by deleting all models, running the script and verify run.sh still works.

Also testing what happens if you run the script multiple times and some files were (partially) downloaded already.

@jfix71
Copy link
Contributor

left a comment

Looks mostly good -- but I'm a little bit concerned there's confusion here between the original models that are downloaded (mnist, cifar10, etc.), and the Caffe2/ONNX models. For example the --datasets flag only applies to the original models (mnist, cifar10, etc.), but I would imagine the distinction is not clear for users of this script. I.e. if they try --datasets resnet50 it will not work as expected.

Show resolved Hide resolved utils/download_test_db.py Outdated
Show resolved Hide resolved utils/download_test_db.py Outdated

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch 2 times, most recently from ce44ace to 997a342 May 9, 2019

@nickgg
Copy link
Contributor

left a comment

This looks awesome, a few minor nits:

Show resolved Hide resolved utils/download_datasets_and_models.py Outdated
Show resolved Hide resolved utils/download_datasets_and_models.py Outdated
Show resolved Hide resolved .circleci/build.sh Outdated
@ZchiPitt

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@SplitInfinity

Allow the user to specify a subset of C2/ONNX models to download

Implemented now

A command-line option to specify the output directory

Do you mean specify a directory that all downloaded files go into this directory? Cuz we generated so downloaded so many files, the command line would look very long if we specify a dest dir for every file it downloads.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch from 997a342 to 7105fa1 May 10, 2019

@SplitInfinity

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

@ZchiPitt

Yes, one directory for all files.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch from 7105fa1 to 0071530 May 10, 2019

@ZchiPitt

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@SplitInfinity

A command-line option to specify the output directory

Implemented now. By specifying -P directory, all downloaded files will be put in specified directory.

@ZchiPitt

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

[Testing]:
python download_datasets_and_models.py -D -C -O

  • Sanity checked by removing all downloaded files and re-run this script. Verified that tests/images/run.sh still works, with no breaks.
  • Also tested by removing some downloaded files and re-run the script. The script skipped all those that have been downloaded and extracted, and just worked on the missing ones. Also verified with run.sh.
  • Tested with -P flag that specifies the directory that generated files will go to. And all files did show up in that directory, lol.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch from 0071530 to 02fd8ef May 10, 2019

@nickgg

nickgg approved these changes May 10, 2019

Copy link
Contributor

left a comment

Looks awesome! Nice work.

@nadavrot

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

Make sure to update the README and other docs.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch 4 times, most recently from 4adb3fa to 91be19e May 10, 2019

@ZchiPitt

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

All affected files that reference to the previous scripts are updated now.

@SplitInfinity
Copy link
Contributor

left a comment

Thanks for addressing my comments.

@jfix71

jfix71 approved these changes May 13, 2019

Copy link
Contributor

left a comment

Awesome, LGTM, just one question.

Show resolved Hide resolved .circleci/build.sh Outdated
@facebook-github-bot
Copy link

left a comment

@ZchiPitt has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch 2 times, most recently from 2bcfae1 to cc3eaff May 13, 2019

@facebook-github-bot
Copy link

left a comment

@ZchiPitt has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch from cc3eaff to 2fb2269 May 14, 2019

ZchiPitt added a commit to ZchiPitt/glow that referenced this pull request May 14, 2019

Unify the scripts for downloading database and caffe2/onnx models. (p…
…ytorch#2875)

Summary:
*Description*:
Merge utils/download_caffe2_models.sh and utils/download_onnx_models.sh into utils/download_test_db.py.

- Add two functions: download_onnx_models() and download_caffe2_models()
- All functionalities for the previous two scripts are in these functions.
- Also modify the argument parser such that the downloading of onnx/caffe2 models will be triggered by passing the parameters -c/-o.

[Testing]:
 python download_datasets_and_models.py -D -C -O
- Sanity checked by removing all downloaded files and re-run this script. Verified that tests/images/run.sh still works, with no breaks.
- Also tested by removing some downloaded files and re-run the script. The script skipped all those that have been downloaded and extracted, and just worked on the missing ones. Also verified with run.sh.
- Tested with -P flag that specifies the directory that generated files will go to. And all files did show up in that directory, lol.

[Optional Fixes #issue]
Pull Request resolved: pytorch#2875

Differential Revision: D15322305

fbshipit-source-id: 3743902912b9a7ccf5ac9d748cf39d7616f1ee42
Unify the scripts for downloading database and caffe2/onnx models. (#…
…2875)

Summary:
*Description*:
Merge utils/download_caffe2_models.sh and utils/download_onnx_models.sh into utils/download_test_db.py.

- Add two functions: download_onnx_models() and download_caffe2_models()
- All functionalities for the previous two scripts are in these functions.
- Also modify the argument parser such that the downloading of onnx/caffe2 models will be triggered by passing the parameters -c/-o.

[Testing]:
 python download_datasets_and_models.py -D -C -O
- Sanity checked by removing all downloaded files and re-run this script. Verified that tests/images/run.sh still works, with no breaks.
- Also tested by removing some downloaded files and re-run the script. The script skipped all those that have been downloaded and extracted, and just worked on the missing ones. Also verified with run.sh.
- Tested with -P flag that specifies the directory that generated files will go to. And all files did show up in that directory, lol.

[Optional Fixes #issue]
Pull Request resolved: #2875

Differential Revision: D15322305

fbshipit-source-id: e58756bd259979b8b0380cb5ec3e4e5581cb9b1c

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:unifyScripts branch from 2fb2269 to e827693 May 14, 2019

@facebook-github-bot
Copy link

left a comment

@ZchiPitt is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 14, 2019

@ZchiPitt merged this pull request in 3ec5904.

@ZchiPitt ZchiPitt deleted the ZchiPitt:unifyScripts branch May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.