fix(registry): don't register deepseek models at import time - #7227
danielhanchen merged 4 commits into
Conversation
`_deepseek.py` called `register_deepseek_models(include_original_model=True)` at module scope, so merely importing `unsloth.registry` registered models (and reached the hub via `list_models`) as a side effect. None of the other five families (`_gemma`/`_llama`/`_mistral`/`_phi`/`_qwen`) do this; they only register when `register_models()` asks them to. Two consequences: - Importing the registry populated MODEL_REGISTRY on its own (32 entries, including 10 `deepseek-ai` original models that no other family leaks) and did network I/O at import time. - Because the import-time call set the `_IS_DEEPSEEK_*_REGISTERED` guards with `include_original_model=True`, the later `register_models()` call (which uses the default `include_original_model=False`) early-returned, so the original-model set won permanently. Remove the stray module-level call. The `if __name__ == "__main__"` block below still registers with `include_original_model=True` for standalone use, so the generator script is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 removes the module-level call to register_deepseek_models in unsloth/registry/_deepseek.py to prevent importing the registry from registering models as a side effect. It also adds a test in tests/test_model_registry.py to verify that importing the registry does not populate MODEL_REGISTRY. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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: f021531d75
ℹ️ 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".
| sys.executable, | ||
| "-c", | ||
| "import unsloth.registry\n" | ||
| "from unsloth.registry.registry import MODEL_REGISTRY\n" | ||
| "print('REGISTRY_SIZE', len(MODEL_REGISTRY))", |
There was a problem hiding this comment.
In the CPU-only repo test workflow, this starts a brand-new interpreter with only python -c, so it does not inherit tests/conftest.py's sys.modules stubs for unsloth_zoo.device_type / unsloth.device_type that are installed on no-accelerator runners. In that context import unsloth.registry goes through unsloth/__init__.py -> _gpu_init and can fail before REGISTRY_SIZE is printed, causing this newly added test (which is not deselected by the CI workflow) to fail even though the registry side effect is fixed; run the check in-process with isolated modules or bootstrap the same test stubs in the child.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, and fixed in eeecd7c: the subprocess child now imports this directory's conftest first, so it inherits the same GPU-free harness the pytest session uses (device_type stubs plus the torch.cuda probe patches). Verified on a real CPU-only torch build (torch==2.10.0+cpu, cuda unavailable): the old child raised NotImplementedError from unsloth_zoo.device_type before printing and errored the test, while the harnessed child prints REGISTRY_SIZE 0 and passes. Also switched to check=False and fold the child stdout/stderr into the assertion so a future import regression is legible instead of an opaque CalledProcessError.
The new test spawned a fresh `python -c "import unsloth.registry"` that did not inherit tests/conftest.py's GPU-free harness, so on no-accelerator CI runners the child raised NotImplementedError from unsloth_zoo.device_type before printing REGISTRY_SIZE. With check=True this surfaced only as an opaque CalledProcessError, turning the "Repo tests (CPU)" job red even though the registry fix is correct. Import this directory's conftest inside the child first so it applies the same device_type stubs and torch.cuda probe patches. Also use check=False and include the child stdout/stderr in the assertion message so a future import regression is legible instead of an opaque non-zero exit.
Adds a fresh-interpreter test that register_models() registers only unsloth-org models (deepseek still present via the normal path) and never leaks the upstream deepseek-ai originals that the import-time guard poisoning used to leak (129 -> 139). Factors the conftest-harness subprocess runner into a shared helper reused by both registry import tests.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Thanks! |
* fix(registry): don't register deepseek models at import time `_deepseek.py` called `register_deepseek_models(include_original_model=True)` at module scope, so merely importing `unsloth.registry` registered models (and reached the hub via `list_models`) as a side effect. None of the other five families (`_gemma`/`_llama`/`_mistral`/`_phi`/`_qwen`) do this; they only register when `register_models()` asks them to. Two consequences: - Importing the registry populated MODEL_REGISTRY on its own (32 entries, including 10 `deepseek-ai` original models that no other family leaks) and did network I/O at import time. - Because the import-time call set the `_IS_DEEPSEEK_*_REGISTERED` guards with `include_original_model=True`, the later `register_models()` call (which uses the default `include_original_model=False`) early-returned, so the original-model set won permanently. Remove the stray module-level call. The `if __name__ == "__main__"` block below still registers with `include_original_model=True` for standalone use, so the generator script is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * test(registry): make import-side-effect test pass on CPU-only runners The new test spawned a fresh `python -c "import unsloth.registry"` that did not inherit tests/conftest.py's GPU-free harness, so on no-accelerator CI runners the child raised NotImplementedError from unsloth_zoo.device_type before printing REGISTRY_SIZE. With check=True this surfaced only as an opaque CalledProcessError, turning the "Repo tests (CPU)" job red even though the registry fix is correct. Import this directory's conftest inside the child first so it applies the same device_type stubs and torch.cuda probe patches. Also use check=False and include the child stdout/stderr in the assertion message so a future import regression is legible instead of an opaque non-zero exit. * test(registry): assert register_models() leaks no upstream originals Adds a fresh-interpreter test that register_models() registers only unsloth-org models (deepseek still present via the normal path) and never leaks the upstream deepseek-ai originals that the import-time guard poisoning used to leak (129 -> 139). Factors the conftest-harness subprocess runner into a shared helper reused by both registry import tests. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <unslothai@gmail.com>
…ai#7227) * fix(registry): don't register deepseek models at import time `_deepseek.py` called `register_deepseek_models(include_original_model=True)` at module scope, so merely importing `unsloth.registry` registered models (and reached the hub via `list_models`) as a side effect. None of the other five families (`_gemma`/`_llama`/`_mistral`/`_phi`/`_qwen`) do this; they only register when `register_models()` asks them to. Two consequences: - Importing the registry populated MODEL_REGISTRY on its own (32 entries, including 10 `deepseek-ai` original models that no other family leaks) and did network I/O at import time. - Because the import-time call set the `_IS_DEEPSEEK_*_REGISTERED` guards with `include_original_model=True`, the later `register_models()` call (which uses the default `include_original_model=False`) early-returned, so the original-model set won permanently. Remove the stray module-level call. The `if __name__ == "__main__"` block below still registers with `include_original_model=True` for standalone use, so the generator script is unaffected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * test(registry): make import-side-effect test pass on CPU-only runners The new test spawned a fresh `python -c "import unsloth.registry"` that did not inherit tests/conftest.py's GPU-free harness, so on no-accelerator CI runners the child raised NotImplementedError from unsloth_zoo.device_type before printing REGISTRY_SIZE. With check=True this surfaced only as an opaque CalledProcessError, turning the "Repo tests (CPU)" job red even though the registry fix is correct. Import this directory's conftest inside the child first so it applies the same device_type stubs and torch.cuda probe patches. Also use check=False and include the child stdout/stderr in the assertion message so a future import regression is legible instead of an opaque non-zero exit. * test(registry): assert register_models() leaks no upstream originals Adds a fresh-interpreter test that register_models() registers only unsloth-org models (deepseek still present via the normal path) and never leaks the upstream deepseek-ai originals that the import-time guard poisoning used to leak (129 -> 139). Factors the conftest-harness subprocess runner into a shared helper reused by both registry import tests. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Daniel Han <unslothai@gmail.com>
3 participants
Problem
unsloth/registry/_deepseek.pycallsregister_deepseek_models(include_original_model=True)at module scope (line 174). None of the other five families (_gemma/_llama/_mistral/_phi/_qwen) have a module-level register call — they register only whenregister_models()asks them to.That one stray line has two effects:
unsloth.registryhas side effects. Merelyimport unsloth.registrypopulatesMODEL_REGISTRY(32 entries, including 10deepseek-aioriginal models that no other family leaks) before any explicitregister_models()call.register_models()can't register deepseek the normal way. The import-time call sets the_IS_DEEPSEEK_*_REGISTEREDguards withinclude_original_model=True, so the laterregister_models()call — which uses the defaultinclude_original_model=False— early-returns. The import-time result wins permanently, andtest_model_registration[deepseek]passes vacuously (guard already set → registers nothing → empty set trivially "not missing").Fix
Remove the stray module-level call. The
if __name__ == "__main__"block below already registers withinclude_original_model=Truefor standalone script use, so that path is unaffected.Before / after,
import unsloth.registrywith no explicit call:After the fix,
register_models()still registers all 129unsloth-org models and no longer leaks thedeepseek-aioriginals — deepseek is now consistent with the other families.Test
Two subprocess tests, each run in a fresh interpreter so it is independent of other tests'
register_models()calls on the shared registry (both are network-free):test_importing_registry_does_not_register_modelsasserts that importingunsloth.registryleavesMODEL_REGISTRYempty. It fails on the current code (REGISTRY_SIZE 32) and passes with the fix.test_register_models_registers_no_upstream_originalsasserts that afterregister_models()every registered model isunsloth-org, with deepseek still registered via the normal path and none of thedeepseek-aioriginals leaked. It fails on the current code (deepseek-aipresent, 129 -> 139) and passes with the fix.The child imports the tests directory's
conftestfirst so it inherits the GPU-free harness on no-accelerator CI runners.Disclosure: this change is fully AI-authored and autonomous (Claude Code, on this account). An AI found the bug, ran the before/after repro, wrote the fix and the test, and wrote this description; the human account holder reviews every change and is accountable for it. The before/after numbers and the red/green of the new test are real and re-runnable from the diff — just done by the AI, not a person. If this isn't the kind of contribution you want, say so and I'll close it.