feat: Robot() factory + top-level lazy imports#86
Conversation
1e2d93b to
253c01a
Compare
yinsong1986
left a comment
There was a problem hiding this comment.
All review comments addressed. LGTM.
|
For all comments in this PR, we should examine common themes and include corrections for them in AGENTS.md so that future agent runs benefit from their lessons. |
Review Thread Triage (14 unresolved)Already fixed in code (4 threads from @yinsong1986) — need thread resolution:
New from @awsarron (10 threads, Apr 10) — need code work:
Additional blockers:
Recommendation: Wait for PR #84 to merge, then rebase and address @awsarron's 10 threads in one pass. Items 7 and 9 may need a design discussion before implementing. 🤖 Pipeline analysis by AI agent. Strands Agents. Feedback welcome! |
1. Rename factory.py → robot.py, robot.py → hardware_robot.py Eliminates two 'Robot' classes in different files. The factory function now lives where users expect: strands_robots.robot.Robot 2. Default mode='sim' instead of mode='auto' Using real hardware should be an explicit decision since it affects the physical world. Robot('so100') now always returns simulation. Use mode='real' to explicitly opt into hardware control. 3. Fix ThreadPoolExecutor leak in _async_utils.py Register atexit.shutdown(wait=False) to clean up the module-level executor on interpreter exit. 4. Remove redundant list_robots() wrapper Was a 1-line passthrough to registry.list_robots(). Now __init__.py points directly to strands_robots.registry.list_robots. 5. Use module names in dataset_recorder docstring 'robot.py' → 'strands_robots.hardware_robot', 'simulation.py' → 'strands_robots.simulation' 6. Make camera shape configurable in dataset_recorder Added camera_shapes parameter to _build_features() instead of hardcoding (3, 480, 640). Default preserved for backward compat. 7. Add mode validation — invalid mode raises ValueError 8. Update __init__.py lazy imports for renamed modules Tests: 230 passed, 10 skipped, 0 failures Lint: ruff check + ruff format clean
7c9cc11 to
3803e26
Compare
📋 Review Status SummaryHi @awsarron — this PR has 11/14 threads resolved. Here's a summary of the 3 remaining unresolved threads to help focus the re-review: Unresolved Thread 1: "Document all env vars in README"
➔ Cross-PR dependency: This is being addressed in PR #87 (docs rewrite), which includes comprehensive env var documentation. Suggest resolving this thread with a note that #87 covers it, or merging #87 first. Unresolved Thread 2: "Where is
|
…s-labs#86 - Add Environment Variables table to README documenting all 6 env vars used across the project (STRANDS_ROBOT_MODE, STRANDS_ASSETS_DIR, STRANDS_URDF_DIR, STRANDS_TRUST_REMOTE_CODE, GROOT_API_TOKEN, MUJOCO_GL) plus cache directory documentation - Add module-level docstring to dataset_recorder.py explaining why it lives at package root (shared by both hardware and simulation paths, avoids circular dependency) - Add docstring to load_lerobot_episode() documenting that it is consumed by simulation.mujoco.policy_runner for replay_episode
…ssets (#84) Simulation foundation layer for `strands-robots`. Pure Python, no MuJoCo dependency. Unblocks #85 (MuJoCo backend) and #86 (Robot factory). ## What's in **Simulation abstractions** (`strands_robots/simulation/`) - `models.py` — `SimWorld`, `SimRobot`, `SimObject`, `SimCamera`, `TrajectoryStep`, `SimStatus` dataclasses. Backend-agnostic: engine handles live in `_model`/`_data`, everything else in `_backend_state: dict`. - `base.py` — `SimEngine` ABC. 12 required abstract methods + 4 optional (raise `NotImplementedError`). Context-manager protocol. `__del__` logs cleanup errors at warning level. - `factory.py` — `create_simulation()` + `register_backend()` with duplicate/alias shadow protection (raises `ValueError`; `force=True` for intentional overrides). Descriptive `ImportError` when a built-in backend module isn't installed. - `model_registry.py` — URDF/MJCF resolution: user-registered → `STRANDS_ASSETS_DIR` → `~/.strands_robots/assets/` → CWD → `robot_descriptions` fallback. Resolves search paths at call time (no import-time `Path.cwd()` snapshot). - `__init__.py` — thin re-exports with lazy `__getattr__`. **Assets** (`strands_robots/assets/`) - `__init__.py` — thin exports only (repo convention). - `manager.py` — path resolution with `safe_join()` traversal protection. `_has_meshes()` uses `os.scandir` + early-exit, cached by `(path, mtime)`. Module-level guard for optional `[sim]` extra — no circular import with `download.py`. - `download.py` — all download logic (`robot_descriptions` → git clone fallback). `_shallow_clone()` enforces `_ALLOWED_CLONE_URL_RE` (HTTPS github.com only). `_copy_and_clean` filters ignored patterns at `copytree()` time so user files in the cache aren't clobbered. **Tools** (`strands_robots/tools/`) - `download_assets.py` — thin `@tool` wrapper (~78 lines) that delegates to `assets.download.download_robots()`. No duplicated logic. **Registry** (`strands_robots/registry/`) - `user_registry.py` — `register_robot()` / `unregister_robot()` persisted to `~/.strands_robots/user_robots.json`. Fails closed on missing asset dir. Warns on alias collisions at registration time. Docstring warns this must not be exposed as an agent `@tool` without `STRANDS_TRUST_REMOTE_CODE` gating (MJCF → MuJoCo plugin code-exec risk). - `loader.py` — merges user-local registry on top of package `robots.json`. Public `invalidate_cache()` API (no private imports from callers). - `robots.json` — 38 → 68 robots (adds aerial, expressive, mobile_manip categories). - `__init__.py` — re-exports `register_robot`, `unregister_robot`, `list_user_robots`, `invalidate_cache`. **Utils** (`strands_robots/utils.py`) - `get_base_dir()` reads `STRANDS_BASE_DIR` — decoupled from `STRANDS_ASSETS_DIR` so setting the assets path no longer drops `user_robots.json` into an unexpected parent. - `get_assets_dir()`, `resolve_asset_path()`, `safe_join()`, `get_search_paths()` — single source of truth; consumed by model_registry, user_registry, assets/manager. **Docs & packaging** - `README.md` — environment variables table (`STRANDS_BASE_DIR`, `STRANDS_ASSETS_DIR`, `GROOT_API_TOKEN`) + cache directory docs. - `AGENTS.md` — documents nested-asset-path convention (e.g. `xmls/asimov.xml` matching upstream layout) and the `auto_download` strategy invariant. - `pyproject.toml` — new `[sim]` extra (`robot_descriptions>=1.11.0,<2.0.0`); included in `[all]`. ## Design decisions **SimEngine ABC contract.** 12 required methods every physics engine must implement; 4 optional (`load_scene`, `run_policy`, `randomize`, `get_contacts`) raise `NotImplementedError` so unimplemented features are explicit during development. `get_observation`/`send_action` are deliberately facade methods bridging Sim ↔ Policy — the agent tool sees a single interface without needing to know the Robot vs Sim split. **Asset resolution order.** Customer assets always win over defaults: `STRANDS_ASSETS_DIR` → `~/.strands_robots/assets/` → `CWD/assets/` → `robot_descriptions` fallback. Single env var for the asset tree (`STRANDS_ASSETS_DIR`); separate `STRANDS_BASE_DIR` for the base dir that holds `user_robots.json`. **Backend registration.** `register_backend()` rejects duplicates by default and blocks shadowing of built-in aliases (`mj`, `mjc`, `mjx`) unless `force=True`. Alias conflicts caught at both the `name` and `aliases` parameters. **Security.** - `safe_join()` applied everywhere registry values flow into filesystem paths (manager + download + user registry). - `_shallow_clone()` URL regex rejects `ssh://`, `git://`, `file://`, non-github hosts. - `register_robot()` is library-only; not surfaced as `@tool`. Docstring spells out the MJCF-plugin exec risk. ## Testing - 338 unit tests pass, 6 skipped, 0 failures - `ruff check` + `ruff format --check`: clean (57 files) - `mypy`: 0 issues in 57 source files - New test files: - `tests/test_simulation_foundation.py` — ABC contracts, factory round-trip, context-manager cleanup - `tests/test_simulation_factory.py` — duplicate rejection, alias shadowing, missing-backend ImportError - `tests/test_user_registry.py` — register/unregister, persistence, validation, path traversal; asserts `STRANDS_ASSETS_DIR` does NOT move the base dir / registry - `tests/test_registry_integrity.py` — auto-download invariant, alias uniqueness, canonical-shadow protection, lerobot_type presence on hardware-only robots ## Review history | Reviewer | Status | Threads | |-------------------|---------------------------------------|-----------------| | @yinsong1986 | APPROVED | 3/3 resolved | | @awsarron | CHANGES_REQUESTED → all addressed | 50/50 addressed | | @max-rattray-aws | COMMENTED → all addressed | 3/3 resolved | Closes #84. --------- Co-authored-by: cagataycali <cagataycali@icloud.com> Co-authored-by: strands-agent <217235299+strands-agent@users.noreply.github.com>
1. Rename factory.py → robot.py, robot.py → hardware_robot.py Eliminates two 'Robot' classes in different files. The factory function now lives where users expect: strands_robots.robot.Robot 2. Default mode='sim' instead of mode='auto' Using real hardware should be an explicit decision since it affects the physical world. Robot('so100') now always returns simulation. Use mode='real' to explicitly opt into hardware control. 3. Fix ThreadPoolExecutor leak in _async_utils.py Register atexit.shutdown(wait=False) to clean up the module-level executor on interpreter exit. 4. Remove redundant list_robots() wrapper Was a 1-line passthrough to registry.list_robots(). Now __init__.py points directly to strands_robots.registry.list_robots. 5. Use module names in dataset_recorder docstring 'robot.py' → 'strands_robots.hardware_robot', 'simulation.py' → 'strands_robots.simulation' 6. Make camera shape configurable in dataset_recorder Added camera_shapes parameter to _build_features() instead of hardcoding (3, 480, 640). Default preserved for backward compat. 7. Add mode validation — invalid mode raises ValueError 8. Update __init__.py lazy imports for renamed modules Tests: 230 passed, 10 skipped, 0 failures Lint: ruff check + ruff format clean
…s-labs#86 - Add Environment Variables table to README documenting all 6 env vars used across the project (STRANDS_ROBOT_MODE, STRANDS_ASSETS_DIR, STRANDS_URDF_DIR, STRANDS_TRUST_REMOTE_CODE, GROOT_API_TOKEN, MUJOCO_GL) plus cache directory documentation - Add module-level docstring to dataset_recorder.py explaining why it lives at package root (shared by both hardware and simulation paths, avoids circular dependency) - Add docstring to load_lerobot_episode() documenting that it is consumed by simulation.mujoco.policy_runner for replay_episode
449aa05 to
0917c24
Compare
|
Rebased onto the updated #85 (which itself is now rebased on main). Stack status:
What changed:
Conflict resolution:
Quality gate:
🤖 AI agent response. Strands Agents. Feedback welcome! |
Rebased on main (post-PR strands-labs#85 merge). Changes: - New strands_robots/robot.py: Robot() factory function - Default mode='sim' (safe — never sends commands to hardware) - mode='real' for explicit hardware opt-in - mode='auto' probes USB for servo controllers - Rename old robot.py → hardware_robot.py (HardwareRobot class) - Updated __init__.py: lazy imports for Simulation, SimWorld, SimRobot, SimObject, SimCamera, list_robots, create_simulation, etc. - Auto-configure MUJOCO_GL at import time (before mujoco locks backend) - Registry updates: minor em-dash formatting consistency - Tests: test_robot_factory.py, test_registry.py, test_registry_integrity.py, test_user_registry.py All review threads (14/14) previously resolved. Addresses feedback from @awsarron and @yinsong1986.
bb959e4 to
bf277f5
Compare
All 14 review items addressed. PR rebased on main (post-#85 merge). CI passing.
✅ Rebased & CI Green
Key changes from the rebase:
Ready for re-review when convenient. 🙏 🤖 AI agent. Strands Agents. Feedback welcome! |
|
Rebased on Changes from the rebase:
Awaiting re-approval (force push invalidated the previous approval). 🤖 AI agent response. Strands Agents. Feedback welcome! |
Tests in tests/simulation/mujoco/ and tests/simulation/ that import or instantiate MuJoCo simulations now properly skip when mujoco is not installed, instead of raising ImportError during collection. Files fixed: - test_agenttool_contract.py - test_backend.py (TestEnsureMujoco class only) - test_recording_backends.py - test_factory.py (TestCreateSimulation class only) - test_policy_runner_behaviour.py
TL;DR
Add
Robot()factory function that auto-detects sim vs real mode, plus lazy imports in__init__.pyfor all simulation types.What changed
strands_robots/factory.pyRobot()factory +list_robots()strands_robots/__init__.pytests/test_factory.pyUsage
Auto-detect logic
STRANDS_ROBOT_MODEenv var (explicit override)"sim"(safest — never accidentally send commands to hardware)Discussion point
Per Arron's feedback: "Robot always returned Robot. robot.backend returned HardwareRobot or Simulator instance" — current implementation is a factory function (returns the backend directly). Consider if
Robotshould be a wrapper class with.backendattribute for nicer typing.Testing
Part 5 of 6 in the MuJoCo simulation PR decomposition