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

Move hooks and fsdp modules onto state rather than trainer #3522

Merged
merged 23 commits into from
Aug 6, 2024

Conversation

JackZ-db
Copy link
Contributor

@JackZ-db JackZ-db commented Aug 5, 2024

Eval'ing after resuming from checkpointing broke since calls to prepare_fsdp_module in state.py didn't modify the handles and FSDP modules in modules. We fix this by moving automicrobatch_fsdp_hook_handles and fsdp_modules into State instance variables instead of Trainer instance variables.

Fixed regression test that previously broken: mpt-7b-composer-eval-regression-KkY29t

tests/checkpoint/test_load.py Outdated Show resolved Hide resolved
tests/checkpoint/test_load.py Outdated Show resolved Hide resolved
composer/checkpoint/load.py Outdated Show resolved Hide resolved
JackZ-db and others added 4 commits August 5, 2024 15:28
Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com>
Co-authored-by: Mihir Patel <mihir.v.patel7@gmail.com>
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, please ensure manual testing works

@JackZ-db
Copy link
Contributor Author

JackZ-db commented Aug 5, 2024

LGTM, please ensure manual testing works

Requested rerun from research

Copy link
Contributor

@bigning bigning left a comment

Choose a reason for hiding this comment

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

LGTM!

@JackZ-db JackZ-db merged commit 2fdfd12 into mosaicml:dev Aug 6, 2024
14 checks passed
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