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.

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

[TorchFix] Add TorchUnsafeLoadVisitor #4671

Merged
merged 2 commits into from
Oct 24, 2023
Merged

[TorchFix] Add TorchUnsafeLoadVisitor #4671

merged 2 commits into from
Oct 24, 2023

Conversation

kit1980
Copy link
Member

@kit1980 kit1980 commented Oct 23, 2023

@vercel
Copy link

vercel bot commented Oct 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
torchci ⬜️ Ignored (Inspect) Visit Preview Oct 23, 2023 8:09pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 23, 2023
@kit1980 kit1980 marked this pull request as ready for review October 23, 2023 20:06
@kit1980 kit1980 requested review from malfet and a team October 23, 2023 20:06
@kit1980 kit1980 changed the title Add TorchUnsafeLoadVisitor [TorchFix] Add TorchUnsafeLoadVisitor Oct 24, 2023
@malfet malfet merged commit 4038bbf into main Oct 24, 2023
3 checks passed
@malfet malfet deleted the sdym/torchfix-load branch October 24, 2023 21:48
kit1980 added a commit that referenced this pull request Nov 9, 2023
#4671 added linter-only
`TorchUnsafeLoadVisitor`, but it turned out that the issue is so
widespread that manual fixes would be tedious.

The codemod is somewhat unsafe correctness-wise because full pickling
functionality may still be needed even without `pickle_module`, but I
think it's OK because it fixes a security-related issue and the codemods
need to be verified anyway.

Maybe later we should add something like Ruff's recently added
`--unsafe-fixes`: https://docs.astral.sh/ruff/linter/#fix-safety

I used this for pytorch/vision#8105
@vadimkantorov
Copy link

vadimkantorov commented Nov 10, 2023

@malfet In some circumstances (or always), weights_only leads to a nasty Deprecation Warning :( pytorch/pytorch#52181 (comment)

I'd say, to be ready for prime-time / or becoming default, this Deprecation Warning needs to go, otherwise it looks not the best practice for users, I'd say

@kit1980
Copy link
Member Author

kit1980 commented Nov 10, 2023

@vadimkantorov I've got some very early attempt at the warning, but it's in progress pytorch/pytorch#113498

kit1980 added a commit to pytorch-labs/torchfix that referenced this pull request Nov 23, 2023
pytorch/test-infra#4671 added linter-only
`TorchUnsafeLoadVisitor`, but it turned out that the issue is so
widespread that manual fixes would be tedious.

The codemod is somewhat unsafe correctness-wise because full pickling
functionality may still be needed even without `pickle_module`, but I
think it's OK because it fixes a security-related issue and the codemods
need to be verified anyway.

Maybe later we should add something like Ruff's recently added
`--unsafe-fixes`: https://docs.astral.sh/ruff/linter/#fix-safety

I used this for pytorch/vision#8105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants