tests: drift detectors cover transformers 5.x (mirror unsloth PR #5423)#642
Conversation
The two transformers-targeting drift detectors in tests/test_upstream_
import_fixes_drift.py had predicates written against transformers 4.x
only; on the Core matrix's HF=latest cell (transformers>=5,<6) they
fire DRIFT DETECTED even when the upstream / fix combination is healthy.
* test_pretrained_model_enable_input_require_grads_uses_old_pattern
fired whenever the source contains "for module in self.modules()".
But the unsloth replacement that patch_enable_input_require_grads
installs deliberately ALSO uses that pattern, just wrapped in
try / except NotImplementedError. The predicate could not
distinguish broken upstream from the working patch. Accept either
pre-HF#41993 shape (no self.modules() loop) or the post-patch shape
(loop + NotImplementedError handler).
* test_transformers_torchcodec_available_flag_is_present asserted the
pre-5.x module-level _torchcodec_available flag. transformers 5
replaced it with an lru_cache'd is_torchcodec_available() callable.
Accept either symbol -- the patch in unsloth/import_fixes.py now
targets both (PR #5423 on unslothai/unsloth).
Local verification:
* transformers 4.57.6 + trl 0.25.1 + peft 0.19.1 + triton 3.5.1 +
vllm 0.15.1+cu130 (the Core HF=4.57.6 cell shape): 18 passed.
* transformers 5.8.1 + peft 0.19.1 + torch 2.9 CPU (no trl / vllm /
datasets / xformers installed): 12 passed, 6 skipped on missing
optional libs, 0 failed.
The corresponding import_fixes.py changes already shipped on
unslothai/unsloth#5423. Zoo only carries the detectors so this PR is
test-only.
There was a problem hiding this comment.
Code Review
This pull request updates drift detection tests in tests/test_upstream_import_fixes_drift.py to better handle changes in the transformers library. Specifically, it refines the logic for detecting patches in enable_input_require_grads and updates the torchcodec availability check to support both legacy flags and newer public functions. The reviewer suggested making the string matching for the NotImplementedError patch more specific to avoid potential false positives from comments or docstrings.
| if "for module in self.modules()" not in src: | ||
| return # healthy: pre-HF#41993 shape | ||
| if "NotImplementedError" in src: | ||
| return # healthy: unsloth's tolerant replacement is installed |
There was a problem hiding this comment.
The check for "NotImplementedError" in the source code is a bit broad and could potentially be triggered by comments or docstrings in the original function. Using a more specific string like "except NotImplementedError" would make the detection of the applied patch more robust while remaining consistent with the project's use of string-matching for drift detection.
| if "for module in self.modules()" not in src: | |
| return # healthy: pre-HF#41993 shape | |
| if "NotImplementedError" in src: | |
| return # healthy: unsloth's tolerant replacement is installed | |
| if "for module in self.modules()" not in src: | |
| return # healthy: pre-HF#41993 shape | |
| if "except NotImplementedError" in src: | |
| return # healthy: unsloth's tolerant replacement is installed |
References
- It is acceptable to use fragile string-matching for code patching if it is consistent with the existing codebase's architecture. This suggestion refines the string match to be more specific without changing the underlying architecture.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bb0665005
ℹ️ 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".
| has_func = callable(getattr(tf_iu, "is_torchcodec_available", None)) | ||
| assert has_flag or has_func, ( |
There was a problem hiding this comment.
Check the torchcodec backend mapping too
On transformers 5.x, accepting the presence of is_torchcodec_available alone leaves this drift detector green even when disable_torchcodec_if_broken cannot actually disable torchcodec for requires_backends(..., "torchcodec"): transformers.utils.import_utils.BACKENDS_MAPPING["torchcodec"] is built at import time with the original checker function, so rebinding only is_torchcodec_available leaves that path reporting a broken torchcodec install as available. Please make this detector cover the cached backend mapping entry as part of the 5.x patch contract, otherwise the regression this test is meant to guard can still slip through.
Useful? React with 👍 / 👎.
Summary
The two transformers-targeting drift detectors in
tests/test_upstream_import_fixes_drift.pyhave predicates written against transformers 4.x only. On the Core matrix'sHF=latest + TRL=latestcell (transformers>=5,<6) they fire DRIFT DETECTED even when the upstream / unsloth fix combination is healthy.Companion PR on unsloth: unslothai/unsloth#5423 (merged) shipped the matching
disable_torchcodec_if_brokenchange and the same two predicate relaxations on its side. This PR mirrors just the test changes onto zoo (zoo no longer carries the import_fixes source, only the drift detectors, since PR #639).What changes
1.
test_pretrained_model_enable_input_require_grads_uses_old_patternFired whenever the source contains
for module in self.modules(). But the unsloth replacement thatpatch_enable_input_require_gradsinstalls deliberately ALSO uses that pattern, just wrapped intry / except NotImplementedError. The predicate could not distinguish broken upstream from the working patch. Accept either:self.modules()loop in the source), orNotImplementedErrorhandler in the source).2.
test_transformers_torchcodec_available_flag_is_presentAsserted the pre-5.x module-level
_torchcodec_availableflag. transformers 5 replaced it with anlru_cache'dis_torchcodec_available()callable. Accept either symbol; the patch site inunsloth/import_fixes.pynow targets both (PR #5423).Local verification
Test plan
pytest tests/test_upstream_import_fixes_drift.pyis 18/18 on the HF=4.57.6 cell shape.