tests: import_fixes drift detectors (HARD GATE on Core matrix)#5414
Conversation
Ports zoo PR #637's drift-detector pattern to unsloth as a new test file + Core matrix step. Background unsloth/import_fixes.py is a 1932-line catalog of hand-rolled patches for upstream regressions: protobuf MessageFactory drift, datasets 4.4.x recursion, TRL tuple-vs-bool _*_available caching, transformers PreTrainedModel.enable_input_require_grads source pattern flip, triton CompiledKernel num_ctas missing, peft weight-converter ctor compat, torch/torchvision pairing, vllm guided_decoding params, etc. Today each fix runs unconditionally at unsloth import; that's defensively correct but it means: a fix becoming a no-op (upstream silently fixed itself) is invisible. a fix becoming needed-but-broken (upstream drifted in a new way the workaround doesn't match) only surfaces as a downstream crash. tests/test_import_fixes_drift.py (18 tests) One drift detector per fix_* / patch_* function in import_fixes.py. Each test asserts the HEALTHY upstream shape absent the regression. When the pathology is currently ACTIVE, fires pytest.fail("DRIFT DETECTED: <fix function> needed because <observation>") -- NEVER pytest.skip. CI must go RED so the maintainer triages on the next PR. First run on the current install surfaces 3 active drifts: peft.utils.transformers_weight_conversion unimportable (transformers.conversion_mapping missing) -- patch_peft_ weight_converter_compatibility will silently no-op. triton 3.5.1 CompiledKernel lacks num_ctas + cluster_dims -- fix_triton_compiled_kernel_missing_attrs is live-needed. vllm exposes only StructuredOutputsParams, not GuidedDecodingParams -- fix_vllm_guided_decoding_params is live-needed. CI wiring (.github/workflows/consolidated-tests-ci.yml) New step `import_fixes drift detectors (18 tests, HARD GATE)` added to the Core matrix BEFORE the Bucket-A tests, so the matrix cell fails fast on a real upstream regression. No continue-on-error: a drift detection MUST go red. This mirrors the same change just landed on unslothai/unsloth-zoo#637 (commit ff5a3d8). Same fail-loud-on-drift semantic; same set of fix functions covered; same 1:1 mapping between test + import_fixes.py source-of-truth function.
for more information, see https://pre-commit.ci
![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 introduces a comprehensive suite of drift detectors in tests/test_import_fixes_drift.py to monitor upstream library regressions that Unsloth currently patches. The feedback identifies several critical issues where the tests as written will cause permanent CI failures even when Unsloth's workarounds are successfully active. Specifically, several tests rely on version checks or string matching that don't account for the fact that the code has been monkey-patched or rewritten by Unsloth. Additionally, there is a recommendation to avoid broad exception handling in the TRL accessor check to prevent masking actual bugs.
| if not isinstance(result, bool): | ||
| bad[name] = (type(result).__name__, result) |
There was a problem hiding this comment.
The broad except Exception: continue block silently ignores any accessor that raises an error. While this is intended to skip functions with required arguments, it also masks potential bugs or new types of drift where an accessor might crash (e.g., due to a broken internal dependency or a regression in trl itself). It is safer to catch specific expected exceptions (like TypeError from signature mismatch) or log the exception to aid in debugging, as per the general rules.
References
- When handling exceptions, avoid broad
except Exception: passclauses. Instead, catch specific exceptions and log them (at least at a debug level) to aid in troubleshooting.
|
|
||
| if "for module in self.modules()" in src: | ||
| pytest.fail( | ||
| "DRIFT DETECTED: PreTrainedModel.enable_input_require_grads now " |
There was a problem hiding this comment.
This test will fail even when the patch_enable_input_require_grads workaround is successfully applied. The workaround (defined in unsloth/import_fixes.py at line 636) uses the for module in self.modules() pattern but wraps the call in a try...except NotImplementedError block. Since this test checks for the presence of that string in the source of the patched method, it will trigger a pytest.fail even if the pathology has been safely mitigated. Consider updating the check to distinguish between the problematic upstream implementation and the safe Unsloth replacement (e.g., by checking for the presence of the NotImplementedError handler).
|
|
||
| pytest.fail( |
There was a problem hiding this comment.
This check targets the CompiledKernel class, but the corresponding fix in import_fixes.py (lines 957-964) only injects the missing attributes into instances via a patched __init__. Consequently, on Triton 3.6.0+, this test will always fail because the class itself still lacks the attributes, even if the workaround is active and effective for instances. To accurately detect drift while allowing the workaround to pass, consider verifying an instance or checking if __init__ has been monkey-patched by Unsloth.
| "fix_vllm_aimv2_issue rewrites ovis.py to skip the duplicate " | ||
| 'AutoConfig.register("aimv2", ...) call.' | ||
| ) | ||
|
|
There was a problem hiding this comment.
This test performs a strict version check (vllm_v < cutoff) and fails if the version is old. However, unsloth/import_fixes.py provides a workaround for these older versions by rewriting the ovis.py config file. As a result, this test will cause a permanent CI failure for environments using these supported (but patched) vLLM versions. To function as a true drift detector that allows the matrix to return to green when a workaround is present, the test should check for the actual presence of the problematic code in ovis.py rather than just the package version. The same applies to test_xformers_is_post_num_splits_key_fix_or_not_installed at line 654.
![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: 18ebd816a1
ℹ️ 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".
| if hasattr(ck_cls, "num_ctas"): | ||
| return # healthy: old-style triton with direct attr |
There was a problem hiding this comment.
For Triton builds that restore CompiledKernel.num_ctas but still lack cluster_dims, this early return marks the stack healthy even though the workaround and the docstring both require cluster_dims as well. Because fix_triton_compiled_kernel_missing_attrs also no-ops when num_ctas exists, this detector would miss exactly the partial upstream drift that can still leave torch.compile launcher paths crashing; verify both attributes before returning.
Useful? React with 👍 / 👎.
| expected_params = {"weight_conversions", "adapter_name"} | ||
| actual_params = set(sig.parameters) |
There was a problem hiding this comment.
If PEFT removes or renames the optional peft_config parameter while leaving weight_conversions and adapter_name, this test still passes, but patch_peft_weight_converter_compatibility always calls the original build_peft_weight_mapping(weight_conversions, adapter_name, peft_config), so the wrapper would start raising TypeError at runtime. Include peft_config in the expected signature so the detector catches that drift.
Useful? React with 👍 / 👎.
Strictly comment / docstring trims. AST-verified comment-only. * Module header: 36 lines -> 7 lines. * Per-test docstring: collapse each 7-15 line prose block to a 1-3 line lead naming the import_fixes.py function + line range plus the one-sentence why; pytest.fail messages stay verbatim so a red CI cell still names the upstream regression. * Helper docstrings (_safe_version, _is_custom_torch_build): drop. * Inline narrative comments inside test bodies: drop. * Section dividers and licence header: untouched. Net: 700 -> 537 lines, zero behaviour changes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c38b8f709
ℹ️ 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".
| # HARD GATE: a red cell here is a real upstream regression | ||
| # without a corresponding zoo / unsloth-side workaround. | ||
| run: | | ||
| python -m pytest -v --tb=short tests/test_import_fixes_drift.py |
There was a problem hiding this comment.
In the Core job this command runs in an environment whose install step adds torch/transformers/trl/peft/etc. but not vllm or xformers (I checked the workflow install block and repo-wide rg only finds those names in comments). The new vLLM detectors call pytest.importorskip("vllm"), and the xformers detector skips when find_spec("xformers") is None, so every matrix cell silently skips the advertised vLLM/aimv2 and xformers drift coverage while this step is labeled a hard gate. Install those optional packages for this step or remove/split those detectors so the gate can actually catch those regressions.
Useful? React with 👍 / 👎.
…thai#5414) * tests: import_fixes drift detectors (HARD GATE on Core matrix) Ports zoo PR unslothai#637's drift-detector pattern to unsloth as a new test file + Core matrix step. Background unsloth/import_fixes.py is a 1932-line catalog of hand-rolled patches for upstream regressions: protobuf MessageFactory drift, datasets 4.4.x recursion, TRL tuple-vs-bool _*_available caching, transformers PreTrainedModel.enable_input_require_grads source pattern flip, triton CompiledKernel num_ctas missing, peft weight-converter ctor compat, torch/torchvision pairing, vllm guided_decoding params, etc. Today each fix runs unconditionally at unsloth import; that's defensively correct but it means: a fix becoming a no-op (upstream silently fixed itself) is invisible. a fix becoming needed-but-broken (upstream drifted in a new way the workaround doesn't match) only surfaces as a downstream crash. tests/test_import_fixes_drift.py (18 tests) One drift detector per fix_* / patch_* function in import_fixes.py. Each test asserts the HEALTHY upstream shape absent the regression. When the pathology is currently ACTIVE, fires pytest.fail("DRIFT DETECTED: <fix function> needed because <observation>") -- NEVER pytest.skip. CI must go RED so the maintainer triages on the next PR. First run on the current install surfaces 3 active drifts: peft.utils.transformers_weight_conversion unimportable (transformers.conversion_mapping missing) -- patch_peft_ weight_converter_compatibility will silently no-op. triton 3.5.1 CompiledKernel lacks num_ctas + cluster_dims -- fix_triton_compiled_kernel_missing_attrs is live-needed. vllm exposes only StructuredOutputsParams, not GuidedDecodingParams -- fix_vllm_guided_decoding_params is live-needed. CI wiring (.github/workflows/consolidated-tests-ci.yml) New step `import_fixes drift detectors (18 tests, HARD GATE)` added to the Core matrix BEFORE the Bucket-A tests, so the matrix cell fails fast on a real upstream regression. No continue-on-error: a drift detection MUST go red. This mirrors the same change just landed on unslothai/unsloth-zoo#637 (commit ff5a3d8). Same fail-loud-on-drift semantic; same set of fix functions covered; same 1:1 mapping between test + import_fixes.py source-of-truth function. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * chore: trim verbose docstrings in import_fixes drift detectors Strictly comment / docstring trims. AST-verified comment-only. * Module header: 36 lines -> 7 lines. * Per-test docstring: collapse each 7-15 line prose block to a 1-3 line lead naming the import_fixes.py function + line range plus the one-sentence why; pytest.fail messages stay verbatim so a red CI cell still names the upstream regression. * Helper docstrings (_safe_version, _is_custom_torch_build): drop. * Inline narrative comments inside test bodies: drop. * Section dividers and licence header: untouched. Net: 700 -> 537 lines, zero behaviour changes. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
…ai#5423) PR unslothai#5414's drift detectors and the corresponding import_fixes helpers were written against transformers 4.x. The Repo tests (CPU) step (which installs transformers>=4.51,<5.5 and currently resolves to 5.4.0) surfaces three real predicate gaps: * test_pretrained_model_enable_input_require_grads_uses_old_pattern fires DRIFT DETECTED whenever the source contains "for module in self.modules()". But the unsloth replacement that patch_enable_input_require_grads installs ALSO uses that pattern -- deliberately, just wrapped in try / except NotImplementedError. So the predicate cannot 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 asserts 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. Also update disable_torchcodec_if_broken to actually disable on 5.x: clear the cache and rebind the function to return False. 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 (the Repo tests (CPU) shape, drop trl / vllm / datasets / xformers): 12 passed, 6 skipped on missing optional libs, 0 failed. PR unslothai#5376's Repo tests (CPU) failure was a triple: - triton + enable_input_require_grads: fixed by merging current main (PR unslothai#5421's relaxed triton predicate + conftest 'import unsloth'). - torchcodec: fixed by THIS PR.
…xes_drift.py) (unslothai#5428) * tests: ship public-api surface drift detector + wire into Core matrix Companion to tests/test_import_fixes_drift.py (PR unslothai#5414): that file catches drift in THIRD-PARTY libs (transformers / trl / triton / peft / vllm / torchcodec / xformers); this file catches drift in unsloth's OWN public-surface API -- the top-9 classmethods + symbols that unslothai/notebooks calls at ~2000 cumulative sites. Closes the gap where a refactor on this repo (e.g. renaming FastLanguageModel.from_pretrained -> .load) would pass unsloth CI green and surface only on the next unslothai/notebooks CI run, or worse, on a user's Colab crash report. Coverage (call-site counts measured against unslothai/notebooks main): test_fast_language_model_class_present test_fast_language_model_from_pretrained_kwargs 506 sites test_fast_language_model_get_peft_model_kwargs 304 sites test_fast_language_model_for_inference_callable 370 sites test_fast_vision_model_class_and_methods (4 methods) test_fast_vision_model_get_peft_model_vision_kwargs (4 kwargs) test_fast_model_class_and_methods (2 methods) test_fast_model_from_pretrained_kwargs 103 sites test_is_bf16_supported_or_alias_callable 48 + 8 sites Each test asserts the healthy public shape via inspect.signature; on regression fires pytest.fail("DRIFT DETECTED: ...") -- never pytest.skip -- so the Core matrix cell goes red. Mirrors the same skeleton used by tests/test_import_fixes_drift.py. Wired as a new step in consolidated-tests-ci.yml right after the import_fixes drift step, inside every Core matrix cell. Local verification on transformers 4.57.6 + unsloth main: pytest tests/test_public_api_surface.py -v -> 9 passed in 0.02s * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 participant
Summary
Adds
tests/test_import_fixes_drift.py-- 18 drift detectors that fail loudly when an upstream pathologyunsloth/import_fixes.pyworks around becomes active without a corresponding patch landing. Wires it into the Core matrix as a HARD GATE (nocontinue-on-error).Mirrors the same pattern just landed on
unslothai/unsloth-zoo#637(commitff5a3d8).Why
import_fixes.pyis a 1932-line catalog of patches for upstream regressions. Today the fixes run unconditionally at unsloth import; this is defensively correct but it means:The new suite is the drift detector: one test per
fix_*/patch_*function, asserting the healthy upstream shape absent the regression. When the pathology is active,pytest.fail("DRIFT DETECTED: ...")-- neverpytest.skip-- so the matrix cell goes RED and the maintainer triages on this PR, not in production.What's covered (18 tests)
import_fixes.pyfunctionfix_message_factory_issuepatch_datasets_*_available(x2)fix_trl_vllm_ascendPreTrainedModel.enable_input_require_gradssource pattern (x2)patch_enable_input_require_gradsdisable_torchcodec_if_broken_disable_transformers_causal_conv1dis_wandb_available(x2)disable_broken_wandbtransformers_weight_conversionimportpatch_peft_weight_converter_compatibilityCompiledKernel.num_ctas/cluster_dimsfix_triton_compiled_kernel_missing_attrstorchvision_compatibility_checkGuidedDecodingParams/StructuredOutputsParamsfix_vllm_guided_decoding_paramsfix_vllm_aimv2_issuehuggingface_hubis_offline_mode/HF_HUB_OFFLINEfix_huggingface_hubtorch.nn.init.trunc_normal_presencepatch_trunc_normal_precision_issuefix_xformers_performance_issueTest plan
pytest tests/test_import_fixes_drift.py -vshows 15 passed + 3 failed on a current install (the 3 failures are real active drifts).import_fixes drift detectors (18 tests, HARD GATE)Core matrix step runs in each of the 3 cells (HF=4.57.6 + TRL<1,HF=default + TRL=default,HF=latest + TRL=latest).