Follow-up work for the remaining comments left on #84 after merge. All items are polish / minor correctness — nothing blocking the simulation foundation landing.
Tasks
1. strands_robots/simulation/models.py — document _model / _data / _backend_state distinction
Thread: #84 (comment)
Reply posted in the thread, but worth folding into the actual class docstring more prominently so future backend implementers don't have to dig through review history. State clearly:
_model: engine's core model handle (mujoco.MjModel, Isaac Scene, PyBullet body registry)
_data: engine's core simulation state (mujoco.MjData, Isaac World)
_backend_state: catch-all dict for everything else (XML, tmpdirs, recording buffers, caches) — prefer this over adding new fields
2. strands_robots/registry/robots.json:551 — registry path regression guard
Thread: #84 (comment)
The asimov_v0.xml → xmls/asimov.xml change happened because the upstream repo layout shifted. To prevent this class of regression, add an integration test that walks the full registry and asserts every entry resolves to an existing file via resolve_model_path(). Any upstream layout change or registry typo should fail CI immediately.
- Add
tests_integ/test_registry_resolves.py — parametrised over every robot in robots.json
- Skip gracefully in unit CI; run in the existing
test-integ hatch env (GPU not required for path resolution)
3. strands_robots/assets/manager.py:258 — lightweight is_present check for status queries
Thread: #84 (comment)
list_available_robots() currently calls resolve_model_path() per robot, which can trigger auto-download attempts, mesh cache walks, and multiple filesystem scans. For download_assets(action="status") this means hitting the network per registry entry.
- Add a
is_robot_asset_present(name) -> bool helper that does a pure filesystem check (no auto-download, no mesh walk) — just checks whether the resolved XML exists on one of the search paths
- Rewire
list_available_robots() to use it
- Keep
resolve_model_path() as-is for the "fetch if missing" path
4. strands_robots/assets/download.py:15 — fix stale [mujoco] extra reference
Thread: #84 (comment)
The extra was renamed to [sim] during the PR but a module-level docstring still says [mujoco]. One-line grep-and-fix:
grep -rn 'strands-robots\[mujoco\]' strands_robots/ README.md
5. strands_robots/utils.py:178 — remove dead dedup check in get_search_paths()
Thread: #84 (comment)
if user_cache not in paths is always true because paths starts empty when that check runs. Remove the dead check, keep the meaningful cwd_assets dedup (which matters when CWD happens to be the assets dir).
Acceptance
Context
Auto-filed by DevDuck at PR #84 merge time.
Follow-up work for the remaining comments left on #84 after merge. All items are polish / minor correctness — nothing blocking the simulation foundation landing.
Tasks
1.
strands_robots/simulation/models.py— document_model/_data/_backend_statedistinctionThread: #84 (comment)
Reply posted in the thread, but worth folding into the actual class docstring more prominently so future backend implementers don't have to dig through review history. State clearly:
_model: engine's core model handle (mujoco.MjModel, IsaacScene, PyBullet body registry)_data: engine's core simulation state (mujoco.MjData, IsaacWorld)_backend_state: catch-all dict for everything else (XML, tmpdirs, recording buffers, caches) — prefer this over adding new fields2.
strands_robots/registry/robots.json:551— registry path regression guardThread: #84 (comment)
The
asimov_v0.xml→xmls/asimov.xmlchange happened because the upstream repo layout shifted. To prevent this class of regression, add an integration test that walks the full registry and asserts every entry resolves to an existing file viaresolve_model_path(). Any upstream layout change or registry typo should fail CI immediately.tests_integ/test_registry_resolves.py— parametrised over every robot inrobots.jsontest-integhatch env (GPU not required for path resolution)3.
strands_robots/assets/manager.py:258— lightweightis_presentcheck for status queriesThread: #84 (comment)
list_available_robots()currently callsresolve_model_path()per robot, which can trigger auto-download attempts, mesh cache walks, and multiple filesystem scans. Fordownload_assets(action="status")this means hitting the network per registry entry.is_robot_asset_present(name) -> boolhelper that does a pure filesystem check (no auto-download, no mesh walk) — just checks whether the resolved XML exists on one of the search pathslist_available_robots()to use itresolve_model_path()as-is for the "fetch if missing" path4.
strands_robots/assets/download.py:15— fix stale[mujoco]extra referenceThread: #84 (comment)
The extra was renamed to
[sim]during the PR but a module-level docstring still says[mujoco]. One-line grep-and-fix:grep -rn 'strands-robots\[mujoco\]' strands_robots/ README.md5.
strands_robots/utils.py:178— remove dead dedup check inget_search_paths()Thread: #84 (comment)
if user_cache not in pathsis always true becausepathsstarts empty when that check runs. Remove the dead check, keep the meaningfulcwd_assetsdedup (which matters when CWD happens to be the assets dir).Acceptance
hatch run lint && hatch run test && hatch run test-integall greenContext
Auto-filed by DevDuck at PR #84 merge time.