-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Improve PEFT integration #4723
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
Improve PEFT integration #4723
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvement.
CI is red:
AttributeError: 'list' object has no attribute 'keys'| def __init__( | ||
| self, | ||
| model: str | PreTrainedModel, | ||
| model: "str | PreTrainedModel | PeftModel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| model: "str | PreTrainedModel | PeftModel", | |
| model: str | PreTrainedModel | "PeftModel", |
| def __init__( | ||
| self, | ||
| model: str | PreTrainedModel, | ||
| model: "str | PreTrainedModel | PeftModel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| model: "str | PreTrainedModel | PeftModel", | |
| model: str | PreTrainedModel | "PeftModel", |
| def __init__( | ||
| self, | ||
| model: str | PreTrainedModel, | ||
| model: "str | PreTrainedModel | PeftModel", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| model: "str | PreTrainedModel | PeftModel", | |
| model: str | PreTrainedModel | "PeftModel", |
edbeeching
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the type annotations could clearer with a small change, but otherwise LGTM
Agree, but it seems not correct: from peft import PeftModel
def func(model: str | "PeftModel"): ... |
This PR makes four changes:
modelargument inSFTTrainer,GRPOTrainer,RLOOTrainer, andRewardTrainerto allowPeftModel.is_peft_modelinstead ofisinstance(model, PeftModel). The two checks are equivalent, butis_peft_modelis more explicit, so this PR adopts it consistently across the codebase.use_adapter, a context manager that temporarily selects an adapter (this is useful for point 4).ref_model(GRPO, RLOO), when the providedmodelis aPeftModel, the adapter is cloned under the name"ref". Previously, the reference model was obtained by just disabling the adapter, which is only correct when the adapter is newly initialized—not when it has already been pretrained. An example of this issue can be found in Allow swapping PEFT adapters for target/ref model. #1193.Note that we don't apply these changes for DPO as there is currently a refactoring in #3906