refactor(core): nothing below the app layer reaches into it - #3859
cagataycali merged 2 commits into
Conversation
The app layer is where the hosts live - the hardware Robot, the rollout runner, teleoperation, recording - so every layer under it exists to be composed by one. Three runtime imports pointed the other way, and each was a contract stored with its first host rather than under all of them. RecordingFrameError moves to a new core module. It is raised in app by DatasetRecorder.add_frame and caught in sim|policies by the rollout drivers, which imported the recorder - numpy, LeRobot version probing and all - to name one exception. teleop_mixin is declared a drivers|mesh member. It reads utils alone at module scope and is mixed into three hosts in three layers: hardware_robot (app), the MuJoCo Simulation (sim|policies) and device_connect.sim_driver (drivers|mesh). It belongs under the lowest of them; the code is unchanged. The roster in KNOWN_UPWARD_EDGES loses those three lines, so the inversions drop 7 -> 4 and the only family left is the four mesh robots that call an @tool entry point. tests/test_import_layers_are_a_dag.py gains the property as a pin plus one row per placement.
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
This PR removes the last three runtime import edges that pointed into the app layer, matching its title and description exactly. RecordingFrameError moves from strands_robots/dataset_recorder.py to a new stdlib-only core module strands_robots/recording_errors.py (the class is a contract shared between the recorder that raises it and the rollout drivers that catch it, so it now sits below both); teleop_mixin is re-declared as a drivers|mesh member in scripts/check_import_layers.py with no code change, since its module-scope imports are utils alone and its hosts span three layers. Exception identity is preserved: dataset_recorder.py re-imports the name at module top, so existing from strands_robots.dataset_recorder import RecordingFrameError call sites (including two test files) resolve to the same class object and every existing except clause keeps working. Verified on the head SHA: scripts/check_import_layers.py reports 4 declared upward edges (down from 7), 0 runtime cycles, OK; all 21 tests in tests/test_import_layers_are_a_dag.py pass; the recorder posture/frame-loss test suites (101 tests) pass; ruff check and format are clean; no non-ASCII in the new files.
What's good
- The new DAG tests pin the property (
test_no_layer_below_app_reaches_into_it) plus one parametrised row per placement stating layer, justifying imports, caller layers, and the single named late read - so a future promotion of the mixin's deferredteleoperatorimport to module scope fails loudly rather than silently re-inverting the graph. - The new core module imports nothing internal and nothing outside the stdlib, so drivers naming the error no longer pay for numpy/LeRobot import costs.
- Docs and the changelog fragment were updated in the same PR, including every
:class:cross-reference insimulation/base.pyandpolicy_runner.py.
appholds the hosts - the hardwareRobot, the rollout runner, teleoperation, recording - so every layer under it exists to be composed by one. Three runtime imports pointed the other way; each was a contract stored with its first host instead of under all of them.scripts/check_import_layers.pysim|policies -> appdrivers|mesh -> appdrivers|mesh -> toolsRecordingFrameError->strands_robots/recording_errors.py(core). Raised inappbyDatasetRecorder.add_frame, caught insim|policiesby the rollout drivers - which imported the recorder module, numpy and LeRobot version probing included, to name one exception.teleop_mixinis declared adrivers|meshmember. No code change: it readsutilsalone at module scope and is mixed into three hosts in three layers -hardware_robot(app), the MuJoCoSimulation(sim|policies),device_connect.sim_driver(drivers|mesh) - so it belongs under the lowest of them, not with the first one that needed it.Tests.
tests/test_import_layers_are_a_dag.pygains the property (no runtime edge targetsapp) and one parametrised row per placement: the layer, the imports that justify it, the caller layers that need it, and the one late read above the layer (teleoperator, deferred because it imports lerobot) pinned by name rather than allowed in general. Onmainwith only the test file applied: 3 failed / 18 passed; on this branch 21 passed.+123/-39 across 8 files; the +37 is the new core module, which carries the moved class and nothing else. Serves #3818 §0.8-0.9 (layered DAG, zero cycles as a test).