tests: tolerate transformers 5.x source/signature drift in two zoo drift detectors#650
Conversation
…ift detectors Two of the upstream-drift detectors started reporting DRIFT on transformers >= 5.0 even though zoo's own behaviour at the pinned patch site is correct on those versions: 1. tests/test_compiler_rewriter_exhaustive.py ::test_compiler_logger_running_training_inner_loop_present On transformers 5.x ``Trainer._inner_training_loop`` is wrapped / compiled so ``inspect.getsource`` raises ``OSError``, mirroring the zoo source-rewriter's own no-op fallback for that branch. Skip on 5.x via the existing ``_skip_if_transformers_5x`` helper and treat a residual ``OSError`` / ``TypeError`` on any version as a soft skip rather than a hard DRIFT, with the same wording the detector already documents. 2. tests/test_temporary_patches_exhaustive.py ::test_pixtral_attention_forward_signature transformers 5.x wraps ``PixtralAttention.forward`` as ``(self, *args, **kwargs)`` via attention-implementation dispatch. The zoo patch's call site stays valid because it forwards kwargs verbatim; static param-name verification just isn't meaningful against a ``**kwargs`` signature. Use the existing ``_has_var_keyword`` helper to short-circuit to ``pytest.skip`` in that case before falling through to ``_assert_params_superset``. Both fixes preserve the detector's strict behaviour on transformers 4.x where the pinned signatures / source bodies are still introspectable.
![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: 6a48d60511
ℹ️ 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".
| except (OSError, TypeError): | ||
| _drift( | ||
| "unsloth_zoo/compiler.py:3988-4040", | ||
| "inspect.getsource(Trainer._inner_training_loop)", | ||
| "transformers.trainer.Trainer", | ||
| "Source unavailable; the whole inner-training-loop rewriter " | ||
| "skips and `_fast_inner_training_loop` is never installed.", | ||
| pytest.skip( |
There was a problem hiding this comment.
This catch now skips for every transformers version/build, but the 5.x case was already handled above by _skip_if_transformers_5x. On a 4.x build where inspect.getsource(Trainer._inner_training_loop) becomes unavailable, unsloth_compile_transformers still reaches the Trainer patch and turns the same failure into RuntimeError("Unsloth: Unsuccessfully patched inner_training_loop") in unsloth_zoo/compiler.py:4173-4179, so the drift detector would go green while the compile path still crashes. Please keep the previous DRIFT failure for residual exceptions, or guard this skip to the specific 5.x wrapper case.
Useful? React with 👍 / 👎.
| if _has_var_keyword(upstream_fwd): | ||
| pytest.skip( |
There was a problem hiding this comment.
This skips whenever the upstream method has any **kwargs, even if it still has an inspectable signature with named parameters. If a future Pixtral forward is forward(self, hidden_states, attention_mask=None, **kwargs) or otherwise drops position_embeddings while retaining a catch-all, the test will skip instead of reporting that temporary_patches/pixtral.py still unpacks position_embeddings; the stated 5.x issue is specifically the generic (self, *args, **kwargs) wrapper, so the skip should check that exact signature (or version) before bypassing _assert_params_superset.
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 exhaustive test suite to accommodate changes in Transformers 5.x, specifically handling cases where source code inspection or static signature verification is no longer feasible due to method wrapping. Feedback suggests that the transition from _drift to pytest.skip in the compiler rewriter tests may inadvertently reduce test strictness for Transformers 4.x by masking cases where optimizations are silently skipped. Additionally, it is recommended to gate the signature check skip for PixtralAttention.forward specifically to Transformers 5.x to avoid losing coverage on 4.x builds that might use variable keywords while still maintaining explicit parameters.
| pytest.skip( | ||
| "Trainer._inner_training_loop source unavailable on this " | ||
| "transformers build (likely wrapped/decorated); the rewriter " | ||
| "silently no-ops, which is acceptable on this matrix cell." | ||
| ) |
There was a problem hiding this comment.
The change from _drift (which fails the test) to pytest.skip in the except block reduces the strictness of the detector for transformers 4.x. Since transformers 5.x is already explicitly handled by the _skip_if_transformers_5x call at line 706, this except block is primarily reached on 4.x builds. If inspect.getsource fails on 4.x, it indicates that the unsloth rewriter is no-oping on a version it is intended to optimize, which should ideally be reported as a drift/failure to ensure the optimization remains active and detectable in CI.
| # via attention-implementation dispatch. The zoo patch passes through | ||
| # kwargs so its call site stays valid; static param names are not | ||
| # introspectable, so skip rather than report spurious drift. | ||
| if _has_var_keyword(upstream_fwd): |
There was a problem hiding this comment.
Using _has_var_keyword as the sole condition to skip the signature check is too broad. Many methods in transformers include **kwargs while still maintaining explicitly named parameters. If PixtralAttention.forward were to add **kwargs in a future 4.x release while keeping its named arguments, this test would spuriously skip and lose coverage. It is safer to gate this skip by _TX_IS_5X to specifically target the wrapping behavior introduced in 5.x, or to check if the required parameters are actually missing from the signature.
| if _has_var_keyword(upstream_fwd): | |
| if _TX_IS_5X and _has_var_keyword(upstream_fwd): |
…**kwargs) (unslothai#651) Generalises the Pixtral-specific tolerance from unslothai#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.
1 participant
Summary
Two upstream-drift detectors started reporting DRIFT on transformers >= 5.0 even though zoo's behaviour at the pinned patch sites is already correct on those versions. Both fixes preserve the detector's strict behaviour on transformers 4.x.
`tests/test_compiler_rewriter_exhaustive.py::test_compiler_logger_running_training_inner_loop_present`
`tests/test_temporary_patches_exhaustive.py::test_pixtral_attention_forward_signature`
Surfacing these on the unsloth side as Core matrix failures: PRs 5427 / 5430 / 5432 / 5434 across HF=4.57.6 / HF=default / HF=latest.
Test plan