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

Remove save overwrite #3431

Merged

Conversation

mvpatel2000
Copy link
Contributor

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.

Copy link
Contributor

@b-chu b-chu left a 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?

@mvpatel2000
Copy link
Contributor Author

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

@mvpatel2000 mvpatel2000 merged commit 5f8265d into mosaicml:dev Jun 27, 2024
14 checks passed
@mvpatel2000 mvpatel2000 deleted the mvpatel2000/remove-save-overwrite branch June 27, 2024 16:56
Copy link
Contributor

@eracah eracah left a 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.

Comment on lines -1735 to -1739
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`. ',
)
Copy link
Contributor

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?

@eracah
Copy link
Contributor

eracah commented Jun 27, 2024

oh too late

mvpatel2000 added a commit to mvpatel2000/composer that referenced this pull request Jul 21, 2024
* remove save overwrite

* fix tests

* lint

* remove bad test
mvpatel2000 added a commit that referenced this pull request Jul 21, 2024
* remove save overwrite

* fix tests

* lint

* remove bad test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants