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.

Sign up for GitHub

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

Jump to bottom

NOMRG Added _restore_import_cache decorator to see what breaks #247

Closed
wants to merge 8 commits into from
Closed

NOMRG Added _restore_import_cache decorator to see what breaks #247

wants to merge 8 commits into from

Conversation

NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Oct 14, 2021 •
edited
Loading

see below

@netlify
Copy link

netlify bot commented Oct 14, 2021 •
edited
Loading

❌ Deploy Preview for pytorch-hub-preview failed.

🔨 Explore the source changes: 6132d9d

🔍 Inspect the deploy log: https://app.netlify.com/sites/pytorch-hub-preview/deploys/61b87b99cd01630009af5756

@NicolasHug NicolasHug mentioned this pull request Dec 14, 2021
Merged
NicolasHug
NicolasHug commented Dec 14, 2021
View reviewed changes
lol_hub.py
Comment on lines +69 to +81
def _restore_import_cache(func):
# call func, and resore the sys.modules dict to what it was prior to the call
# This avoids bugs like https://github.com/pytorch/hub/issues/243
def inner(*args, **kwargs):
before = set(sys.modules.keys())
out = func(*args, **kwargs)
after = set(sys.modules.keys())
for module in after - before:
sys.modules.pop(module)

return out

return inner
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This decorator and its use in help(), list(), and load() is the only relevant chance of this PR. I need to copy-paste the entire file but the rest is unchanged.

@NicolasHug
Copy link
Member Author

Soooooo......

This works, but it doesn't.

This is an attempt at fixing #243. I added a decorator to all torchhub functions to avoid import clashes. It almost works, but it makes all torchvision scripts fail (except the first one), all with this same error:

E           Traceback (most recent call last):
E             File "python_code/pytorch_vision_deeplabv3_resnet101.py", line 15, in <module>
E               from torchvision import transforms
E             File "/home/circleci/miniconda3/lib/python3.8/site-packages/torchvision/__init__.py", line 7, in <module>
E               from torchvision import models
E             File "/home/circleci/miniconda3/lib/python3.8/site-packages/torchvision/models/__init__.py", line 6, in <module>
E               from .densenet import *
E             File "/home/circleci/miniconda3/lib/python3.8/site-packages/torchvision/models/densenet.py", line 25, in <module>
E               class _DenseLayer(nn.Module):
E             File "/home/circleci/miniconda3/lib/python3.8/site-packages/torchvision/models/densenet.py", line 69, in _DenseLayer
E               def forward(self, input: List[Tensor]) -> Tensor:  # noqa: F811
E             File "/home/circleci/miniconda3/lib/python3.8/site-packages/torch/_jit_internal.py", line 831, in _overload_method
E               raise RuntimeError("Cannot currently overload the same method name in two different"
E           RuntimeError: Cannot currently overload the same method name in two different classes with the same name in the same module

which is due to torchscript and in particular to these lines in the _DenseLayer class:

https://github.com/pytorch/vision/blob/d98cccb063e3ff67431d572e6f3e9845e5f8aba4/torchvision/models/densenet.py#L68-L78

I'm not sure whether we could remove them in torchvision, but even if we could, there's a chance that other packages would use something similar, and we would break them.

@NicolasHug NicolasHug mentioned this pull request Dec 15, 2021
Closed
@NicolasHug
Copy link
Member Author

Closing:

@NicolasHug NicolasHug closed this Dec 15, 2021
@NicolasHug NicolasHug mentioned this pull request Aug 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NicolasHug @facebook-github-bot