tests: skip _assert_params_superset when upstream forward is (*args, **kwargs)#651
Conversation
…**kwargs) Generalises the Pixtral-specific tolerance from #650 to all drift detectors that use ``_assert_params_superset``. Newer transformers wrap an expanding set of model forwards as ``(self, *args, **kwargs)`` via attention-implementation dispatch and generic decorators. Recent casualties on transformers 4.57.6 include: - tests/test_temporary_patches_exhaustive.py ::test_csm_depth_decoder_for_causal_lm_forward_named_params ::test_csm_for_conditional_generation_forward_named_params The zoo patches at those sites pass through kwargs verbatim, so the call sites stay valid; the static param-name pin just is not meaningful against a passthrough wrapper. Skip with a descriptive message instead of emitting spurious DRIFT. The original FAIL branch is preserved for the case where required names are missing AND the function does not take **kwargs (a real upstream removal). Strict behaviour on transformers 4.x with introspectable signatures is unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27fae0d2b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ): | ||
| got = _param_names(func) | ||
| missing = [name for name in required if name not in got] | ||
| if missing and _has_var_keyword(func): |
There was a problem hiding this comment.
This treats any signature with a **kwargs catch-all as an opaque passthrough, but several callers verified by this helper still rely on explicit positional parameters. For example, test_gemma4_text_decoder_layer_init_signature requires layer_idx, and the patch calls _original_decoder_init(self, config, layer_idx) positionally; if upstream changed to __init__(self, config, **kwargs), this new branch would skip the drift test even though that call would raise from the extra positional argument. Please restrict the skip to true generic wrappers such as (self, *args, **kwargs) or keep positional call sites strict.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the _assert_params_superset function in the exhaustive test suite to skip static parameter verification when functions use variable keywords, which handles newer transformer versions that wrap signatures with **kwargs. The reviewer suggests optimizing the implementation by caching the function signature to avoid redundant introspection calls and recommends removing a manual check in test_pixtral_attention_forward_signature that has become redundant due to this change.
| ): | ||
| got = _param_names(func) | ||
| missing = [name for name in required if name not in got] | ||
| if missing and _has_var_keyword(func): |
There was a problem hiding this comment.
Calling _has_var_keyword(func) here introduces a redundant call to inspect.signature(func). Since _param_names(func) at line 171 already performs this introspection, and the failure block at line 187 may do it a third time, it would be more efficient to obtain the signature once at the beginning of _assert_params_superset and reuse it for all subsequent checks. While performance in tests is often secondary, inspect.signature is relatively expensive and used here up to three times on the same object.
| if missing and _has_var_keyword(func): | ||
| # Newer transformers wrap many upstream forwards as | ||
| # ``(self, *args, **kwargs)`` (attention-impl dispatch, generic | ||
| # decorators, etc.). The zoo patch's call site stays valid because | ||
| # kwargs flow through verbatim; static param-name verification is | ||
| # not meaningful for a wrapped signature. Skip rather than emit | ||
| # spurious DRIFT. | ||
| pytest.skip( | ||
| f"{label} is wrapped as `(self, *args, **kwargs)` on " | ||
| f"transformers {_TX_VERSION}; static param-name verification " | ||
| "is not meaningful for this passthrough signature." | ||
| ) |
There was a problem hiding this comment.
The generalization of this skip logic in _assert_params_superset makes the manual check in test_pixtral_attention_forward_signature (lines 1200-1205) redundant. Since the PR's intent is to generalize the Pixtral-specific tolerance, that manual check should be removed to maintain a clean test suite and avoid logic duplication.
1 participant
Summary
Follow-up to #650. Generalises the Pixtral-specific tolerance to all drift detectors that use `_assert_params_superset`. Newer transformers (already on 4.57.6) wrap an expanding set of upstream forwards as `(self, *args, **kwargs)` via attention-implementation dispatch and generic decorators.
Recent casualties surfaced on the unsloth Core matrix:
Both expect specific named params on `CsmDepthDecoderForCausalLM.forward` / `CsmForConditionalGeneration.forward`, but on transformers 4.57.6 those forwards are now `(self, *args, **kwargs)`.
Fix
`_assert_params_superset` already has access to the `_has_var_keyword` helper. Before failing on "missing" required names, check whether the function is a passthrough wrapper (`**kwargs` present) and skip with a descriptive message in that case. The zoo patches at those sites forward kwargs verbatim, so the call sites stay valid; the static param-name pin just is not meaningful against a passthrough wrapper.
Strict FAIL behaviour is preserved when required names are missing and the function does not accept `**kwargs` (a real upstream removal).
Test plan