-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix(sft): support functools.partial model.forward in chunked CE patch #6486
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| # Copyright 2020-2026 The HuggingFace Team. All rights reserved. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| import functools | ||
| import inspect | ||
| import types | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import torch | ||
| import torch.nn as nn | ||
|
|
||
| from trl.trainer.sft_trainer import _patch_chunked_ce_lm_head | ||
|
|
||
|
|
||
| class _TinyLM(nn.Module): | ||
| def __init__(self): | ||
| super().__init__() | ||
| self.config = MagicMock() | ||
| self.config.text_config = None | ||
| # top-level config attrs used when is_vlm=False | ||
| self.config.final_logit_softcapping = None | ||
| self.config.logit_scale = 1.0 | ||
| self.config.output_router_logits = False | ||
| self.lm_head = nn.Linear(4, 8, bias=False) | ||
| self.base_model = MagicMock() | ||
|
|
||
| def get_output_embeddings(self): | ||
| return self.lm_head | ||
|
|
||
| def forward(self, input_ids=None, attention_mask=None, labels=None, **kwargs): | ||
| return MagicMock(loss=torch.tensor(0.0), logits=None) | ||
|
|
||
|
|
||
| def test_patch_chunked_ce_accepts_partial_forward(): | ||
| """Qwen3.5-style models expose forward as functools.partial (#6483).""" | ||
| model = _TinyLM() | ||
| real = model.forward | ||
| model.forward = functools.partial(real) # no __func__ | ||
|
|
||
| # Should not raise AttributeError: 'partial' object has no attribute '__func__' | ||
| _patch_chunked_ce_lm_head(model, chunk_size=2, is_vlm=False) | ||
|
|
||
| assert isinstance(model.forward, types.MethodType) | ||
| sig = inspect.signature(model.forward) | ||
| # Bound method signature should include original kwargs surface | ||
| assert "input_ids" in sig.parameters or len(sig.parameters) >= 1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| # limitations under the License. | ||
|
|
||
| import contextlib | ||
| import functools | ||
| import inspect | ||
| import json | ||
| import os | ||
|
|
@@ -373,8 +374,16 @@ def _chunked_ce_forward( | |
|
|
||
| # Keep the original forward signature so `generate`'s `_validate_model_kwargs` still sees the | ||
| # model's real inputs (e.g. VLM `pixel_values`, `spatial_shapes`) and doesn't reject them. The | ||
| # unbound `__func__` signature makes `MethodType`'s `self`-stripping land correctly. | ||
| _chunked_ce_forward.__signature__ = inspect.signature(original_forward.__func__) | ||
| # unbound function signature makes `MethodType`'s `self`-stripping land correctly. | ||
| # | ||
| # Some multimodal models (e.g. Qwen3.5) expose `forward` as a `functools.partial` rather than a | ||
| # bound method, so `original_forward.__func__` is unavailable — unwrap partials / fall back to | ||
| # the callable itself (see #6483). | ||
| forward_for_sig = original_forward | ||
| while isinstance(forward_for_sig, functools.partial): | ||
| forward_for_sig = forward_for_sig.func | ||
| unbound = getattr(forward_for_sig, "__func__", forward_for_sig) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defensive getattr on forward unwrapLow Severity The new signature path uses Triggered by project rule: ../.ai/AGENTS.md Reviewed by Cursor Bugbot for commit ca1a551. Configure here. |
||
| _chunked_ce_forward.__signature__ = inspect.signature(unbound) | ||
| model.forward = types.MethodType(_chunked_ce_forward, model) | ||
|
|
||
|
|
||
|
|
||


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.
Vacuous signature test assertion
Low Severity
The new test claims to check that the patched bound-method signature still exposes the original kwargs surface, but the assertion accepts any non-empty parameter list. A broken unwrap that drops
input_idsafterMethodTypeself-stripping would still pass, so the regression this PR guards against is not actually covered.Reviewed by Cursor Bugbot for commit ca1a551. Configure here.