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.

Sign up for GitHub

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

Jump to bottom

Expose flag for should_save_peft_only #1357

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Expose flag for should_save_peft_only #1357

merged 2 commits into from
Jul 15, 2024

Conversation

irenedea
Copy link
Contributor

@irenedea irenedea commented Jul 13, 2024 •
edited
Loading

This will allow users to control whether they want their composer checkpoints from lora to have the full weights or just the adapter weights.

Note: This only applies to composer checkpoints, not HF checkpoints. HF checkpoints saved through hf_checkpointer.py will still only contain the adapters only.

Tests

should_save_peft_only: False

2024-07-15 21:27:50     844110 .metadata
2024-07-15 21:27:55 4028659956 __0_0.distcp
2024-07-15 21:27:55 4019819348 __1_0.distcp
2024-07-15 21:27:55 4019819348 __2_0.distcp
2024-07-15 21:27:55 4019819348 __3_0.distcp
2024-07-15 21:27:55 4019819348 __4_0.distcp
2024-07-15 21:27:55 4019819348 __5_0.distcp
2024-07-15 21:27:55 4019819348 __6_0.distcp
2024-07-15 21:27:55 4019819348 __7_0.distcp

should_save_peft_only: True

2024-07-15 21:35:06     291838 .metadata
2024-07-15 21:35:06   13185952 __0_0.distcp
2024-07-15 21:35:06    4345344 __1_0.distcp
2024-07-15 21:35:06    4345344 __2_0.distcp
2024-07-15 21:35:06    4345344 __3_0.distcp
2024-07-15 21:35:06    4345344 __4_0.distcp
2024-07-15 21:35:06    4345344 __5_0.distcp
2024-07-15 21:35:06    4345344 __6_0.distcp
2024-07-15 21:35:06    4345344 __7_0.distcp

@irenedea irenedea marked this pull request as ready for review July 15, 2024 22:03
@irenedea irenedea requested a review from a team as a code owner July 15, 2024 22:03
snarayan21
snarayan21 previously requested changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@snarayan21 snarayan21 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

code changes lgtm.
before merging, mind doing a save/resumption test with and without the flag? want to make sure we can resume from checkpoints with this enabled

@snarayan21 snarayan21 dismissed their stale review July 15, 2024 22:17

lol offline discussion

@irenedea irenedea enabled auto-merge (squash) July 15, 2024 22:17
snarayan21
snarayan21 approved these changes Jul 15, 2024
View reviewed changes
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.

lgtm

@irenedea irenedea merged commit 54746bf into mosaicml:main Jul 15, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakinggg dakinggg dakinggg approved these changes

@snarayan21 snarayan21 snarayan21 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@irenedea @snarayan21 @dakinggg