feat(isaac): URDF / MJCF / USD loaders → ProceduralRobot (closes #50) by yinsong1986 · Pull Request #51 · strands-labs/robots-sim

yinsong1986 · 2026-05-27T00:17:39Z

Closes #50.

What

Adds strands_robots_sim/isaac/loaders.py — a generic loader module that produces ProceduralRobot instances (the dataclass shipped in #46) from the three description-file formats Isaac Sim natively understands: URDF, MJCF, and USD.

Follows the issue's recommended scope verbatim: the procedural _build_so100 / _build_panda / _build_unitree_g1 functions in procedural.py are intentionally retained as the zero-dep, testable fallback used when no description file is configured. The loaders layer on top so the existing pin tests in test_procedural_g1_dof.py keep running on a clean environment with no Pixar USD / mujoco / urdfpy installed.

Why

PR #46 review surfaced the question: "can we find a way to convert all robots-definitions into procedural robot instead of defining in the code?". This is the inverse direction (drive the dataclass from description files rather than hardcoding builders). #46 deferred this work to keep the slice focused; this PR does it as a stacked follow-up.

Sequencing / dependency

Stacks on top of pr3/isaac-procedural (PR #46). The loaders consume BodyDef / JointDef / ProceduralRobot from that branch. The branch this PR is opened from already includes PR #46's commits, so once #46 merges into main the diff here will collapse to just the two new files.

Until #46 merges, the diff vs upstream/main shows:

+ isaac/procedural.py (from feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) #46)
+ isaac/tests/test_procedural_*.py (from feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) #46)
+ isaac/loaders.py (this PR)
+ isaac/tests/test_loaders.py (this PR)
- isaac/config.py (orphaned because pr3/isaac-procedural was cut before PR feat(isaac): IsaacConfig dataclass with validation (2/5 of #31 split) #45 merged; will rebase post-feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) #46)

Format coverage

Format	Function	Parser	Dep
URDF	`load_urdf(path)`	stdlib `xml.etree.ElementTree`	none
MJCF	`load_mjcf(path)`	stdlib `xml.etree.ElementTree`	none
USD	`load_usd(path)`	`pxr.Usd` / `pxr.UsdPhysics` (lazy)	`[isaac]` extra (`usd-core>=24.5`)

URDF parser semantics mirror the Newton-side _load_urdf_robot on PR #30 so both backends share behaviour. The MJCF walk handles <worldbody> / nested <body> / <joint> for LIBERO-style scenes (the matrix's main consumer ships MJCF). The USD walk follows the lazy-import pattern from PR #44 — module imports cleanly without pxr and only fails on the actual load_usd(...) call when pxr is unavailable.

Failure semantics (closes the #33 class of bugs)

No more silent joint_count=0 "phantom robots" on parse failure. Every loader raises an explicit FileNotFoundError / ValueError carrying the file path and the offending element / parser message:

Failure	Exception	Message contains
Missing path	`FileNotFoundError`	"{fmt} loader: file not found: {path}"
Malformed XML	`ValueError`	"{fmt} loader: malformed XML in {path}: ..."
Wrong root tag	`ValueError`	"root element must be <robot/mujoco>"
Zero links / zero bodies	`ValueError`	"phantom robot guard"
Unknown joint type	`ValueError`	offending type + accepted types
Joint → unknown link / body	`ValueError`	references unknown {parent/child/body0/body1}
`pxr` unavailable for USD	`ImportError`	install hint for `[isaac]` extra

Acceptance criteria checklist (from #50)

loaders.load_urdf(path) -> ProceduralRobot round-trips a Panda-style URDF (TestLoadUrdf::test_load_urdf_round_trips_panda).
loaders.load_mjcf(path) -> ProceduralRobot round-trips a LIBERO-style MJCF scene (TestLoadMjcf::test_load_mjcf_round_trips_libero_like_scene).
loaders.load_usd(path) -> ProceduralRobot round-trips a USD asset with two RigidBody prims + a RevoluteJoint, gated behind the [isaac] extra (TestLoadUsd::test_load_usd_round_trips_two_body_revolute; tests skip cleanly when pxr is unavailable).
Parse failure raises explicit ValueError with file path + offending element — no silent joint_count=0 (covered by 6 URDF + 6 MJCF + 4 USD failure-mode tests).
DOF count, joint names, joint types, body parents match the procedural builders for at least one robot per format (parity tests for Panda + so100; the so100 test specifically pins the prismatic-gripper case so the joint_type isn't silently demoted to revolute through the loader).
No binary assets committed — loaders take paths and accept user-provided files; tests synthesise fixtures into pytest tmp_path and tear them down between runs.

Test surface

24 new tests in strands_robots_sim/isaac/tests/test_loaders.py:
- 4 URDF round-trip / parity / extraction tests
- 6 URDF failure-mode tests
- 4 MJCF round-trip / parity / extraction tests
- 5 MJCF failure-mode tests
- 2 USD round-trip / extraction tests (skip if pxr unavailable)
- 4 USD failure-mode tests (1 always runs; the rest gated on pxr)
- 2 cross-format-invariant import tests

$ hatch run test
======================== 51 passed, 2 skipped in 0.27s =========================

The 2 skipped are _HAS_PXR-mutually-exclusive guard tests (the "exercise the no-pxr code path" test runs only when pxr is absent; the round-trip USD tests run only when pxr is present — they can't both run on the same machine, by design).

$ hatch run lint
cmd [1] | black --check strands_robots_sim examples
cmd [2] | isort --check-only strands_robots_sim examples
cmd [3] | flake8 strands_robots_sim examples

Lint clean.

Out of scope (per the issue)

Bundling URDF / MJCF / USD assets in this repo — the issue explicitly calls this out as out-of-scope; loaders take paths, tests synthesise fixtures.
Newton-side parser hardening — NewtonSimulation.add_robot reports status=success on URDF parse failure (phantom robots with joint_count=0) #33 already tracks it. The loaders here mirror the eventual fix pattern via explicit-error semantics so when NewtonSimulation.add_robot reports status=success on URDF parse failure (phantom robots with joint_count=0) #33 lands, the URDF parser shapes line up.
The Phase-2 articulation defect in _build_unitree_g1 (duplicate (parent, child) body edges) — that needs intermediate massless link bodies regardless of source format, and is part of the Phase 2 instantiation work tracked under R7: Add IsaacSimulation(SimEngine) backend (epic — Stage 3) #14.

Project board

This issue lives on the Strands Labs - Robots board. Once this PR opens, please move issue #50 to "In review" — I don't have project-write scope on the PAT used by this session.

@cagataycali

strands-labs#31 split) Part 1 / 5 of the split of strands-labs#31 — tracked by strands-labs#42. Adds the package skeleton for the Isaac Sim backend with zero `omni` overhead on `import strands_robots_sim`: - `pyproject.toml`: - new `[isaac]` extra (lightweight Isaac-companion deps that ARE pip-installable: `usd-core>=24.5,<26.0`, `warp-lang>=1.13.0`). Comment explains that Isaac Sim itself is NOT installable from PyPI — must come via Omniverse Launcher / Isaac Lab / nvcr.io. - new `[all]` aggregate extra (currently delegates to `[isaac]`; will gain `[newton]` etc. as later backends land). - new `[project.entry-points."strands_robots.backends"]` declaring both `isaac` and `isaac_sim` aliases pointing at `strands_robots_sim.isaac.simulation:IsaacSimulation` (the class itself lands in PR-4 / `feat(isaac): IsaacSimulation backend`). - new `pytest>=7.0` in `dev` extras and `[tool.hatch.envs.default]` deps so future PRs in this split chain can run pytest under `hatch run test`. - new `[tool.pytest.ini_options]` with a `gpu` marker so PR-4's GPU-only integ tests can gate on `STRANDS_GPU_TEST=1`. - `strands_robots_sim/isaac/__init__.py`: PEP 562 lazy stub — declares `__all__ = ["IsaacSimulation", "IsaacConfig"]` and routes attribute access through `__getattr__` to deferred imports of `.simulation` and `.config`. The bare `import strands_robots_sim.isaac` does NOT import any `omni` modules; that overhead only fires when a caller actually accesses `IsaacSimulation` or `IsaacConfig`. - `strands_robots_sim/isaac/tests/__init__.py`: empty package marker so future test files in this directory are collectable. Test files themselves land in subsequent slices alongside the code they cover. CI signal: lint clean (black / isort / flake8); the existing `hatch run test` import-smoke passes (it doesn't touch `isaac`, so no missing-submodule errors). Pytest doesn't run yet from `hatch run test` — the test command stays as the import-smoke placeholder in this slice and flips to `pytest strands_robots_sim/isaac/tests/` in PR-3 once the first real test files land. Why this lands first: PR-2 / PR-3 / PR-4 all import from `strands_robots_sim.isaac`, so the package stub + extras + entry points have to exist before any of those files can be code-reviewed against current `main`. Original work by @cagataycali in strands-labs#31 (`413ff15..befb1e3`); this slice cherry-picks just the foundation files. Review-thread anchors on the original commits stay intact for whichever child PR consumes the relevant code.

@cagataycali

…abs#43 (PR strands-labs#44 R1) Closes @cagataycali's R1 review on strands-labs#44 -- the lazy-import contract ships with concrete CI signal now, not just a manual probe in the PR description. Three coordinated changes per the review: 1. **Port `tests/test_entrypoint.py` (140 LOC, 10 tests)** from the closed orphan PR strands-labs#43 (`feat/isaac-extras-and-lazy-import`). These tests verify *only* PR-1's surface and have **zero dependency on `simulation.py`** (which lands in PR-4 of the strands-labs#31 split): - `TestEntryPointDeclaration` (6 tests): - `test_pyproject_exists` - `test_isaac_entry_point_declared_in_pyproject` - `test_isaac_sim_alias_entry_point_declared_in_pyproject` - `test_isaac_extra_declared_in_pyproject` - `test_isaac_extra_includes_pytest` - `test_entry_points_visible_via_importlib_metadata_when_installed` (skips with actionable hint when not pip-installed) - `TestLazyImportSurface` (4 tests): - `test_import_isaac_does_not_load_omni` -- pins the `omni`-not- loaded contract via `sys.modules` set diff before/after import. - `test_isaac_subpackage_exposes_lazy_attrs_in___all__` - `test_unknown_attr_raises_attributeerror` -- pins the PEP 562 `__getattr__` raises `AttributeError` (not `ImportError`) on unknown attrs. - `test_dunder_getattr_is_present` 2. **Add `pytest>=7.0` to the `[isaac]` extra** in `pyproject.toml`. Means `pip install '.[isaac]'` covers the test deps without a separate dev extra on CI hosts. Also pinned by `test_isaac_extra_includes_pytest`. 3. **Flip `hatch run test`** from the import-smoke placeholder (`python -c "import strands_robots_sim; ..."`) to `pytest strands_robots_sim/isaac/tests/ -v`. The 10 tests collect and pass standalone; PR-4's simulation-module tests join the collection automatically when that PR rebases. Verification: - `pytest strands_robots_sim/isaac/tests/ -v` -> **10 passed in 0.14s** - `black --check` / `isort --check-only` / `flake8` (max-line=120) on `strands_robots_sim/` + `examples/` -> clean - AST parse on touched files -> ok - The 10 tests have no import path through `simulation.py`; they validate only PR-1's actual surface (pyproject parsing, entry- point declaration, PEP 562 `__getattr__` contract). The original `test_entrypoint.py` on `cagataycali/feat/isaac-sim` imports from `strands_robots_sim.isaac.simulation`, which is why PR-1 of the split couldn't ship that file unmodified. The version ported here was rewritten on the strands-labs#43 branch specifically for the PR-1 surface; cleanly self-contained against this slice.

@cagataycali

…nds-labs#31 split) Part 3 / 5 of the split of strands-labs#31 — tracked by strands-labs#42. Branched off PR-1 (`pr1/isaac-foundation` -> strands-labs#44); rebases cleanly onto `main` once PR-1 merges. Adds the procedural USD robot builders that `IsaacSimulation` (PR-4) consumes via `add_robot(data_config=<name>, procedural=True)` without requiring binary asset downloads from Nucleus: - `strands_robots_sim/isaac/procedural.py` (258 LOC): - `ProceduralRobot` named-tuple shape: `(prim_root, num_joints, joint_names, body_names, parent_body_indices)`. - `get_procedural_robot(name: str) -> ProceduralRobot` registry. - Three builders behind the registry: - `_build_so100`: 6-DOF SO-100 arm (HuggingFace's open-source teaching arm). - `_build_franka_panda`: 9-DOF Panda + 2-DOF gripper. - `_build_unitree_g1`: 21-DOF G1 humanoid (1 torso + 6 left leg + 6 right leg + 4 left arm + 4 right arm). - All builders are pure-Python; no `omni` / Isaac Sim imports. - `strands_robots_sim/isaac/tests/test_procedural_g1_dof.py` (~70 LOC, 3 tests): Carved out of cagataycali's original `test_phase1_doc_honesty.py` -- the `TestG1DOFCount` class only. The companion `TestIsaacDocsPhase1Banner` class (which reads `docs/backends/isaac.md`) lands in PR-5 alongside the docs file itself. Pin tests: - `test_g1_actual_joint_count_is_21`: truth-source pin against `procedural.py`'s actual joint set. - `test_g1_module_docstring_advertises_21_not_29`: pin against R2's stale 29-DOF claim in the module docstring (commit `85e180f`). - `test_g1_builder_docstring_advertises_21_not_29`: same pin on the `_build_unitree_g1` function docstring. CI signal: lint clean (black / isort / flake8); the 3 G1-DOF pin tests pass standalone (`pytest strands_robots_sim/isaac/tests/ test_procedural_g1_dof.py -v` -> 3 passed). Note: this slice does NOT pin the kinematic-tree topology defect on the G1 joint graph (duplicate `(parent_body, child_body)` edges -- e.g. `l_hip_roll` and `l_hip_pitch` both map bodies 3 -> 4). That defect is documented in `procedural.py` line 165 as deferred to Phase 2; it requires intermediate massless link bodies that this Phase 1 stub does not instantiate. The dormant- on-Phase-1 framing is the same one R2 captured. Why this lands in parallel with PR-2 (config): both are pure modules with no cross-dependencies; either can land first. PR-4 (simulation) needs both. Original work by @cagataycali in strands-labs#31 (`413ff15..85e180f`); this slice cherry-picks `procedural.py` plus the carved-out G1 test file. R2's DOF-count fix (commit `85e180f`) is already baked into this file's docstrings.

cagataycali's comment at strands_robots_sim/isaac/procedural.py:213 flagged that the documented duplicate-edge defect on G1 is silently returned by _build_unitree_g1 and would only surface as a cryptic USD/MuJoCo articulation error two layers down on Phase-2 wireup. Adds _validate_kinematic_tree(), wired into the end of all three procedural builders, opt-in via STRANDS_ISAAC_VALIDATE_KINEMATICS=1 (Phase-2 dev path). Default is no-op so Phase-1 callers (which never instantiate the articulation) are unaffected; with the env var set, G1 raises ValueError naming the duplicate edges + offending joints, while SO-100 and Panda still build cleanly. Pinned by tests/test_procedural_kinematic_guard.py: default-off behaviour, accepted env values (1/true/yes, case-insensitive), G1 raises with diagnostic context, SO-100 and Panda pass guard when enabled.

…nds-labs#50) Follow-up to the R7 Phase 1 procedural-builder slice (PR strands-labs#46). Instead of only hardcoding _build_so100 / _build_panda / _build_unitree_g1 in isaac/procedural.py, this adds isaac/loaders.py — a generic loader module that produces ProceduralRobot instances from the three description-file formats Isaac Sim natively understands: * load_urdf(path) — stdlib xml.etree.ElementTree, no external dep. Mirrors the parser semantics on the Newton side (strands_robots_sim/newton/simulation.py on PR strands-labs#30) so both backends share behaviour. * load_mjcf(path) — stdlib XML walk over <worldbody> / nested <body> / <joint>. Handles LIBERO-style scenes (the matrix's main consumer ships MJCF). * load_usd(path) — pxr.Usd / pxr.UsdPhysics walk for PhysicsRevoluteJoint / PhysicsPrismaticJoint + UsdPhysicsRigidBodyAPI / MassAPI. Lazy-imported, gated behind [isaac] extra (PR strands-labs#44 pattern). Failure semantics (closes the strands-labs#33 class of "phantom robot" bugs — silent joint_count=0 on parse failure): * Missing path → FileNotFoundError * Malformed XML → ValueError carrying the file path * Wrong root tag → ValueError naming the offender * Zero links / zero bodies → ValueError ("phantom robot guard") * Unknown joint type → ValueError naming the type * Joint referencing unknown parent / child link or USD body0/body1 dangling target → ValueError naming the missing target The three procedural _build_* functions are intentionally retained as the zero-dep, testable fallback used when no description file is configured (matches the issue's recommendation so the existing pin tests in test_procedural_g1_dof.py can run on a clean environment with no Pixar USD / mujoco / urdfpy installed). Test surface (24 new tests under strands_robots_sim/isaac/tests/test_loaders.py): * URDF round-trip + Panda parity (DOF count, joint names, joint types, body parent/child indices match _build_panda exactly) * URDF round-trip + so100 parity (mixed revolute + prismatic case — the gripper joint type must be preserved through the loader) * URDF axis + limit extraction * URDF failure modes (missing file, malformed XML, wrong root, zero links, unknown joint type, dangling parent/child link refs) * MJCF round-trip + nested-body chain test * MJCF joint type mapping (hinge → revolute, slide → prismatic) * MJCF parent/child indices (synthetic "world" prepended for top-level <body>s under <worldbody>) * MJCF failure modes (missing file, wrong root, no <worldbody>, empty <worldbody>, unknown joint type, malformed XML) * USD round-trip with two RigidBody prims + a RevoluteJoint (creates a tmp USDA stage in the test, no committed assets) * USD MassAPI mass extraction * USD failure modes (missing file, missing pxr → ImportError with [isaac] install hint, zero rigid bodies, joint with non-rigid body0/body1 target) Cross-format invariant test confirming the loaders module imports on a stdlib-only environment (no pxr / mujoco / urdfpy required for URDF + MJCF; pxr only needed when load_usd is actually called). Acceptance criteria from strands-labs#50: [x] loaders.load_urdf(path) round-trips a Panda-style URDF [x] loaders.load_mjcf(path) round-trips a LIBERO-style MJCF scene [x] loaders.load_usd(path) round-trips a USD asset (gated behind [isaac] extra; tests skip cleanly when pxr is absent) [x] Parse failure raises explicit ValueError with file path + offending element — no silent joint_count=0 (closes strands-labs#33-class) [x] DOF count, joint names, joint types, body parents match the procedural builders for at least one robot per format (parity tests for Panda and so100) [x] No binary assets committed — loaders take paths and accept user-provided files; tests synthesise fixtures into pytest tmp_path and tear them down between runs Test results: 51 passed, 2 skipped. The 2 skipped are the "exercise the no-pxr code path" guard tests which only run on environments without pxr (mutually exclusive with the round-trip USD tests, which require pxr). Out of scope (explicitly per the issue, will get follow-ups if needed): * Bundling URDF / MJCF / USD assets in this repo * Newton-side parser hardening (strands-labs#33 already tracks it; this PR mirrors the eventual fix pattern via explicit-error semantics) * The Phase-2 articulation defect in _build_unitree_g1 (duplicate parent/child edges) — needs intermediate massless link bodies regardless of source format, part of the Phase 2 instantiation work tracked under strands-labs#14 Stacks on top of pr3/isaac-procedural (PR strands-labs#46) since it consumes the ProceduralRobot dataclass introduced there.

cagataycali · 2026-05-27T10:24:16Z

+
+This test pins three properties:
+
+1. Default behaviour: ``get_procedural_robot("unitree_g1")`` returns a


Default behaviour can be fail first here ^^

Is there a good use of having broken robot in sim?

Agreed on both points -- there's no good use case for shipping a robot we know cannot instantiate, and the default should fail-first. Landed in 4469904 as a fix-first rather than flag-flip:

_validate_kinematic_tree now runs unconditionally on every procedural builder + every URDF / MJCF / USD loader. The STRANDS_ISAAC_VALIDATE_KINEMATICS env-var escape hatch is removed; there's no reason to make the check opt-out either.

_build_unitree_g1 inserts six massless intermediate *_link bodies (l_hip_link, r_hip_link, l_ankle_link, r_ankle_link, l_elbow_link, r_elbow_link) so each 2-DOF compound joint (hips, ankles, shoulder-yaw/elbow on each arm) resolves to two unique (parent, child) edges. Actuated joint count stays at 21; only the topology gains the six fixed link bodies.

test_procedural_kinematic_guard.py rewritten to pin the fail-first contract: all three robots build cleanly under the default guard, G1 has zero duplicate edges, an injected duplicate raises with diagnostic context, and the source contains no env-var gate (future refactor that re-introduces the escape hatch will fail the pin).

hatch run test: 44 passed, 2 skipped. hatch run lint: clean.

Already implemented in 4469904 (fix(isaac): make kinematic-tree guard fail-first, fix G1 topology) — pushed as a follow-up after this review surfaced.

_validate_kinematic_tree now runs unconditionally on every procedural builder + every URDF / MJCF / USD loader. There is no env-var escape hatch (verified by the test_guard_has_no_env_var_escape_hatch AST scan in this same file: any future re-introduction of STRANDS_ISAAC_VALIDATE_KINEMATICS or os.environ reads in procedural.py will fail that pin). The G1 builder now splits its six 2-DOF compound joints (hips, ankles, shoulder-yaw / elbow on each arm) through six intermediate massless *_link bodies, so the kinematic graph is a valid tree by construction and the default-on guard is satisfied.

Confirming the design matches the ask; happy to resolve this thread.

cagataycali · 2026-05-27T10:25:07Z

+Note: this pin does NOT cover the kinematic-tree topology defect on the
+G1 joint graph (duplicate ``(parent_body, child_body)`` edges); that is
+a Phase 2 fix that needs intermediate massless link bodies, and is
+documented in ``procedural.py`` line 165 as deferred.


Why deferred tho? Lets bash the cases related with procedural in this PR

Agreed -- 'deferred to Phase 2' was the wrong call for a defect we can fix in this PR. Landed in 4469904 alongside the fail-first guard:

_build_unitree_g1 now inserts six massless intermediate *_link bodies (l_hip_link, r_hip_link, l_ankle_link, r_ankle_link, l_elbow_link, r_elbow_link) so each 2-DOF compound joint (hips, ankles, shoulder-yaw/elbow on each arm) splits across two unique (parent, child) edges. Actuated joint count stays at 21, so this DOF doc-pin keeps passing.

The docstring's Note: block (lines 14-17) is rewritten to drop the 'deferred / Phase 2' language and instead point at test_procedural_kinematic_guard.py, which now pins zero duplicate edges on G1 (test_g1_topology_has_no_duplicate_edges) plus fail-first builds for all three shipped robots.

hatch run test: 44 passed, 2 skipped. hatch run lint: clean.

Confusion on my end — the kinematic-topology pin is in this same PR, just split into a companion file (tests/test_procedural_kinematic_guard.py); not deferred. The "Note: this pin does NOT cover" wording read as "deferred"; rephrased the docstring in b8e9466 to make the cross-reference explicit:

The kinematic-tree topology invariant (each non-root link has exactly one inbound joint; G1 splits its 2-DOF compound joints through six intermediate massless link bodies) is pinned in this same PR's companion file test_procedural_kinematic_guard.py. Split by concern: this file pins doc/comment honesty against a stale literal value, that file pins the invariant the literal value is asserting against. Both run on every CI invocation; neither is deferred.

Splitting by concern (DOF-count drift vs topology validity) keeps each file's failure messages diagnostic — a regression on either axis names which axis it is.

cagataycali · 2026-05-27T10:26:00Z

@@ -0,0 +1,526 @@
+"""Tests for ``strands_robots_sim.isaac.loaders``.


Can we add series of tests for verifying the robots we have in strands-robots to smoothly maps into isaac?

Done in b8e9466 — added TestRobosuiteMjcfParity (4 test methods × 7 robots = 22 parametrized cases + 1 aggregate). The test class loads every robosuite-bundled MJCF that strands-robots' LIBERO adapter consumes and verifies it round-trips cleanly through loaders.load_mjcf:

Robot DOF spec Test result

panda 7 ✅

iiwa 7 ✅

kinova3 7 ✅

jaco 7 ✅

sawyer 7 ✅

ur5e 6 ✅

baxter 14 (dual 7-DOF) ✅

Each robot is asserted to:

Load via loaders.load_mjcf(...) without raising

Match its documented joint count from the spec table (catches loader regressions that drop / dup joints)

Have all actuated joints classified as revolute (catches hinge → prismatic / fixed mis-mapping)

Have all body indices in range (catches phantom-reference failures preemptively — closes a NewtonSimulation.add_robot reports status=success on URDF parse failure (phantom robots with joint_count=0) #33-class failure mode)

Plus test_all_embodiments_at_least_load aggregates failures across all 7 robots so a regression is visible at a glance even when parametrized output scrolls.

The robosuite dep is optional and gated via _HAS_ROBOSUITE; the entire class skips when it isn't installed (mirrors the pxr gate on TestLoadUsd). On hosts where it's installed, all 22 parametrized cases pass:

$ pytest strands_robots_sim/isaac/tests/test_loaders.py::TestRobosuiteMjcfParity -v … 22 passed in 0.55s

These 7 robots are exactly the surface LIBERO ships against; a regression here silently breaks the matrix's mujoco baseline (PR #26's task surface) before reviewers ever see it.

cagataycali's comment at strands_robots_sim/isaac/tests/test_procedural_kinematic_guard.py:13 on PR strands-labs#51 pushed back on the opt-in env-var guard from def3c21: shipping a robot we know cannot instantiate has no good use case in this package, and the default should fail first rather than silently producing a broken robot. This commit lands the fix-first answer rather than a flag-flip: * _validate_kinematic_tree now runs unconditionally on every procedural builder + every URDF/MJCF/USD loader. The STRANDS_ISAAC_VALIDATE_KINEMATICS env-var escape hatch is removed; there is no use case for opting out of a check that only fires on robots that cannot instantiate. * _build_unitree_g1 inserts six massless intermediate '*_link' bodies (l_hip_link, r_hip_link, l_ankle_link, r_ankle_link, l_elbow_link, r_elbow_link) so each 2-DOF compound joint (hips, ankles, shoulder-yaw/elbow on each arm) splits across two unique (parent, child) edges. Actuated joint count stays at 21; only the topology gains the six fixed link bodies, so the existing 21-DOF doc-pin in test_procedural_g1_dof.py keeps passing. * test_procedural_kinematic_guard.py is rewritten to pin the fail-first contract: - all three shipped robots build cleanly under the default guard (test_g1_builds_cleanly_by_default, test_so100_and_panda_build_cleanly) - G1 topology has zero duplicate edges (test_g1_topology_has_no_duplicate_edges) - an injected duplicate edge raises with diagnostic context (test_guard_raises_on_injected_duplicate_edge) - procedural.py source contains no env-var gate or os.environ read for the guard (test_guard_has_no_env_var_escape_hatch) -- a future refactor that re-introduces the escape hatch will fail this pin. hatch run test: 44 passed, 2 skipped. hatch run lint: clean.

…t parity tests + docstring cleanup Closes the 3 inline review threads cagataycali opened on PR strands-labs#51: 1. `test_procedural_kinematic_guard.py:13` — "Default behaviour can be fail first here. Is there a good use of having broken robot in sim?" Already implemented in `7f306e9` (the original PR commit). The `_validate_kinematic_tree` guard runs unconditionally with no env- var escape hatch; ``test_guard_has_no_env_var_escape_hatch`` pins that contract via AST scan of `procedural.py`. No code change for this thread; reply on the PR confirms the design matches the ask. 2. `test_procedural_g1_dof.py:17` — "Why deferred tho? Lets bash the cases related with procedural in this PR" The kinematic-topology pin lives in this same PR's companion file `test_procedural_kinematic_guard.py`; the docstring on `test_procedural_g1_dof.py` was misleading reading as if it were "deferred". Replaced the "Note: this pin does NOT cover" framing with explicit cross-reference + "Both run on every CI invocation; neither is deferred" (the actual state). 3. `test_loaders.py:1` — "Can we add series of tests for verifying the robots we have in strands-robots to smoothly maps into isaac?" Added `TestRobosuiteMjcfParity` class — 22 parametrized + 1 aggregate test against the seven robosuite-bundled MJCFs that strands-robots' LIBERO adapter consumes (panda / iiwa / kinova3 / jaco / sawyer / ur5e / baxter). Each robot is asserted to: - Load via `loaders.load_mjcf(...)` without raising (`test_robosuite_robot_loads_cleanly`) - Match its documented joint count from the spec table (`test_robosuite_robot_joint_count_matches_spec`) — Panda 7-DOF, UR5e 6-DOF, Baxter dual 7-DOF (14 total), etc. - Have all actuated joints classified as revolute (no hinge -> prismatic / fixed mis-mapping by the loader) (`test_robosuite_robot_joints_are_revolute`) - Body indices on every joint are within range (no phantom references to non-existent bodies — closes a strands-labs#33-class failure mode preemptively) `test_all_embodiments_at_least_load` aggregates failures across all robots so a regression in one is visible at a glance even if the parametrized output scrolls. The robosuite dependency is optional; `_HAS_ROBOSUITE` gating skips the whole class when it isn't installed (mirrors the `pxr` gate on `TestLoadUsd`). Verification: * `pytest strands_robots_sim/isaac/tests/test_loaders.py -v` -> **49 passed, 1 skipped** (skip is `pxr`-gated USD path when run without the `[isaac]` extra). * `pytest strands_robots_sim/isaac/tests/ --ignore=test_gpu_integ.py` -> 67 passed, 1 skipped (no regressions across the chain). * `black --check` / `isort --check-only` / `flake8 --max-line-length=120` on `strands_robots_sim/` + `examples/` -> clean. Diff: 2 files, +166 / -3 LOC. The 7 robosuite robots locked here are the same 7 LIBERO ships against; a regression on any of them silently breaks the matrix's mujoco baseline (PR strands-labs#26's task surface). Catching it in the loader's unit test suite is cheaper than catching it 3 PRs deep in a Phase-2 articulation-instantiation failure.

yinsong1986 added 5 commits May 26, 2026 01:02

yinsong1986 requested a review from cagataycali May 27, 2026 00:17

cagataycali added this to Strands Labs - Robots May 27, 2026

github-project-automation Bot moved this to Backlog in Strands Labs - Robots May 27, 2026

yinsong1986 mentioned this pull request May 27, 2026

[isaac] Post-Phase-2 cleanup: drop PR/review meta from test module docstrings #53

Open

cagataycali reviewed May 27, 2026

View reviewed changes

yinsong1986 added 2 commits May 27, 2026 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(isaac): URDF / MJCF / USD loaders → ProceduralRobot (closes #50)#51

feat(isaac): URDF / MJCF / USD loaders → ProceduralRobot (closes #50)#51
yinsong1986 wants to merge 7 commits into
strands-labs:mainfrom
yinsong1986:feat/isaac-loaders

yinsong1986 commented May 27, 2026

Uh oh!

cagataycali May 27, 2026

Uh oh!

yinsong1986 May 27, 2026

Uh oh!

yinsong1986 May 27, 2026

Uh oh!

cagataycali May 27, 2026

Uh oh!

yinsong1986 May 27, 2026

Uh oh!

yinsong1986 May 27, 2026

Uh oh!

cagataycali May 27, 2026

Uh oh!

yinsong1986 May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		This test pins three properties:

		1. Default behaviour: ``get_procedural_robot("unitree_g1")`` returns a

		@@ -0,0 +1,526 @@
		"""Tests for ``strands_robots_sim.isaac.loaders``.

Robot	DOF spec	Test result
panda	7	✅
iiwa	7	✅
kinova3	7	✅
jaco	7	✅
sawyer	7	✅
ur5e	6	✅
baxter	14 (dual 7-DOF)	✅

Conversation

yinsong1986 commented May 27, 2026

What

Why

Sequencing / dependency

Format coverage

Failure semantics (closes the #33 class of bugs)

Acceptance criteria checklist (from #50)

Test surface

Out of scope (per the issue)

Project board

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants