fix(sft): support functools.partial model.forward in chunked CE patch#6486
fix(sft): support functools.partial model.forward in chunked CE patch#6486Solaris-star wants to merge 1 commit into
Conversation
_patch_chunked_ce_lm_head assumed model.forward was a bound method and accessed original_forward.__func__ for signature cloning. Multimodal models such as Qwen3.5 expose forward as a functools.partial, which has no __func__ and crashed SFTTrainer init under loss_type=chunked_nll. Unwrap partials and fall back to the callable itself when __func__ is missing. Fixes huggingface#6483 Signed-off-by: Solaris-star <820622658@qq.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ca1a551. Configure here.
| 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.
Defensive getattr on forward unwrap
Low Severity
The new signature path uses getattr with a fallback for __func__, which conflicts with the project simplicity guidance to avoid getattr and defensive fallback branches. The bound-method versus plain-callable cases can be distinguished explicitly after partial unwrapping without that pattern.
Triggered by project rule: ../.ai/AGENTS.md
Reviewed by Cursor Bugbot for commit ca1a551. Configure here.
| 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 |
There was a problem hiding this comment.
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_ids after MethodType self-stripping would still pass, so the regression this PR guards against is not actually covered.
Reviewed by Cursor Bugbot for commit ca1a551. Configure here.
|
Thank you Solaris! |
|
Hello @albertvillanova , could you please you review it? |
2 participants
What does this PR do?
_patch_chunked_ce_lm_headclones the originalmodel.forwardsignature viaoriginal_forward.__func__. That assumes a bound method. Multimodal checkpoints such asQwen3_5ForConditionalGenerationcan exposeforwardas afunctools.partial, which has no__func__, soSFTTrainercrashes during init when applying the chunked CE LM-head patch (even for text-only training with the vision tower frozen).This PR unwraps nested
functools.partialobjects and falls back to the callable itself when__func__is missing, then keeps the existingMethodTyperebinding.Fixes #6483
Before submitting
AI writing disclosure
Who can review?
Anyone in the community is free to review the PR once the tests have passed.
Note
Low Risk
Narrow change to signature extraction during chunked CE patching; behavior for normal bound
forwardmethods is unchanged.Overview
Fixes
SFTTrainerinit crashes whenloss_type='chunked_nll'is used with models whoseforwardis afunctools.partial(e.g. Qwen3.5-style VLMs), where readingoriginal_forward.__func__raisedAttributeError._patch_chunked_ce_lm_headnow unwraps nested partials, then uses__func__when present or the underlying callable forinspect.signature, so the patched forward still exposes the original kwargs forgeneratevalidation.Adds
test_patch_chunked_ce_accepts_partial_forwardto lock in the behavior (#6483).Reviewed by Cursor Bugbot for commit ca1a551. Bugbot is set up for automated code reviews on this repo. Configure here.