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

Make loaded peft adapters optionally trainable #1701

Merged
merged 5 commits into from
Dec 16, 2024
Merged

Conversation

snarayan21
Copy link
Contributor

Previously, loaded peft adapters (from HF) were not trainable. This PR adds the option to continue training peft adapters that are initialized from some existing adapter HF checkpoint.

Added unit tests to make sure adapter parameters are trainable when needed. this requires setting pretrained=True since pretrained_lora_id_or_path is only used in that case.

@snarayan21 snarayan21 marked this pull request as ready for review December 16, 2024 18:55
@snarayan21 snarayan21 requested a review from a team as a code owner December 16, 2024 18:55
@snarayan21 snarayan21 requested a review from dakinggg December 16, 2024 20:28
Copy link
Collaborator

@dakinggg dakinggg left a comment

Choose a reason for hiding this comment

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

do you have any sense why the default is False? mostly just wondering if i'm missing something, cause I'm not sure why they would have the default as fals.

@snarayan21
Copy link
Contributor Author

snarayan21 commented Dec 16, 2024

@dakinggg i think just because they assume that a saved adapter should be used directly for inference by default:
https://huggingface.co/docs/peft/en/package_reference/peft_model#peft.PeftModel.from_pretrained.is_trainable

Like they probably expect most people to specify the peft config if they want to train new adapters instead of instantiating from existing adapters

@snarayan21 snarayan21 enabled auto-merge (squash) December 16, 2024 21:30
@snarayan21 snarayan21 merged commit 5a62fba into main Dec 16, 2024
9 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.

2 participants