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

Adding checkpoint backwards compatibility tests after 0.23.0 release #3377

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

bigning
Copy link
Contributor

@bigning bigning commented Jun 6, 2024

What does this PR do?

Following the release processing doc, this is the last step to add checkpoint backward compatibility tests

Daily test: https://github.com/mosaicml/composer/actions/runs/9407180074

@bigning bigning marked this pull request as ready for review June 6, 2024 17:44
Copy link
Contributor

@snarayan21 snarayan21 left a comment

Choose a reason for hiding this comment

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

Can you run dailies and make sure they pass before merging this? And it would be helpful to link the daily test run in a comment

Copy link
Contributor

@mvpatel2000 mvpatel2000 left a comment

Choose a reason for hiding this comment

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

LGTM. Agree with Saaketh, can we fire off dailies to verify?

@bigning
Copy link
Contributor Author

bigning commented Jun 6, 2024

@mvpatel2000 , @snarayan21 sounds good. I will fire Daily test after the CI issues are resolved.

@bigning
Copy link
Contributor Author

bigning commented Jun 6, 2024

Can you run dailies and make sure they pass before merging this? And it would be helpful to link the daily test run in a comment

The daily test indeed failed test_fsdp_load_old_checkpoint. It seems expected because 2.1/2.2 can't load checkpoint saved by 2.3. I see it skipped composer 0.22.0. I guess I should do a similar change here. @snarayan21 , is my understanding correct ? I started a new daily test.

@snarayan21
Copy link
Contributor

@bigning yes, that's correct. we don't need to ensure that torch 2.1 and 2.2 users can load torch 2.3 checkpoints, only that torch 2.3 users can load old checkpoints for backwards compat.

@bigning bigning merged commit 07d5d3a into dev Jun 6, 2024
37 checks passed
@bigning bigning deleted the after_release branch June 6, 2024 21:22
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.

5 participants