Skip to content

Remove hasattr(__slots__) for BUILD logic in weights_only unpickler #139541

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.

Sign up for GitHub

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

Jump to bottom
Closed
mikaylagawarecki wants to merge 1 commit into from
Closed

Remove hasattr(__slots__) for BUILD logic in weights_only unpickler #139541

mikaylagawarecki wants to merge 1 commit into from

Conversation

@mikaylagawarecki
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki commented Nov 2, 2024 •
edited
Loading

This is tested in PR stacked above in

python test/distributed/fsdp/test_fsdp_state_dict.py TestFSDPStateDict.test_torch_save_load

We cannot depend on whether hasattr(..., __slots__) to know whether a BUILD instruction has slotstate. For example, if a class subclasses ABC hasattr(__slots__) will be True but there might be no slots (and hence state will not be a tuple). So revert #138936 to following the pickle library's code

>>> from abc import ABC
>>> hasattr(ABC, "__slots__")
True

So

import torch
from abc import ABC
from dataclasses import dataclass

class Foo(ABC):
    pass

class FooWrapper(Foo):
    def __init__(self, x, y):
        self.x = x
        self.y = y

f = FooWrapper(1, 2)
torch.save(f, "temp.pt")
with torch.serialization.safe_globals([FooWrapper]):
    torch.load("temp.pt")

Would fail on the previous code with

File "/data/users/mg1998/pytorch/torch/serialization.py", line 1934, in _load
    result = unpickler.load()
  File "/data/users/mg1998/pytorch/torch/_weights_only_unpickler.py", line 366, in load
    for k, v in slotstate.items():

As there is actually no slotstate

Stack from ghstack (oldest at bottom):

cc @albanD

@mikaylagawarecki mikaylagawarecki mentioned this pull request Nov 1, 2024
Closed
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 2, 2024 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit cf14ff2 with merge base 73fde0d (image):
💚 Looks good so far! There are no failures yet. 💚

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

@mikaylagawarecki mikaylagawarecki mentioned this pull request Nov 1, 2024
Closed
@mikaylagawarecki mikaylagawarecki added module: python frontend For issues relating to PyTorch's Python frontend topic: bug fixes topic category release notes: python_frontend python frontend release notes category and removed module: python frontend For issues relating to PyTorch's Python frontend labels Nov 2, 2024
pytorchmergebot pushed a commit that referenced this pull request Nov 4, 2024
@mikaylagawarecki @pytorchmergebot
Flip default on weights_only (#137602)
ca43ecd 
Pull Request resolved: #137602
Approved by: https://github.com/malfet, https://github.com/albanD
ghstack dependencies: #138936, #139221, #139433, #139541
pytorchmergebot pushed a commit that referenced this pull request Nov 4, 2024
@mikaylagawarecki @pytorchmergebot
[BE] Change _marked_safe_globals_list to set (#139303)
e947649 
Prevent same global from being added multiple times

Pull Request resolved: #139303
Approved by: https://github.com/janeyx99
ghstack dependencies: #138936, #139221, #139433, #139541, #137602
atalman pushed a commit to atalman/pytorch that referenced this pull request Nov 11, 2024
@mikaylagawarecki @atalman
[BE] Change _marked_safe_globals_list to set (pytorch#139303)
45171ae 
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
Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
@mikaylagawarecki @Ryo-not-rio
Flip default on weights_only (pytorch#137602)
c1fab91 
Pull Request resolved: pytorch#137602
Approved by: https://github.com/malfet, https://github.com/albanD
ghstack dependencies: pytorch#138936, pytorch#139221, pytorch#139433, pytorch#139541
Ryo-not-rio pushed a commit to Ryo-not-rio/pytorch that referenced this pull request Dec 2, 2024
@mikaylagawarecki @Ryo-not-rio
[BE] Change _marked_safe_globals_list to set (pytorch#139303)
fca4184 
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
@github-actions github-actions bot deleted the gh/mikaylagawarecki/284/head branch December 5, 2024 02:13
@Archana-Shinde1 Archana-Shinde1 mentioned this pull request May 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malfet malfet malfet approved these changes

Assignees

No one assigned

Labels

Merged release notes: python_frontend python frontend release notes category topic: bug fixes topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mikaylagawarecki @malfet @pytorchmergebot