-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
[DCP] overwrites existing checkpoint by default #125877
Conversation
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/125877
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 94cd15b with merge base 96bdb7a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This pull request was exported from Phabricator. Differential Revision: D57186174 |
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) ghstack-source-id: 225828668 Pull Request resolved: #125877
self.assertTrue(torch.allclose(sd["random"], t2)) | ||
|
||
dcp.save( | ||
{"random": t2}, storage_writer=FsspecWriter(self.temp_dir, overwrite=True) |
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.
Should this be overwrite=False when exception is raised, As the default is overwrite=True.
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.
yes, and the format for assert raises is wrong as well, I'll have to take a look
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.
fixed, thanks @wz337 !
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]
This pull request was exported from Phabricator. Differential Revision: D57186174 |
Pull Request resolved: #125877 Checks for existing checkpoints and overwrites, based on an `overwrite` flag ghstack-source-id: 225839692 @exported-using-ghexport Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/)
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.
It's good to have the overwritten feature. But shouldn't this be an opt-in feature? Just like the cp
linux command will explicitly ask if users would want to overwrite the existing checkpointing. If we do this implicitly, would it cause some surprises to the users?
good point, will update |
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]
This pull request was exported from Phabricator. Differential Revision: D57186174 |
Pull Request resolved: #125877 Checks for existing checkpoints and overwrites, based on an `overwrite` flag ghstack-source-id: 226331209 @exported-using-ghexport Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/)
Will fix errors before merging |
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) cc mrshenli pritamdamania87 zhaojuanmao satgera gqchen aazzolini osalpekar jiayisuse H-Huang kwen2501 awgu penguinwu fegin XilunWu wanchaol fduwjj wz337 tianyu-l wconstab yf225 chauhang d4l3k [ghstack-poisoned]
This pull request was exported from Phabricator. Differential Revision: D57186174 |
Pull Request resolved: #125877 Checks for existing checkpoints and overwrites, based on an `overwrite` flag ghstack-source-id: 226452355 @exported-using-ghexport Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/)
We decided actually that setting I added a warning when we detect a checkpoint, and will change |
@pytorchbot merge |
Merge startedYour 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 |
Checks for existing checkpoints and overwrites, based on an `overwrite` flag Differential Revision: [D57186174](https://our.internmc.facebook.com/intern/diff/D57186174/) Pull Request resolved: pytorch#125877 Approved by: https://github.com/fegin
Stack from ghstack (oldest at bottom):
Checks for existing checkpoints and overwrites, based on an
overwrite
flagDifferential Revision: D57186174
cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @chauhang @d4l3k