[Bugfix] Resolve decorated forwards without functools.wraps in autowrap_forward - #3058
[Bugfix] Resolve decorated forwards without functools.wraps in autowrap_forward#3058malaiwah wants to merge 1 commit into
Conversation
…ap_forward
## Purpose ##
Sequential-pipeline calibration (GPTQ, SparseGPT, AWQ via this pipeline)
cannot trace any model whose module `forward` is wrapped by a decorator
that does not apply `functools.wraps`. Concretely, transformers >= 5.x
decorates `Qwen3_5GatedDeltaNet.forward` with
`@force_accelerate_hooks("conv1d")` (see
transformers/integrations/accelerate.py), which returns an inner
`def wrapped(self, *args, **kwargs)` without copying `__wrapped__` or
`__name__`.
As a result:
- `inspect.getsource(module.forward)` returns the wrapper's source, so
the re-exec'd code defines a function named `wrapped`, not `forward`;
- `namespace["forward"].__get__(module)` raises `KeyError: 'forward'`;
- `inspect.unwrap` does not help because there is no `__wrapped__`
attribute to follow.
## Changes ##
- In `autowrap_forward`, resolve the decorated method to the original
function before calling `inspect.getsource`, by searching the
wrapper's closure cells for a function named `forward`.
- `inspect.getsource` on the original includes its decorator line, so
`exec` re-applies the decorator and preserves its behaviour (e.g.
accelerate hook setup).
- No behaviour change for undecorated forwards: `inspect.unwrap` returns
the method unchanged and the closure search is skipped.
## Testing ##
- Added regression tests in
tests/llmcompressor/pipelines/sequential/test_ast_helpers.py covering
both a no-`functools.wraps` decorated forward (previously raised
`KeyError`) and a plain forward (unchanged behaviour). The decorated
test also asserts the decorator's side effect still fires after
autowrapping, proving the decorator is re-applied.
- Validated end-to-end: a full GPTQ run of Qwen/Qwen3.8-27B (64-layer
hybrid, 48 GDN layers) over 482 subgraphs completed in ~45 minutes on
a single RTX 5090, where it previously could not trace at all.
Signed-off-by: malaiwah <malaiwah@users.noreply.github.com>
|
👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review. Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Merge Protections🔴 1 of 1 protections blocking · waiting on 👀 reviews
🔴 Require one maintainer reviewWaiting for any of
This rule is failing.All PRs must have at least one approving review from a maintainer before merging.
|
There was a problem hiding this comment.
Code Review
This pull request updates autowrap_forward in ast_helpers.py to handle cases where module.forward is wrapped by a decorator that does not use functools.wraps (such as force_accelerate_hooks from transformers) by searching the wrapper's closure cells. It also adds regression tests to verify this behavior. However, a critical issue was identified: because module.forward returns a bound method, it does not have a __closure__ attribute directly (only its underlying __func__ does), which causes the closure-unwrapping logic to be skipped. You should extract the underlying function from the bound method before checking the closure cells.
| target = inspect.unwrap(module.forward) | ||
| # Some decorators (e.g. transformers' `force_accelerate_hooks`, see | ||
| # transformers/integrations/accelerate.py) wrap `forward` with an inner | ||
| # function but do not apply `functools.wraps`. inspect.unwrap therefore | ||
| # cannot recover the original (there is no `__wrapped__` attribute) and | ||
| # inspect.getsource returns the *wrapper's* source, so the re-exec'd code | ||
| # defines a function named `wrapped` rather than `forward` and the | ||
| # `namespace["forward"]` lookup below raises KeyError. Recover the original | ||
| # `forward` by searching the wrapper's closure cells. inspect.getsource on | ||
| # the original includes its decorator line, so exec re-applies the | ||
| # decorator and preserves its behaviour (e.g. accelerate hook setup). | ||
| if getattr(target, "__name__", "") != "forward" and getattr( | ||
| target, "__closure__", None | ||
| ): |
There was a problem hiding this comment.
When module.forward is accessed, Python's descriptor protocol binds the function to the module instance, returning a bound method object (types.MethodType).
inspect.unwrap only follows __wrapped__ chains and does not unwrap bound methods. Therefore, target remains a bound method object. Since bound method objects do not have a __closure__ attribute (only their underlying function __func__ does), getattr(target, "__closure__", None) will always return None. As a result, the entire closure-unwrapping block is skipped, and the original issue remains unresolved for actual module instances.
To fix this, you should first extract the underlying function from the bound method (using inspect.unwrap or accessing __func__ if it is a method) before performing the closure search.
target = inspect.unwrap(module.forward)
# If target is a bound method, extract the underlying function
if inspect.ismethod(target):
target = target.__func__
# Some decorators (e.g. transformers' `force_accelerate_hooks`, see
# transformers/integrations/accelerate.py) wrap `forward` with an inner
# function but do not apply `functools.wraps`. inspect.unwrap therefore
# cannot recover the original (there is no `__wrapped__` attribute) and
# inspect.getsource returns the *wrapper's* source, so the re-exec'd code
# defines a function named `wrapped` rather than `forward` and the
# `namespace["forward"]` lookup below raises KeyError. Recover the original
# `forward` by searching the wrapper's closure cells. inspect.getsource on
# the original includes its decorator line, so exec re-applies the
# decorator and preserves its behaviour (e.g. accelerate hook setup).
if getattr(target, "__name__", "") != "forward" and getattr(
target, "__closure__", None
):
kylesayrs
left a comment
There was a problem hiding this comment.
Thanks for the fix! Could you please
- Break this functionality into a helper function (something like
get_unwrapped_forward) - Make sure that imports are declared at the top module level
Thank you!
SUMMARY:
Fixes #3057.
The sequential-pipeline calibration path (GPTQ, SparseGPT, AWQ via this pipeline) cannot trace any model whose module
forwardis wrapped by a decorator that does not applyfunctools.wraps. This blocks quantization of such models entirely.Concretely, in transformers >= 5.x
Qwen3_5GatedDeltaNet.forwardis decorated with@force_accelerate_hooks("conv1d")(seetransformers/integrations/accelerate.py), which returns an innerdef wrapped(self, *args, **kwargs)without copying__wrapped__or__name__.Root cause
In
src/llmcompressor/pipelines/sequential/ast_helpers.py,autowrap_forwarddoes:Because
inspect.getsource(module.forward)returns the source of the wrapper (the innerdef wrapped(...)), the re-exec'd code defines a function namedwrapped, notforward, andnamespace["forward"]raisesKeyError: 'forward'.inspect.unwrapdoes not help because the wrapper has no__wrapped__attribute to follow.The fix
Resolve the decorated method to the original
forwardfunction before callinginspect.getsource, by searching the wrapper's closure cells for a function namedforward:Why decorator behaviour is preserved
inspect.getsourceon the originalforwardincludes its decorator line, so re-executing the source re-applies the decorator. The accelerate hook setup (and any other wrapper side effects) therefore still take effect on the autowrapped forward. This is important: we are not bypassing the decorator, we are recovering the decorated definition so thattorch.fxautowrapping can be applied to the realforwardbody while the decorator is re-applied around it.There is no behaviour change for undecorated forwards:
inspect.unwrapreturns the method unchanged and the closure search is skipped.TEST PLAN:
tests/llmcompressor/pipelines/sequential/test_ast_helpers.py:test_autowrap_forward_handles_unwrapped_decorator: a module whoseforwardis wrapped by a no-functools.wrapsdecorator (mirroringforce_accelerate_hooks). Previously raisedKeyError: 'forward'; now autowraps successfully, produces correct output, and the decorator's side effect still fires (proving the decorator is re-applied).test_autowrap_forward_plain_module_unchanged: an undecorated forward, asserting behaviour is unchanged.KeyError: 'forward'onmainand pass with this change.ruff checkandruff format --checkpass on the touched files.Qwen/Qwen3.8-27B(64-layer hybrid: 16 full-attention + 48Qwen3_5GatedDeltaNetlayers) over 482 subgraphs completed in ~45 minutes on a single RTX 5090, where onmainit could not trace at all. This confirms tracing/calibration now completes end-to-end for this model class. (Note: this PR claims only that tracing/calibration completes; it makes no claim about the resulting checkpoint quality.)