8000 Pass `torch.load(weights_only=)` internally to avoid FutureWarning by awaelchli · Pull Request #130663 · pytorch/pytorch · GitHub
[go: up one dir, main page]

Skip to content

Pass torch.load(weights_only=) internally to avoid FutureWarning #130663

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

Closed
wants to merge 4 commits into from

Conversation

awaelchli
Copy link
Contributor
@awaelchli awaelchli commented Jul 13, 2024

Copy link
pytorch-bot bot commented Jul 13, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/130663

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit e0275f8 with merge base 9df4bc6 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added module: distributed_checkpoint oncall: distributed Add this issue/PR to distributed oncall triage queue labels Jul 13, 2024
@bdhirsh bdhirsh requested a review from mikaylagawarecki July 13, 2024 01:51
@bdhirsh bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 13, 2024
@ppwwyyxx
Copy link
Collaborator

There is also:

torch24/lib/python3.10/site-packages/torch/storage.py:414: FutureWarning: You are using `torch.load` with `weights_only=False` (the current default value), which uses the default pickle
 module implicitly. It is possible to construct malicious pickle
8000
 data which will execute arbitrary code during unpickling (See https://github.com/pytorch/pytorch/blob/main/SECURITY.md#untrusted-models for more det
ails). In a future release, the default value for `weights_only` will be flipped to `True`. This limits the functions that could be executed during unpickling. Arbitrary objects will no longer be allowed to be loa
ded via this mode unless they are explicitly allowlisted by the user via `torch.serialization.add_safe_globals`. We recommend you start setting `weights_only=True` for any use case where you don't have full contro
l of the loaded file. Please open an issue on GitHub for any issues related to this experimental feature.
  return torch.load(io.BytesIO(b))

@awaelchli
Copy link
Contributor Author

@ppwwyyxx I believe you are referring to this line, that's already taken care of by a previous fix.

Copy link
Contributor
@malfet malfet left a comment

Choose a reason for hiding this comment

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

Sure, but why not change it to True? Are you expecting to store something that is unsafe?

Copy link
Contributor
@mikaylagawarecki mikaylagawarecki left a comment

Choose a reason for hiding this comment

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

For weights_only=True, if there is a known list of GLOBALs that might be encountered (e.g. for DTensor) that we expect to be safe we can use the

with torch.serialization.safe_globals([bla]): context manager

e.g. for DTensor, [DTensor, DeviceMesh, Shard, DTensorSpec, TensorMeta] might be needed

@@ -108,7 +108,7 @@ def _broadcast_object(
)
dist.broadcast(data_recv_tensor, src=src_rank, group=group, async_op=False)
buffer = io.BytesIO(data_recv_tensor.cpu().numpy())
obj = torch.load(buffer, map_location=device)
obj = torch.load(buffer, map_location=device, weights_only=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the code above, we can see this is broadcasting an object pickled to bytes, so loading here means unpickling. The function doesn't make any assumptions like obj being a tensor, so we need to set weights_only=False here.

@awaelchli
Copy link
Contributor Author

@mikaylagawarecki @malfet The reason I set them to False is simply to ensure I don't break anything, since False was implicitly the default before. For the distributed checkpoint utilities, I don't know whether we can make the assumptions that only tensors are in the checkpoint.

@wz337
Copy link
Contributor
wz337 commented Jul 15, 2024

@mikaylagawarecki @malfet The reason I set them to False is simply to ensure I don't break anything, since False was implicitly the default before. For the distributed checkpoint utilities, I don't know whether we can make the assumptions that only tensors are in the checkpoint.

Switching to weights_only=True would probably break things. One use case is for TrainingState here. https://github.com/pytorch/pytorch/blob/main/test/distributed/checkpoint/e2e/test_e2e_save_and_load.py#L100
Also, we are not sure whether users have similar use case so if we make the switch now, users might experience disruption.

Copy link
Contributor
@LucasLLC LucasLLC left a comment

Choose a reason for hiding this comment

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

lgtm!

@awaelchli
Copy link
Contributor Author
awaelchli commented Jul 15, 2024

Could someone approve the CI workflow again, thx. I didn't know my push would disable it again.

@awaelchli
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 15, 2024
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR needs a release notes: label
If your changes are user facing and intended to be a part of release notes, please use a label starting with release notes:.

If not, please add the topic: not user facing label.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "topic: not user facing"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Details for Dev Infra team Raised by workflow job

@awaelchli
Copy link
Contributor Author

@pytorchbot label "release notes: python_frontend"

@pytorch-bot pytorch-bot bot added the release notes: python_frontend python frontend release notes category label Jul 15, 2024
@awaelchli
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@atalman
Copy link
Contributor
atalman commented Aug 15, 2024

@pytorchbot cherry-pick --onto release/2.4 -c critical --fixes #130658

pytorchbot pushed a commit that referenced this pull request Aug 15, 2024
@pytorchbot
Copy link
Collaborator

Cherry picking #130663

The cherry pick PR is at #133594 and it is linked with issue #130658. The following tracker issues are updated:

Details for Dev Infra team Raised by workflow job

atalman pushed a commit that referenced this pull request Aug 20, 2024
…133594)

Pass `torch.load(weights_only=)` internally to avoid FutureWarning (#130663)

Fixes #130658

Pull Request resolved: #130663
Approved by: https://github.com/malfet, https://github.com/LucasLLC

(cherry picked from commit ad314a2)

Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: distributed Add this issue/PR to distributed oncall triage queue open source release notes: python_frontend python frontend release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal uses of torch.load are missing weights_only and raise FutureWarning
10 participants
0