-
Notifications
You must be signed in to change notification settings - Fork 430
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
Remove save overwrite #3431
Remove save overwrite #3431
Conversation
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.
Need approval from either Ning or Evan. Did we not test that save_overwrite=True and autoresume=True threw an error before?
We did, I removed test |
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.
looks good to me besides maybe adding a warning.
if save_overwrite: | ||
error_message += textwrap.dedent( | ||
'The flag `save_overwrite` must be False when autoresume is enabled as autoresume always loads the ' | ||
'latest existing checkpoint in `save_folder`. ', | ||
) |
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.
maybe add a warning here instead?
oh too late |
* remove save overwrite * fix tests * lint * remove bad test
* remove save overwrite * fix tests * lint * remove bad test
What does this PR do?
Previously, we required
save_overwrite=False
as it was not clear if a user intended to overwrite or resume from a checkpoint.However, it turns out this situation is well defined. Autoresume = resume from symlink file. It is a transformation on load_path, and nothing else. So, you can actually set autoresume on and save_overwrite to True, which would be useful for partial checkpoint uploads where the symlink file is not added.