fix: import strands_robots leaves numpy out of sys.modules - #3593
cagataycali merged 3 commits into
Conversation
The package root promises a bare import that pulls in neither torch, lerobot, numpy nor mujoco, and initialised numpy through two chains on every import. That is load-bearing rather than cosmetic: coverage resolves a dotted --cov=strands_robots.<sub> source with find_spec inside sys_modules_saved(), so a numpy the parent package initialised is dropped from sys.modules and the next import numpy re-executes its Python layer over an already-initialised C extension. On tests/simulation/isaac/ under --cov=strands_robots.simulation that was 149 of 2268 failing on float() of a foreign _NoValue at 4578a5f, and on the current numpy the run does not start at all. policies/__init__ exported Cosmos3Policy eagerly and policies.base imported numpy for one annotation; both now resolve lazily. The MUJOCO_GL selector the root runs at import time lived in simulation.mujoco.backend, so importing it ran simulation/__init__ -> SimEngine -> the policy runner -> the rendering package, twelve numpy-importing modules for a function that reads os.environ. The stdlib-only cluster moves verbatim to strands_robots._mujoco_gl beside _dyld, the other import-time shim; the backend re-imports the helpers its rendering probes still call, doctor reads the vocabulary from the leaf, and the tests that patch a GL helper set it on the module whose namespace the selector reads. Pinned by tests/test_package_lazy_imports.py::TestBareImportLeavesNumpyUnloaded: the bare import in a subprocess, the coverage probe itself, and that the leaf imports only the stdlib at module scope. The first two fail on main. Closes strands-labs#3587
The fleet-example GL test still imported the helper from strands_robots.simulation.mujoco.backend, which this branch narrows to the four names the backend itself reads. mypy reports the stale import as attr-defined and the required check fails before pytest runs. Point the test at strands_robots._mujoco_gl, as the doctor tests on this branch already do.
tests/policies/cosmos3/test_package_import_skips_simulation.py asserted that a bare `import strands_robots` loads `strands_robots.policies.cosmos3` as its non-vacuity check, and relied on that eager import to reach the package whose lazy `sim_ik` re-export it grades. This branch makes `Cosmos3Policy` resolve through `policies.__getattr__`, so the root import no longer loads cosmos3 and the premise assertion failed on CI (the first failure in the run, 8844 of 52988 items executed). The conclusion still holds and is still worth pinning; only the route changed. The probe now imports `strands_robots.policies.cosmos3` itself, snapshots `sys.modules` before and after reading `cosmos3.MinkIKBridge`, and asserts the two `_IK_CHAIN` modules are absent before and present after. The second half is the non-vacuity the eager import used to supply, and it is stronger: a renamed chain module fails the cell instead of making the leak check a check of nothing. Measured on this head with mujoco + libEGL: 5 passed; with cosmos3 re-exporting sim_ik eagerly, 1 failed naming the two leaked modules; with a stale `_IK_CHAIN` entry, 1 failed naming the missing module. Docstring updated: the root's own footprint is graded in test_package_lazy_imports.
|
The required suite runs under
All 5 are |
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Restores the documented bare-import contract (import strands_robots leaves numpy out of sys.modules) by cutting the two chains that initialised it: policies/__init__ now resolves Cosmos3Policy through the package's existing PEP 562 __getattr__ (same shape as list_policy_types, with the TYPE_CHECKING import keeping the export statically defined and __all__ unchanged), policies.base moves its annotation-only numpy under TYPE_CHECKING with a string annotation, and the stdlib-only MUJOCO_GL selector cluster moves verbatim out of simulation.mujoco.backend into a new strands_robots/_mujoco_gl.py leaf beside _dyld. I verified the move with git diff --color-moved=zebra — every non-moved line is a docstring, an import, or one xref path; no logic changed. The leaf really is stdlib-only at module scope (its one in-function import, strands_robots.utils.get_base_dir, is itself stdlib-only at module scope, so the NVIDIA ICD staging path adds no heavy import either), and no production module outside backend.py's re-import and doctor.py reaches the moved names from the old path. The private names kept in simulation/__init__'s lazy map still resolve. The fix is pinned three ways in tests/test_package_lazy_imports.py (the property in a subprocess, the exact coverage find_spec-inside-sys_modules_saved() sequence, and an AST guard that the leaf stays stdlib-only), and the invalidated cosmos3 premise test was restated rather than deleted, with a stronger non-vacuity check — per the AGENTS.md guidance on replacing invalidated premise tests.
What's good
- The move is verbatim and auditable; the r0-r2 round changelog honestly names what each red check found and what changed.
- Public API is untouched:
from strands_robots.policies import Cosmos3Policystill works,__all__is unchanged, and the lazy pattern matches the in-tree precedent exactly. - The monkeypatch-target consequence (a patch on the backend's re-imported name is not read by the selector) is stated in the leaf docstring, AGENTS.md, and applied consistently across the five re-pointed test files.
- Changelog fragment uses the issue number, so it shipped in the first push.
Verification suggestions
git diff --color-moved=zebra main...HEAD -- strands_robots/simulation/mujoco/backend.py strands_robots/_mujoco_gl.py— confirms the move carries no logic edit.python -c "import sys, strands_robots; assert 'numpy' not in sys.modules and 'mujoco' not in sys.modules"in the CI venv (mujoco installed) — the property itself, one line.
Closes #3587
What
import strands_robotsnow leaves numpy out ofsys.modules, which__getattr__has documented all along. Two chains initialised it on every bare import:policies/__init__->Cosmos3Policy(eager) andpolicies.base(import numpyfor one annotation)Cosmos3Policyresolves through the package's existing PEP 562__getattr__besidelist_policy_types;basekeepsnpunderTYPE_CHECKINGwith a string annotationsimulation.mujoco.backend->simulation/__init__->SimEngine->policy_runner->rendering.video-> the wholerenderingpackage +dataset_recorderMUJOCO_GLcluster (_is_headless..._configure_gl_backend, 325 lines) moves verbatim tostrands_robots/_mujoco_gl.py, beside_dyld, the other import-time shimThe backend re-imports the four helpers its rendering probes still call;
doctorreads the vocabulary from the leaf; thesimulation/__init__lazy map points its two private GL names at the leaf. Nothing in the moved block changed -git diff --color-movedon the two files shows the move.Why the leaf, and not TYPE_CHECKING tweaks
The issue's second chain is not a stray import.
simulation/base.pyimportsPolicyRunnerat module scope on purpose (the comment there: an inline import would leave an AST-visible cycle CodeQL flags), and the runner needs the rendering package for real. Twelve numpy-importing modules stood behind a function that readsos.environ, and no import understrands_robots.simulationcan avoid that package's__init__. A leaf outside the package is the only shape that cuts it, and_dyldis the precedent: same job (a shim the root runs at import time), same home.The cost is that a test patching one of the GL helpers has to set it on the module whose namespace the selector reads. Five test files re-point their alias to
strands_robots._mujoco_gl as gl_mod;test_backend.pykeepsbackend_modfor the probes that stayed. The leaf's docstring and AGENTS.md both say so.Evidence
Runner with mujoco + libEGL installed (the CI shape), same venv, branch vs
main:main@ c7d5b87import strands_robots->numpy in sys.modulesTrue(12 importers)False(0)tests/simulation/isaac/with--cov=strands_robots.simulationImportError: cannot load module more than once per processinnumpy/_core/multiarray.py(the current numpy's spelling of the_NoValuecorruption the issue measured at 4578a5f)--no-covwhole-tree-checkroster (143 graders, run as a delta)test_all_exports_are_statically_defined+test_examples_mujoco_gl+tests/test_doctor*ruff check/ruff format --checkTests
tests/test_package_lazy_imports.py::TestBareImportLeavesNumpyUnloaded, three cases:test_the_bare_import_leaves_numpy_out_of_sys_modules- subprocess, the property itself. Fails onmain:import strands_robots initialised numpy: ['numpy', 'numpy.__config__', 'numpy._array_api_info'].test_a_coverage_style_probe_of_a_subpackage_leaves_numpy_usable- subprocess running exactly what coverage does (find_specinside a saved-and-restoredsys.modules, thenimport numpy, thenfloat(ndarray.max())). Fails onmainwith theImportErrorabove.test_the_gl_selector_leaf_imports_only_the_stdlib_at_module_scope- AST over the leaf'stree.bodyagainstsys.stdlib_module_names, because a third-party import added there lands on everyimport strands_robotsand nothing else would report it.Docs
AGENTS.md's "Cheap-guard optional imports" bullet cited the old import path; corrected, plus one bullet recording why an import-time shim lives in a stdlib-only leaf and where a patch on a GL helper has to be set. Fragment:
changelog.d/3587-import-leaves-numpy-unloaded.md.Round changelog
4de8d76): required check red onhatch run lint- onemypyattr-definedattests/test_fleet_examples_run_on_macos.py:20, a sixth GL-helper importer the r0 sweep missed (it imports_mujoco_gl_valid_valuesfrombackend, and r0 narrowed the backend's re-import to the four names it reads itself). Re-pointed atstrands_robots._mujoco_gl, matching the doctor tests on this branch. The lint step runs ahead of pytest so no test verdict was hidden behind it; locally the file is mypy/ruff clean and the 9 GL-touching test files pass (228 passed, 5 skipped). No production change.722609b): required check red on pytest, first failure at 16% (-x, 8844 of 52988 ran):tests/policies/cosmos3/test_package_import_skips_simulation.pyasserted that a bareimport strands_robotsloadspolicies.cosmos3as its non-vacuity check - the exact eager import this branch removes. A premise test invalidated by the fix, so it is restated rather than deleted: the probe now importsstrands_robots.policies.cosmos3itself and snapshotssys.modulesbefore and after readingcosmos3.MinkIKBridge; the two_IK_CHAINmodules must be absent before and present after. The second half is the non-vacuity the eager import used to supply, and it is stronger - a renamed chain module now fails the cell rather than making the leak check vacuous. Verified with mujoco + libEGL: 5 passed on this head; eagersim_ikre-export planted in cosmos3 -> 1 failed naming both leaked modules; stale_IK_CHAINentry -> 1 failed naming the missing module. Swept the un-run 84% for the same class (tests asserting a subpackage is loaded by the bare import): this file was the only one. Delta on the 10 GL/import-touching test files, branch vsmain: 275 vs 272 passed (+3 = the new cells), same single lerobot-absent failure on both, 0 branch-only. One test file, no production change.