remove unsloth_zoo/import_fixes.py: redundant with unsloth's#639
Conversation
…l patch unsloth/import_fixes.py::fix_triton_compiled_kernel_missing_attrs wraps CompiledKernel.__init__ to inject num_ctas / cluster_dims per-instance. The previous drift detector only accepted a class-level num_ctas (the shape zoo's now-removed import_fixes used to install). The class-level attr is unnecessary for the upstream pathology -- torch Inductor's make_launcher only ever sees CompiledKernel instances, and unsloth's __init__ wrapper covers them. Accept either shape so the detector still surfaces real upstream drift without false-firing on the instance-level fix shape.
There was a problem hiding this comment.
Code Review
This pull request removes the redundant import_fixes.py module from unsloth_zoo, delegating the application of upstream drift fixes to the main unsloth package. The test suite and initialization logic are updated to reflect this change. A review comment correctly identified a potential AttributeError in the drift detection test when inspecting the __code__ attribute of a class's __init__ method without a null check, providing a fix to ensure graceful failure.
| if init is not None: | ||
| code = getattr(init, "__code__", None) | ||
| freevars = set(getattr(code, "co_freevars", ()) or ()) | ||
| co_names = set(getattr(code, "co_names", ()) or ()) | ||
| if "_orig_init" in freevars or {"num_ctas", "cluster_dims"}.issubset( | ||
| co_names | ||
| ): | ||
| return # healthy: unsloth's __init__ wrapper is installed |
There was a problem hiding this comment.
The code currently accesses attributes on code without verifying that it is not None. If ck_cls.__init__ is a built-in function or a wrapper that does not expose __code__ (such as a slot wrapper like object.__init__), getattr(init, "__code__", None) will return None. This would cause an AttributeError on the subsequent lines when calling getattr(code, ...). Adding a guard for code ensures the test fails gracefully with the intended drift message instead of crashing with an unhandled exception during pytest collection or execution.
| if init is not None: | |
| code = getattr(init, "__code__", None) | |
| freevars = set(getattr(code, "co_freevars", ()) or ()) | |
| co_names = set(getattr(code, "co_names", ()) or ()) | |
| if "_orig_init" in freevars or {"num_ctas", "cluster_dims"}.issubset( | |
| co_names | |
| ): | |
| return # healthy: unsloth's __init__ wrapper is installed | |
| if init is not None: | |
| code = getattr(init, "__code__", None) | |
| if code is not None: | |
| freevars = set(getattr(code, "co_freevars", ()) or ()) | |
| co_names = set(getattr(code, "co_names", ()) or ()) | |
| if "_orig_init" in freevars or {"num_ctas", "cluster_dims"}.issubset( | |
| co_names | |
| ): | |
| return # healthy: unsloth's __init__ wrapper is installed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90ba8b1167
ℹ️ 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".
| # in ``unsloth/import_fixes.py`` and are applied at ``import unsloth`` time. | ||
| # ``unsloth_zoo`` cannot be imported standalone -- the GPU init path below | ||
| # requires ``find_spec("unsloth")`` to succeed -- so ``import unsloth`` has | ||
| # always already run by the time we get here, and the patches are in | ||
| # place. We therefore do NOT re-host or re-apply them at zoo import time. |
There was a problem hiding this comment.
Import unsloth before relying on its fixes
This assumes unsloth/import_fixes.py has already executed, but find_spec("unsloth") later in this file only proves the package is installed; it does not import it. A downstream user can still import unsloth_zoo directly before importing unsloth (and pyproject.toml also does not pin a minimum unsloth version containing the newly ported PEFT fix), so the deleted local apply_import_fixes() no longer installs the transformers conversion stubs or the triton/vLLM patches. On supported stacks such as peft 0.19.x with transformers 4.x, the next from peft.utils import transformers_weight_conversion can again raise ModuleNotFoundError instead of being repaired at zoo import time.
Useful? React with 👍 / 👎.
Pull unslothai/unsloth@main rather than PyPI so the matrix cells exercise the latest patch chain (specifically PR #5416's stub-module injection for peft.utils.transformers_weight_conversion, which the drift detector needs). Reverts back to PyPI once the next unsloth release ships.
Summary
PR #637 introduced
unsloth_zoo/import_fixes.pycontaining sevenfix_*/patch_*workarounds for upstream pathologies on triton / vllm / trl / peft / transformers / datasets / torchcodec. Six of those seven functions are byte-for-byte (or near-byte-for-byte) mirrors of identically-named functions already living onunsloth/import_fixes.pyand run there atimport unslothtime. The seventh,fix_peft_transformers_weight_conversion_import, is the only novel one; it is being ported tounsloth/import_fixes.pyvia the open PR unslothai/unsloth#5416.Hosting the same patches on both packages adds drift surface without any operational benefit:
unsloth_zoo/__init__.pyalready raisesImportErrorunlessfind_spec("unsloth")succeeds (the_SKIP_GPU_INITbranch), and at runtimeimport unslothalways runs before anything touches zoo. By the time we reach zoo init the fixes are already applied; running them a second time is a no-op when it works and a footgun (mirror divergence) when it doesn't.Changes
unsloth_zoo/import_fixes.py(973 lines).unsloth_zoo/__init__.py: drop thefrom .import_fixes import apply_import_fixesblock. Replace with a comment explaining why zoo no longer hosts these patches (and where they now live).tests/conftest.pysection 3: replace the file-path loader forunsloth_zoo.import_fixeswith a guardedimport unslothso the CPU-free test harness still triggersunsloth/import_fixes.pyand the drift detectors see the patched state. ImportError is swallowed so security-only test runs (no unsloth installed) keep passing.tests/test_upstream_import_fixes_drift.py::test_triton_compiled_kernel_has_num_ctas_and_cluster_dims: relax the detector to accept either a class-levelnum_ctas(the zoo-side shape, no longer present) or an instance-level__init__wrapper (unsloth's actual shape). The class-level attr was a stricter zoo-only stamping; the instance-level wrapper is what torch Inductor'smake_launcheractually needs, and is whatunsloth/import_fixes.py::fix_triton_compiled_kernel_missing_attrsinstalls.Compatibility
fix_peft_transformers_weight_conversion_importonce import_fixes: stub transformers.conversion_mapping so peft 0.19.x imports on transformers 4.x unsloth#5416 lands.fix_trl_vllm_ascendstill coerces tuple-cached_*_availableflags via unsloth's copy.Drift Detection
All drift detectors in
tests/test_upstream_import_fixes_drift.pycontinue to fire on real upstream regressions. The tests already citeunsloth/import_fixes.pyline numbers in their docstrings, so the rewire is consistent with their stated source of truth.Test Plan
pytest tests/test_upstream_import_fixes_drift.py tests/test_upstream_pinned_symbols_*.py tests/test_zoo_history_regressions_deep.py tests/test_zoo_source_upstream_refs.py tests/security -q->226 passed, 5 skipped(matches baseline before the rewire).python -c "import unsloth_zoo"still imports cleanly with the patches in place (verifiedtransformers.conversion_mappingandtransformers.core_model_loadingstubs are installed insys.modulesafter the import chain runs).Related
fix_peft_transformers_weight_conversion_importtounsloth/import_fixes.py. Once that merges, the symmetric removal here is purely subtractive: zoo loses redundant copies, the patches still apply via the always-imports-first unsloth side.