[WIP][hydra-configs-torchvision] v0.7 models #65
Conversation
|
Hi @shivamdb! Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Nice! Thanks for the addition! Let's keep this as a draft until we have the tests complete and passing. |
| @@ -2,3 +2,4 @@ git+https://github.com/facebookresearch/hydra#subdirectory=tools/configen | |||
| git+https://github.com/facebookresearch/hydra | |||
| torch==1.6.0 | |||
| torchvision==0.7.0 | |||
| scipy | |||
There was a problem hiding this comment.
We shouldn't make the primary requirements.txt the union of the dependencies of all subprojects.
There was a problem hiding this comment.
yeah we need to migrate this out of here. I think we should generally have per project dependencies for both using the library and running tests / doing dev.
It's not clear that someone who wants to use torchvision configs will ever use a model that depends on scipy for example, so we shouldn't force them to install it. But if they plan to do dev/run tests, then it makes sense.
There was a problem hiding this comment.
Otherwise I think this PR looks good.
Maybe after the minor changes, we can merge and then I can follow up with a reorganization of how we manage dependencies across projects in a new PR.
There was a problem hiding this comment.
The tests seems a bit too shallow to me. Is it really the best we can do to verify that the instantiated objects are good?
There was a problem hiding this comment.
I agree that the tests are minimal.
@shivamdb, would it be possible to check that both:
- the models instantiate correctly (which you've already done)
- each instantiated model is capable of running a single
forward()pass?
There was a problem hiding this comment.
About the dependencies:
another approach is to put things needed to run the tests in a requirements/dev.txt.
There was a problem hiding this comment.
Yes, that's the plan. We can have requirements/dev.txt for each project. I can update nox to create a specific environment when testing a specific project.
There was a problem hiding this comment.
As long as we have a handful of projects, we can also have a unified dev dependencies for all of them.
There was a problem hiding this comment.
Addressing the above comments in #59
| from typing import Any | ||
|
|
||
| bb = BasicBlock(10, 10) | ||
| mnasnet_dict = {"alpha": 1.0, "num_classes": 1000} |
There was a problem hiding this comment.
What is this for? Looks like it's unused. You can probably remove it.
| from hydra.utils import get_class, instantiate | ||
| from omegaconf import OmegaConf | ||
|
|
||
| import torchvision.models as models |
There was a problem hiding this comment.
I might suggest tidying up the imports here slightly. Not the end of the world, but I think it would be better to follow PEP8:
Imports are always put at the top of the file, just after any module comments and docstrings, and before module globals and constants.
Imports should be grouped in the following order:
- Standard library imports.
- Related third party imports.
- Local application/library specific imports.
You should put a blank line between each group of imports.
I'm probably also guilty of this in some of the other tests, but I want to continue improving the standards.
There was a problem hiding this comment.
I suggest to integrate with isort.
|
Hey, @shivamdb, I would love to close this PR! I think it's a great addition! How are you doing on the change requests? Do you need any more support? Happy to chat more =]. |
I have added classes for torchvison models like AlexNet, DenseNet, ResNet, SqueezeNet, GoogleNet, MNASNet
I'll add tests later.