Skip to content

tests: skip _assert_params_superset when upstream forward is (*args, **kwargs) #651

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.

Sign up for GitHub

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

Merged
danielhanchen merged 1 commit into from
May 15, 2026
Merged

tests: skip _assert_params_superset when upstream forward is (*args, **kwargs) #651

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions tests/test_temporary_patches_exhaustive.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,18 @@ def _assert_params_superset(
):
got = _param_names(func)
missing = [name for name in required if name not in got]
if missing and _has_var_keyword(func):

@chatgpt-codex-connector chatgpt-codex-connector Bot May 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Only skip fully generic wrappers

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 gemini-code-assist Bot May 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

# 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."
)
Comment on lines +173 to +184

@gemini-code-assist gemini-code-assist Bot May 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

if missing:
try:
sig = inspect.signature(func)
Expand Down
Loading