Skip to content

Conversation

@qgallouedec
Copy link
Member

@qgallouedec qgallouedec commented Dec 19, 2025

This PR makes four changes:

  1. Fixes the type hints and documentation for the model argument in SFTTrainer, GRPOTrainer, RLOOTrainer, and RewardTrainer to allow PeftModel.
  2. Uses is_peft_model instead of isinstance(model, PeftModel). The two checks are equivalent, but is_peft_model is more explicit, so this PR adopts it consistently across the codebase.
  3. Introduces use_adapter, a context manager that temporarily selects an adapter (this is useful for point 4).
  4. For methods that use a ref_model (GRPO, RLOO), when the provided model is a PeftModel, 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

@HuggingFaceDocBuilderDev

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.

@qgallouedec qgallouedec changed the title [WIP] Improve PEFT integration Improve PEFT integration Dec 19, 2025
Copy link
Member

@albertvillanova albertvillanova left a 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'

Base automatically changed from disallow-peft-model-with-config to main December 19, 2025 15:09
def __init__(
self,
model: str | PreTrainedModel,
model: "str | PreTrainedModel | PeftModel",
Copy link
Collaborator

@edbeeching edbeeching Jan 5, 2026

Choose a reason for hiding this comment

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

Suggested change
model: "str | PreTrainedModel | PeftModel",
model: str | PreTrainedModel | "PeftModel",

def __init__(
self,
model: str | PreTrainedModel,
model: "str | PreTrainedModel | PeftModel",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
model: "str | PreTrainedModel | PeftModel",
model: str | PreTrainedModel | "PeftModel",

def __init__(
self,
model: str | PreTrainedModel,
model: "str | PreTrainedModel | PeftModel",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
model: "str | PreTrainedModel | PeftModel",
model: str | PreTrainedModel | "PeftModel",

Copy link
Collaborator

@edbeeching edbeeching left a 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

@qgallouedec
Copy link
Member Author

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"): ...
Traceback (most recent call last):
  File "/fsx/qgallouedec/trl/dem.py", line 3, in <module>
    def func(model: str | "PeftModel"): ...
                    ~~~~^~~~~~~~~~~~~
TypeError: unsupported operand type(s) for |: 'type' and 'str'

@qgallouedec qgallouedec enabled auto-merge (squash) January 6, 2026 18:02
@qgallouedec qgallouedec disabled auto-merge January 6, 2026 18:03
@qgallouedec qgallouedec merged commit 42cce51 into main Jan 6, 2026
11 checks passed
@qgallouedec qgallouedec deleted the better-peft-integration branch January 6, 2026 18:38
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