-
Notifications
You must be signed in to change notification settings - Fork 7.1k
disable weight download and state dict loading for model tests #4867
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
Conversation
💊 CI failures summary and remediationsAs of commit b6e6569 (more details on the Dr. CI page):
2 failures not recognized by patterns:
1 job timed out:
This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, it seems it might be still downloading the weights. Worth confirming though! Marking as "request changes" to give time for more investigations.
) | ||
|
||
for target in targets: | ||
with contextlib.suppress(AttributeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is (atleast) one situation where this patching doesn't work. We have the module torchvision.models.alexnet
that defines the function alexnet
, which is then imported into the torchvision.models
namespace. Thus, after the import is complete, we can no longer access the module through torchvision.models.alexnet
.
This is an anti-pattern and should be avoided. Given that we expose everything under torchvision.models
, I suggest making all the other modules private. This avoids this naming issue and gives us the freedom to change the underlying structure however we want without needing to keep BC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, it is what it is. Not sure many other models have the same issue but I don't think it's worth breaking BC over it. Alexnet is in for historical reasons and unlikely to be used as a backbone for any modern architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks a lot @pmeier
I've tested this and it works locally.
) | ||
|
||
for target in targets: | ||
with contextlib.suppress(AttributeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, it is what it is. Not sure many other models have the same issue but I don't think it's worth breaking BC over it. Alexnet is in for historical reasons and unlikely to be used as a backbone for any modern architecture.
…ts (#4867) Summary: * disable weight download and state dict loading for model tests * fix indent * debug * nuclear option * revert unrelated change * cleanup * add explanation * typo Reviewed By: datumbox Differential Revision: D32298972 fbshipit-source-id: 6c73192e0de442a691a993f2a4782811c5db2c32
…ch#4867) * disable weight download and state dict loading for model tests * fix indent * debug * nuclear option * revert unrelated change * cleanup * add explanation * typo
Fixes #4660.
cc @datumbox @pmeier