-
Notifications
You must be signed in to change notification settings - Fork 23.5k
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
Remove tensor subclass detection logic from weights_only unpickler #127808
Remove tensor subclass detection logic from weights_only unpickler #127808
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/127808
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit c675f31 with merge base 1b704a1 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
ghstack-source-id: 29dff9bf4e999d3ca4db4d5d74d981bfa1c2beb6 Pull Request resolved: #127808
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, but it would be nice to have at least one test that validate subclasses work via globals. Also, perhaps worth having a safe globals context manager
for method, func in restore_methods.items(): | ||
setattr(subclass, method, func) | ||
a = subclass(torch.randn(2, 3)) | ||
|
||
@skipIfTorchDynamo("name 'SYNTHETIC_LOCAL' is not defined") | ||
def test_safe_globals_for_weights_only(self): |
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.
@malfet This tests that functionality with TwoTensor
, is that what you were looking for?
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: .github/workflows/generated-linux-binary-manywheel-main.yml / manywheel-py3_8-cuda11_8-test / test Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Add context manager mentioned in #127808 (review) Pull Request resolved: #127939 Approved by: https://github.com/albanD
Add context manager mentioned in pytorch#127808 (review) Pull Request resolved: pytorch#127939 Approved by: https://github.com/albanD
Remove logic to auto-detect and allow subclasses that did not override certain methods from the weights_only unpickler from #124331 for 2.4 release
Subclasses should be loadable using
torch.serialization.add_safe_globals
Stack from ghstack (oldest at bottom):
cc @albanD