From 2eb71f12b5c4abc84d3ae3920f9662b676f605a3 Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Fri, 15 May 2026 03:31:36 +0000 Subject: [PATCH 1/2] tests: contain security-conftest network block + fix stale mlx paths + skip GPU import in trainer-exec-marker Three CI harness bugs surfaced once the unsloth-side matrix started running zoo's full pytest tree on every PR (the "unsloth_zoo @ main - full pytest (CPU)" step on unslothai/unsloth PR #5428). 1. tests/security/conftest.py The network_blocker fixture was scope="session" + autouse=True and monkey-patched the module-global socket.socket. Once any security test ran, the patch stayed live for every subsequent test in the same pytest session - silently breaking every non-security test that needed the network (test_upstream_pinned_symbols_transformers alone has ~40 parametrised cases that fetch HF modeling source over HTTPS). Drop the scope to "function" so setup/teardown happens per-test; cost is ~10us and blast radius is contained to the security tree only. 2. tests/test_upstream_pinned_symbols_accelerator.py Two tests read unsloth_zoo/mlx_trainer.py and unsloth_zoo/mlx_loader.py as flat modules. They were promoted to the mlx/ subpackage in e6d8f7f (mlx/trainer.py, mlx/loader.py); the tests have been failing with FileNotFoundError ever since. Accept either layout so the test survives the rename and still pins the deprecated-API regression both originally guarded. 3. tests/test_compiler_rewriter_exhaustive.py::test_unsloth_trainer_exec_marker Did pytest.importorskip("unsloth") then a plain import unsloth.trainer. On a CPU-only CI runner without zoo's GPU-spoof harness applied, import unsloth raises NotImplementedError("Unsloth cannot find any torch accelerator? You need a GPU.") - which neither importorskip nor the existing ImportError branch handle. Convert that exact exception to a pytest.skip; the GPU cell still exercises the real import. Local validation (single pytest session, proves the leak is gone): python -m pytest tests/security \ 'tests/test_upstream_pinned_symbols_transformers.py::test_gemma3_modeling_required_classes[v5.5.0]' \ 'tests/test_upstream_pinned_symbols_transformers.py::test_ministral_attention_module_present[v5.5.0]' \ -v -> 17 passed in 0.90s --- tests/security/conftest.py | 20 ++++++++--- tests/test_compiler_rewriter_exhaustive.py | 14 +++++++- ...est_upstream_pinned_symbols_accelerator.py | 33 +++++++++++++++---- 3 files changed, 56 insertions(+), 11 deletions(-) diff --git a/tests/security/conftest.py b/tests/security/conftest.py index 6febeec3e..25a3d6846 100644 --- a/tests/security/conftest.py +++ b/tests/security/conftest.py @@ -1,11 +1,20 @@ """Shared fixtures for the security regression suite. The scanner scripts under audit are designed to be offline-safe. Pin -that invariant by autouse-installing a session-scoped network blocker +that invariant by autouse-installing a function-scoped network blocker that refuses any non-loopback `socket.connect()` from inside the test process. If a future test (or a scanner regression) accidentally tries to reach the public internet, pytest fails loudly instead of leaking the request. + +Scope is intentionally ``function`` rather than ``session``: the swap +mutates a module-global (``socket.socket``), and a session-scoped swap +keeps the patch live for every test pytest runs after the first +security test in the same session -- which silently broke every +network-using test elsewhere in the tree (e.g. +``tests/test_upstream_pinned_symbols_transformers.py`` which fetches +HF modeling source over HTTPS). Per-function setup/teardown costs +~10us and contains the blast radius to security tests only. """ from __future__ import annotations @@ -68,12 +77,15 @@ def connect_ex(self, address): # type: ignore[override] return super().connect_ex(address) -@pytest.fixture(scope = "session", autouse = True) +@pytest.fixture(scope = "function", autouse = True) def network_blocker(): - """Session-scoped fixture; replaces `socket.socket` with a blocker. + """Function-scoped fixture; replaces `socket.socket` with a blocker. Yields nothing; the swap is the side effect. Restored at teardown - so other test sessions (run interleaved) see a clean module. + so the *next* test (security or otherwise) sees the real socket. + Session scope was a footgun: it leaked the patch into every + network-using test in the parent ``tests/`` tree once a single + security test ran. See the module docstring for the regression. """ original = socket.socket socket.socket = _BlockedSocket # type: ignore[assignment] diff --git a/tests/test_compiler_rewriter_exhaustive.py b/tests/test_compiler_rewriter_exhaustive.py index b933ab895..d8d80237c 100644 --- a/tests/test_compiler_rewriter_exhaustive.py +++ b/tests/test_compiler_rewriter_exhaustive.py @@ -2033,7 +2033,15 @@ def test_unsloth_rl_peft_pattern_27_marker(): def test_unsloth_trainer_exec_marker(): """``unsloth/trainer.py:614`` exec()'s synthesized trainer source; - pin that unsloth.trainer is importable.""" + pin that unsloth.trainer is importable. + + Skips on a host without a real accelerator: ``import unsloth`` raises + ``NotImplementedError("Unsloth cannot find any torch accelerator")`` + at top-level on a CPU-only CI runner, which is neither an ImportError + nor a drift signal -- it's just the harness gate. Treat it as skip + so the no-GPU CI cell goes green; the GPU cell still exercises this + test for real. + """ pytest.importorskip("unsloth") try: import unsloth.trainer as trainer_mod @@ -2046,6 +2054,10 @@ def test_unsloth_trainer_exec_marker(): "unreachable.", ) return + except NotImplementedError as e: + if "accelerator" in str(e) or "GPU" in str(e): + pytest.skip(f"No accelerator visible to unsloth import: {e}") + raise # Module must expose some Trainer-family symbol downstream rewriter consumes. if not any( hasattr(trainer_mod, sym) diff --git a/tests/test_upstream_pinned_symbols_accelerator.py b/tests/test_upstream_pinned_symbols_accelerator.py index 41b20d95c..894618a3b 100644 --- a/tests/test_upstream_pinned_symbols_accelerator.py +++ b/tests/test_upstream_pinned_symbols_accelerator.py @@ -215,7 +215,7 @@ def test_moe_expert_merges_call_active_merge_device(): # --------------------------------------------------------------------------- def test_mlx_trainer_uses_modern_memory_apis_only(): - """unsloth_zoo.mlx_trainer must call the non-namespaced memory APIs + """unsloth_zoo.mlx.trainer must call the non-namespaced memory APIs (mx.set_memory_limit, mx.set_cache_limit, mx.set_wired_limit). The namespaced mx.metal.set_* forms are deprecated upstream and reverting to them resurrects the per-run deprecation warning that 70b93ad fixed. @@ -223,9 +223,19 @@ def test_mlx_trainer_uses_modern_memory_apis_only(): import importlib.util import pathlib - mlx_trainer_path = pathlib.Path( + pkg_root = pathlib.Path( importlib.util.find_spec("unsloth_zoo").submodule_search_locations[0] - ) / "mlx_trainer.py" + ) + # The MLX path was promoted from a flat module (mlx_trainer.py) to a + # subpackage (mlx/trainer.py) in e6d8f7f. Accept either layout so the + # test survives the rename. + candidates = [pkg_root / "mlx" / "trainer.py", pkg_root / "mlx_trainer.py"] + mlx_trainer_path = next((c for c in candidates if c.is_file()), None) + assert mlx_trainer_path is not None, ( + f"Neither {candidates[0]} nor {candidates[1]} exists; the MLX " + f"trainer module was relocated again. Update this test's path " + f"candidates." + ) src = mlx_trainer_path.read_text() # The deprecated forms must NOT appear. @@ -240,7 +250,7 @@ def test_mlx_trainer_uses_modern_memory_apis_only(): # The modern forms must appear. for modern in ("mx.set_memory_limit", "mx.set_cache_limit", "mx.set_wired_limit"): - assert modern in src, f"Expected modern API {modern} missing from mlx_trainer.py" + assert modern in src, f"Expected modern API {modern} missing from {mlx_trainer_path.name}" # --------------------------------------------------------------------------- @@ -327,10 +337,21 @@ def test_get_existing_mlx_quantization_detects_both_keys(): """ # Import the helper without triggering the heavy mlx_loader import # chain on the GPU-free harness. We pull the function directly. + # Layout was promoted from mlx_loader.py (flat) to mlx/loader.py + # (subpackage) in e6d8f7f. Try both so the test survives the rename. import importlib.util import pathlib - pkg_loc = importlib.util.find_spec("unsloth_zoo").submodule_search_locations[0] - src = (pathlib.Path(pkg_loc) / "mlx_loader.py").read_text() + pkg_loc = pathlib.Path( + importlib.util.find_spec("unsloth_zoo").submodule_search_locations[0] + ) + candidates = [pkg_loc / "mlx" / "loader.py", pkg_loc / "mlx_loader.py"] + loader_path = next((c for c in candidates if c.is_file()), None) + assert loader_path is not None, ( + f"Neither {candidates[0]} nor {candidates[1]} exists; the MLX " + f"loader module was relocated again. Update this test's path " + f"candidates." + ) + src = loader_path.read_text() # The function must check BOTH key names; otherwise repos saved by # mlx-lm (key "quantization") OR by HF transformers ("quantization_config") From 06718ae1022ae356da374d4913abb1dddd76b63e Mon Sep 17 00:00:00 2001 From: Daniel Han Date: Fri, 15 May 2026 03:50:35 +0000 Subject: [PATCH 2/2] tests: install torchvision in Core CI; harden trainer-exec-marker importorskip path Two follow-up fixes after the first push: 1. .github/workflows/consolidated-tests-ci.yml The Core matrix step installed torch but not torchvision. transformers chains torchvision in at module top for the qwen2_vl / qwen2_5_vl image processors, so the drift-existence probe in test_zoo_source_upstream_refs fired RED on "ModuleNotFoundError: No module named 'torchvision'". The drift message itself says "Either install the dep in CI or remove the zoo reference" -- installing is the right move since zoo legitimately references the image processor. CPU build only. 2. tests/test_compiler_rewriter_exhaustive.py::test_unsloth_trainer_exec_marker First pass wrapped the post-importorskip `import unsloth.trainer` call. But unsloth.__init__ raises NotImplementedError at line 144 (`from ._gpu_init import *`), which means importorskip itself propagates the exception -- it only converts ImportError to skip. Move both `import unsloth` and `import unsloth.trainer` inside the try-block so the NotImplementedError branch catches both. Local verification: python -m pytest tests/test_compiler_rewriter_exhaustive.py::test_unsloth_trainer_exec_marker -v -> 1 passed --- .github/workflows/consolidated-tests-ci.yml | 8 +++++++- tests/test_compiler_rewriter_exhaustive.py | 12 ++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/.github/workflows/consolidated-tests-ci.yml b/.github/workflows/consolidated-tests-ci.yml index 672f2548b..b6c6f5534 100644 --- a/.github/workflows/consolidated-tests-ci.yml +++ b/.github/workflows/consolidated-tests-ci.yml @@ -207,7 +207,13 @@ jobs: set -euxo pipefail python -m pip install --upgrade pip pip install --index-url https://download.pytorch.org/whl/cpu \ - "torch>=2.4.0,<2.11.0" + "torch>=2.4.0,<2.11.0" "torchvision<0.26" + # torchvision: transitive import of transformers.models.qwen2_vl + # / qwen2_5_vl image processors. The Qwen2_VL image-processor + # zoo references chains through `from torchvision...` at module + # top, so a missing torchvision turns the existence-probe drift + # tests RED on "ModuleNotFoundError: No module named 'torchvision'". + # CPU build is plenty; we don't need the CUDA variant. pip install -e .[core] pip install --no-deps "unsloth @ git+https://github.com/unslothai/unsloth@main" || true # Override with matrix-resolved specs. diff --git a/tests/test_compiler_rewriter_exhaustive.py b/tests/test_compiler_rewriter_exhaustive.py index d8d80237c..2db19de49 100644 --- a/tests/test_compiler_rewriter_exhaustive.py +++ b/tests/test_compiler_rewriter_exhaustive.py @@ -2038,14 +2038,18 @@ def test_unsloth_trainer_exec_marker(): Skips on a host without a real accelerator: ``import unsloth`` raises ``NotImplementedError("Unsloth cannot find any torch accelerator")`` at top-level on a CPU-only CI runner, which is neither an ImportError - nor a drift signal -- it's just the harness gate. Treat it as skip - so the no-GPU CI cell goes green; the GPU cell still exercises this - test for real. + nor a drift signal -- it's just the harness gate. ``importorskip`` + only converts ``ImportError`` to ``skip``, so we have to wrap the + whole import path. Treat the no-accelerator case as skip so the + no-GPU CI cell goes green; the GPU cell still exercises the import + end-to-end. """ - pytest.importorskip("unsloth") try: + import unsloth # noqa: F401 import unsloth.trainer as trainer_mod except ImportError as e: + if e.name == "unsloth": + pytest.skip(f"unsloth is not installed: {e}") _drift( "unsloth/trainer.py:614", "import unsloth.trainer",