Fix torch.hub for new zipfile format. #42333
Conversation
💊 CI failures summary and remediationsAs of commit 9d5e4ef (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet:
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 10 times. |
|
@jamesr66a can you review? |
jamesr66a
left a comment
jamesr66a
left a comment
There was a problem hiding this comment.
LGTM barring one thing
torch/hub.py
Outdated
| # then fall back to the old way if that fails. | ||
| try: | ||
| return torch.load(cached_file, map_location=map_location) | ||
| except RuntimeError: |
There was a problem hiding this comment.
Not thrilled that this catches any RuntimeError. Is it possible for us to define a torch.is_loadable or something that checks specifically for that condition? Otherwise this seems fine
torch/hub.py
Outdated
| with zipfile.ZipFile(cached_file) as cached_zipfile: | ||
| members = cached_zipfile.infolist() | ||
| if len(members) != 1: | ||
| raise RuntimeError('Only one file(not dir) is allowed in the zipfile') |
There was a problem hiding this comment.
should we change some documentation or something to not recommend this file format anymore?
There was a problem hiding this comment.
and also maybe change the RuntimeError (if this is no longer the recommended serialization option) for something like: "Falling back to old format...". Because otherwise users will see this error if they say, try to load a corrupted new format, right?
ailzhang
force-pushed
the
fix_hub_load_zipfile
branch
from
6a603ff to
f48a46c
Compare
ailzhang
force-pushed
the
fix_hub_load_zipfile
branch
from
f48a46c to
9d5e4ef
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@ailzhang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
5 participants
Fixes #42239