Skip to content

Conversation

@mikaylagawarecki
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki commented Jul 31, 2024

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Jul 31, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit c7fd26f with merge base 57d1ffc (image):
💚 Looks good so far! There are no failures yet. 💚

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

[ghstack-poisoned]
[ghstack-poisoned]
…erroneously reported with the weights_only message"

[ghstack-poisoned]
@mikaylagawarecki mikaylagawarecki marked this pull request as ready for review August 13, 2024 18:20
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Not sure what's the impact of this change tbh. Is Pickle handling this type of error specially?
Also should we add a new unit test to ensure whatever this is fixing remains fixed?

@mikaylagawarecki
Copy link
Contributor Author

mikaylagawarecki commented Aug 13, 2024

@albanD main change is that we were always catching any RuntimeError (e.g. those that a corrupted checkpoint would cause) and reporting it with the weights_only message when using weights_only see here and the message here using the UnpicklingError in the unpickler makes sure that we specifically report only weights_only errors with that message and yep sorry will add a test (I think it was tested in the above PR when I set weights_only to True, but then stopped being tested when I flipped the default back to None)

@albanD
Copy link
Collaborator

albanD commented Aug 13, 2024

But aren't other things maybe going to raise this type of error as well? Why use UnpicklingError vs WeightsOnlyFailureError ?

@mikaylagawarecki
Copy link
Contributor Author

mikaylagawarecki commented Aug 13, 2024

Hm not sure how else would an UnpicklingError get raised since we re-implemented the entire Unpickler and don't inherit from pickle.Unpickler, but fair enough, perhaps there's something I missed

@albanD
Copy link
Collaborator

albanD commented Aug 13, 2024

Ho that sounds fair then. Feel free to keep it as is then!

…erroneously reported with the weights_only message"

Caught in above PR #127627 




[ghstack-poisoned]
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM!

…erroneously reported with the weights_only message"

Caught in above PR #127627 




[ghstack-poisoned]
[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Rebased gh/mikaylagawarecki/217/orig onto refs/remotes/origin/viable/strict because #127627 was rebased, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/132349)

pytorchmergebot pushed a commit that referenced this pull request Aug 16, 2024
Tests on XLA shard not fixed yet but there is an issue here pytorch/xla#7799

Pull Request resolved: #127627
Approved by: https://github.com/albanD
ghstack dependencies: #132349
Chao1Han pushed a commit to Chao1Han/pytorch that referenced this pull request Sep 26, 2024
Tests on XLA shard not fixed yet but there is an issue here pytorch/xla#7799

Pull Request resolved: pytorch#127627
Approved by: https://github.com/albanD
ghstack dependencies: pytorch#132349
@github-actions github-actions bot deleted the gh/mikaylagawarecki/242/head branch September 28, 2024 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants