-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Flip default on weights_only #137602
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
Flip default on weights_only #137602
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/137602
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 27c7380 with merge base 73fde0d ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
[ghstack-poisoned]
[ghstack-poisoned]
@@ -1 +1 @@ | |||
2eb4a60ed14a38260b85b0c765161f0ce45be6d1 | |||
f71c02d1f457d58371e013632efb016c01bd1866 |
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.
This was intentional to include pytorch/xla#8259
I'm not sure if it's OK to use different default in fbcode/OSS. |
Also this will break a lot of people. |
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.
Tentatively LGTM, but please change the signatures a bit further. And not IS_FBCODE
feels a bit suspicious, perhaps better behavior would be to add weights_only=False
to respective callsights?
@@ -1 +1 @@ | |||
2eb4a60ed14a38260b85b0c765161f0ce45be6d1 | |||
f71c02d1f457d58371e013632efb016c01bd1866 |
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.
This is not intended, is it?
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.
This is intended, see #137602 (comment)
torch/serialization.py
Outdated
@@ -1159,7 +1160,7 @@ def load( | |||
map_location: MAP_LOCATION = None, | |||
pickle_module: Any = None, | |||
*, | |||
weights_only: Optional[bool] = None, | |||
weights_only: Optional[bool] = not IS_FBCODE, |
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.
if you are changing it to hard boolean, please remove Optional
and delete the logic associated with opt-in checks
weights_only: Optional[bool] = not IS_FBCODE, | |
weights_only: bool = not IS_FBCODE, |
And just FYI users can use TorchFix to find and update call sites where weights_only is not explicitly set: https://github.com/pytorch-labs/torchfix?tab=readme-ov-file#tor102-torchload-without-weights_only-parameter-is-unsafe |
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.
A couple more pieces need to be added to the CI and we should announce this one as a bc-breaking change broadly for sure!
But yes let's do it!
Summary: Some fixes for pytorch/pytorch#137602 Reviewed By: xuzhao9 Differential Revision: D64628614 Pulled By: mikaylagawarecki
Summary: Some fixes for pytorch/pytorch#137602 Reviewed By: xuzhao9 Differential Revision: D64628614 Pulled By: mikaylagawarecki
Summary: Some fixes for pytorch/pytorch#137602 Reviewed By: xuzhao9 Differential Revision: D64628614 Pulled By: mikaylagawarecki
Summary: Some fixes for pytorch/pytorch#137602 Pull Request resolved: #2514 Reviewed By: xuzhao9 Differential Revision: D64628614 Pulled By: mikaylagawarecki fbshipit-source-id: edebf25cc6648919d5673a3baeaffdac26e5b91f
[ghstack-poisoned]
Prevent same global from being added multiple times Pull Request resolved: pytorch#139303 Approved by: https://github.com/janeyx99 ghstack dependencies: pytorch#138936, pytorch#139221, pytorch#139433, pytorch#139541, pytorch#137602
…139810) After pytorch#137602, the default `weights_only` has been set to True. This test is failing in trunk slow jobs atm benchmark_utils/test_benchmark_utils.py::TestBenchmarkUtils::test_collect_callgrind [GH job link](https://github.com/pytorch/pytorch/actions/runs/11672436111/job/32502454946) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/1aa71be56c39908893273bd9558b127159e1ef3a) Pull Request resolved: pytorch#139810 Approved by: https://github.com/kit1980
Starting from version 2.4 PyTorch introduces a stricter check for the objects which can be loaded with torch.load(). Starting from version 2.6 loading with weights_only=True requires allowlisting of such objects. This commit adds allowlist of some numpy objects used to load model checkpoints. Usage is restricted by context manager. User can still additionally call torch.serialization.add_safe_globals() to add other objects into the safe globals list. Accelerate library also stepped into same problem and addressed it with PR-3036. Fixes: huggingface#34631 See: pytorch/pytorch#137602 See: https://pytorch.org/docs/stable/notes/serialization.html#torch.serialization.add_safe_globals See: huggingface/accelerate#3036 Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Starting from version 2.4 PyTorch introduces a stricter check for the objects which can be loaded with torch.load(). Starting from version 2.6 loading with weights_only=True requires allowlisting of such objects. This commit adds allowlist of some numpy objects used to load model checkpoints. Usage is restricted by context manager. User can still additionally call torch.serialization.add_safe_globals() to add other objects into the safe globals list. Accelerate library also stepped into same problem and addressed it with PR-3036. Fixes: #34631 See: pytorch/pytorch#137602 See: https://pytorch.org/docs/stable/notes/serialization.html#torch.serialization.add_safe_globals See: huggingface/accelerate#3036 Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
…139810) After pytorch#137602, the default `weights_only` has been set to True. This test is failing in trunk slow jobs atm benchmark_utils/test_benchmark_utils.py::TestBenchmarkUtils::test_collect_callgrind [GH job link](https://github.com/pytorch/pytorch/actions/runs/11672436111/job/32502454946) [HUD commit link](https://hud.pytorch.org/pytorch/pytorch/commit/1aa71be56c39908893273bd9558b127159e1ef3a) Pull Request resolved: pytorch#139810 Approved by: https://github.com/kit1980
Starting from version 2.4 PyTorch introduces a stricter check for the objects which can be loaded with torch.load(). Starting from version 2.6 loading with weights_only=True requires allowlisting of such objects. This commit adds allowlist of some numpy objects used to load model checkpoints. Usage is restricted by context manager. User can still additionally call torch.serialization.add_safe_globals() to add other objects into the safe globals list. Accelerate library also stepped into same problem and addressed it with PR-3036. Fixes: huggingface#34631 See: pytorch/pytorch#137602 See: https://pytorch.org/docs/stable/notes/serialization.html#torch.serialization.add_safe_globals See: huggingface/accelerate#3036 Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
Stack from ghstack (oldest at bottom):
cc @H-Huang @awgu @kwen2501 @wanchaol @fegin @fduwjj @wz337 @wconstab @d4l3k @c-p-i-o @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @rec