feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split)#46
Merged
Merged
Conversation
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.
This was referenced May 26, 2026
Merged
cagataycali
reviewed
May 26, 2026
Member
cagataycali
left a comment
cagataycali
left a comment
There was a problem hiding this comment.
Local review on Thor — 3/3 tests pass ✅, but G1 defect needs a runtime guard
Verified locally:
pip install -e . pytestcleanpytest strands_robots_sim/isaac/tests/test_procedural_g1_dof.py -v→ 3 passed in 0.02sget_procedural_robot('unitree_g1').num_joints == 21✅- DOF count drift (R2 fix
85e180f) is correctly preserved in module + builder docstrings
Real defect: G1 has 6 duplicate parent→child edges
Ran Counter(((j.parent_body, j.child_body) for j in g1.joints)) on the actual built robot:
Duplicate parent->child body edges: {(3,4): 2, (5,6): 2, (7,8): 2, (9,10): 2, (12,13): 2, (15,16): 2}
Total joints: 21
Total bodies: 17
The NOTE block at line 166-171 acknowledges this as Phase 2 work, but the code has no runtime guard. If anyone enables Phase-2 articulation instantiation (which the rest of #47 expects), _build_unitree_g1() returns a ProceduralRobot that USD/MuJoCo will reject with a non-obvious error (or worse, silently merge joints).
What works ✅
- SO-100 (line 64-96), Panda (line 99-136) builders — clean kinematic chains, no duplicate edges
ProceduralRobot.num_jointsproperty correctly reflects the joint list length- Registry-style
get_procedural_robot(name)API is clean - 3 R2 pin tests guard against DOF count regressions
Leaving an inline suggestion to add the runtime guard so Phase 2 fails fast.
9 tasks
…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.
…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.
yinsong1986
force-pushed
the
pr3/isaac-procedural
branch
from
1a44179 to
def3c21
Compare
Contributor
Author
|
Rebased onto SHA shift on this branch:
Same content; rebased commit objects only. Parallel-session reconciliation: the kinematic-edge-guard review-fix commit ( Local verification post-rebase: After PR-1 (#44) and this PR both land in |
cagataycali
requested changes
May 27, 2026
This was referenced May 27, 2026
cagataycali
reviewed
May 27, 2026
cagataycali
approved these changes
May 27, 2026
github-project-automation
Bot
moved this from In review
to Done
in Strands Labs - Robots
yinsong1986
added a commit
that referenced
this pull request
May 28, 2026
…r) (4/5 of #31 split) (#47) * feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) Part 4 / 5 of the split of #31 — tracked by #42. The bulk slice; ~2,000 LOC of simulation backend + tests carrying all R1/R2/R4 review fixes. Branched off PR-3 (`pr3/isaac-procedural` -> #46); needs PR-1 (#44) + PR-2 (#45) + PR-3 (#46) all in main before the rebase produces a clean diff and CI goes green. Files in this slice (excluding the foundation + procedural files that come in via PR-3's branch base): - `strands_robots_sim/isaac/simulation.py` (1,097 LOC): `IsaacSimulation(SimEngine)` with `__init__`, `is_available`, `create_world`, `destroy`, `reset`, `step`, `get_state`, `add_robot`, `add_object`, `get_observation`, `send_action`, `render`, `add_camera`, `replicate`, `_load_usd_robot`, `_load_urdf_robot`, `cleanup`, `__enter__/__exit__/__repr__`. Plus module-level `_get_or_create_simulation_app` (SimulationApp singleton) and `_RobotState` / `_CameraState` named-tuples. Lazy `omni` imports throughout; thread-safe via `RLock`. Carries: - R1's `is_available()` probing `omni.isaac.kit` specifically (commit `65ca6fe`) -- not the bare `omni` namespace, which is a PEP 420 shared package. - R1's no-`__del__` finalizer (commit `29ccfe3`); cleanup must be explicit via `cleanup()` or context manager. - R1's narrowed `except` clauses (commit `5309623`) at the four sites that previously swallowed bare `Exception`. - R4's distinguishable WARNING / DEBUG diagnostic logs across `get_observation`'s four silent-empty modes (commit `befb1e3`): pre-init probe DEBUG; ambiguous + unknown-name WARNING with typo echo + sorted known-robots; Phase 1 articulation-None DEBUG. - `strands_robots_sim/isaac/config.py` (116 LOC): Duplicated from PR-2 (#45) so this branch is locally testable. After PR-2 lands, this file becomes a no-change file in the rebase and the dup goes away. - `strands_robots_sim/isaac/tests/test_unit.py` (464 LOC, 6 test classes): `TestIsaacConfig` (R1/R2 carry-over), `TestIsaacSim ulationAvailability`, `TestIsaacSimulationContract` (R1's unknown-kwarg pin lives here), `TestProceduralRobots`, `TestNoEmojisInOutput`, `TestExceptionClauseHygiene` (AST walk pinning R1's narrowed exception clauses). - `strands_robots_sim/isaac/tests/test_entrypoint.py` (139 LOC): R6 entry-point registration tests + `IsaacSimulation` is a `SimEngine` subclass + `is_available()` returns `(bool, str|None)` tuple. - `strands_robots_sim/isaac/tests/test_get_observation_diagnostic_logs.py` (209 LOC, R4's pin file): 9 tests across 5 classes pinning the WARNING / DEBUG level discipline + content of the diagnostic log lines + the docstring contract enumerating the four silent- empty modes. - `strands_robots_sim/isaac/tests/test_gpu_integ.py` (117 LOC): GPU-only integ tests gated on `STRANDS_GPU_TEST=1` + the `gpu` pytest marker (declared in pyproject.toml from PR-1). CI signal: red in isolation -- `simulation.py` imports from `isaac.config` and `isaac.procedural`. Once PR-1/PR-2/PR-3 land in main and this branch rebases, expect 33-50 tests passing on CPU (the 25 tests that currently fail with `TypeError: Can't instantiate abstract class` are a separate pre-existing defect on Original work by @cagataycali in #31 (`413ff15..befb1e3`); this slice cherry-picks `simulation.py` + 4 test files. R1/R2/R4 review fixes are baked in. * fix(isaac): mirror real SimEngine abstract surface in fallback ABC cagataycali's review comment on simulation.py:50 (#47) flagged that the fallback ABC declared zero @AbstractMethod decorators, so IsaacSimulation() instantiated cleanly in tests when strands-robots was absent — masking the abstract-method gap that surfaces the moment the real ABC is in scope. Mirror the real strands_robots.simulation.base.SimEngine surface (14 abstract methods) in the fallback. Now `pytest` produces the same failure mode (25 failed / 33 passed) whether strands-robots is installed or not, including the dedicated canary test_isaac_implements_all_abstract_methods. The 4 missing concrete implementations on IsaacSimulation (list_robots, remove_robot, remove_object, robot_joint_names) remain a separately tracked gap per the PR body — this commit just makes the gap visible in CI. * fix(isaac): implement missing SimEngine abstract methods cagataycali's review at strands_robots_sim/isaac/simulation.py:774 flagged that IsaacSimulation didn't implement four abstract methods inherited from strands_robots.simulation.base.SimEngine — list_robots, robot_joint_names, remove_robot, remove_object — making the class non-instantiable with TypeError. Added concrete implementations using the existing _robots dict and _prim_registry list, all guarded by self._lock for thread safety. Removal methods only update the in-Python registry; USD prim deletion is deferred to world teardown (Phase 1 scope, matches add_robot's procedural / load-stub behaviour). Unblocks 24 previously-failing tests (now 73 passed, 5 skipped). Refs #49. * fix(isaac): use scalar gravity magnitude for Isaac Sim 5.1 set_gravity cagataycali's comment at strands_robots_sim/isaac/simulation.py:346 flagged that Isaac Sim 5.1 changed PhysicsContext.set_gravity to take a scalar magnitude rather than a vector. Passing a list/tuple now raises TypeError. Extract the Z-component from the configured gravity vector (convention: gravity points along -Z) and pass the scalar magnitude to set_gravity. Preserves the public list[float] API of create_world(gravity=...). * fix(isaac): type SimulationApp launch config kwargs cagataycali's review at strands_robots_sim/isaac/simulation.py:108 flagged that the **kwargs forwarded to omni.isaac.kit.SimulationApp were untyped. Add a SimulationAppLaunchConfig TypedDict (total=False) documenting the well-known Kit launch keys (renderer, width, height, physics_gpu, active_gpu, multi_gpu, sync_loads, hide_ui, anti_aliasing) and accept it as an optional launch_config parameter on _get_or_create_simulation_app. The **kwargs path is preserved as the escape hatch for Kit options not in the TypedDict; the explicit headless argument always wins over either source so caller intent is unambiguous. Tool exposure (the second half of the comment) is intentionally out-of-scope: _get_or_create_simulation_app is a process-singleton lifecycle helper and the agent-facing entry point is IsaacSimulation.create_world(), which already routes through this function. * refactor(isaac): centralise install hints in _install module cagataycali's comment at strands_robots_sim/isaac/simulation.py:272 flagged that the Isaac Sim install instructions (docker tag, Omniverse Launcher hint, Isaac Lab bootstrap) were duplicated across docstrings and error messages -- they would drift whenever the upstream image is bumped (security update or otherwise). Extract them into strands_robots_sim/isaac/_install.py as a single source of truth: - ISAAC_SIM_DOCKER_IMAGE / ISAAC_SIM_MIN_VERSION / ISAAC_LAB_BOOTSTRAP / PIP_EXTRA constants - not_importable_reason() composes the multi-line bullet block used by IsaacSimulation.is_available() - not_available_import_error() composes the inline ImportError raised by _get_or_create_simulation_app simulation.py now imports those helpers; the package docstring in isaac/__init__.py points readers at _install for the canonical values. A regression pin in TestInstallConstants asserts simulation.py contains no hardcoded `nvcr.io/nvidia/isaac-sim:` literal so re-hardcoding gets caught at CI time. PR: #47, comment: #47 (comment) * feat(isaac): include world-info json in create_world() success result cagataycali's review on PR #47 at strands_robots_sim/isaac/simulation.py:512 asked for the freshly-created sim env to also be surfaced as a 'json' content block alongside the existing 'text' summary, so an agent that just spun up a world can introspect device / dt / scene config without re-querying via get_state(). Following the existing convention in this file (get_state() at L624 and replicate() at L1238 already attach a 'json' payload to the same content block as the text), I add a 'world_info' dict next to the text in the create_world() success branch. Fields cover the snapshot the agent is most likely to need at startup: physics_dt, rendering_dt, gravity, ground_plane, stage_path, stage_units_in_meters, device, headless, render_mode, num_envs, num_envs_active, replicated, sim_time, step_count. The error branches stay text-only (an agent inspecting them just needs the message). Tests + lint unchanged. * feat(isaac): include destroy-info json in destroy() success result cagataycali's review on PR #47 at strands_robots_sim/isaac/simulation.py:534 ("Similar to get_state implementation below ^^") asked for the destroy() success branch to also surface a structured 'json' content block alongside the existing 'text' summary, matching the convention already established by get_state() (and recently extended to create_world() in 30a9487). Capture pre-teardown counts BEFORE the registry/state clears, then attach them as a 'destroy_info' dict on the same content block as the text: num_robots_released, num_cameras_released, num_prims_released, num_envs_released, sim_time_at_destroy, step_count_at_destroy, stage_path, and simulation_app_alive=True (the singleton survives destroy()). This is the snapshot an agent inspecting destroy()'s return value is most likely to need -- get_state() is no longer reachable after destroy() returns. The error branches stay text-only. Tests (78 passed, 5 skipped) and lint unchanged; the existing test_destroy_without_world / test_destroy_message _no_emoji assertions still hold. * lint(isaac): annotate intentional E402 on simulation.py late import Surfaces during PR #47 rebase onto current main: the late `from strands_robots_sim.isaac.config import IsaacConfig` import inside simulation.py is intentional — it must follow the SimEngine ABC fallback class definition above it (~line 95-130) which provides the import-time abstract surface mirror. Adding `# noqa: E402 # <reason>` keeps lint green without weakening the flake8 rule for real violations elsewhere. * test(isaac): mark test_add_procedural_robot xfail for Phase 1 cagataycali's L40S GPU validation comment on PR #47 confirmed that test_add_procedural_robot fails with len(obs)==0 because the procedural builder registers metadata but does not yet create USD prims / Articulation handle on the stage -- the documented Phase 1 limitation in simulation.py:1001. Mark the test xfail (strict=False) with a reason linking the expected code path so the gating signal stays meaningful: the other three GPU tests (create_world+step, render, replicate) still gate on PASS, while this one is allowed to fail until Phase 2's procedural USD prim builder lands. strict=False so the suite stays green if a future Phase 2 commit makes it pass before this xfail marker is removed. --------- Co-authored-by: yinsong1986 <yinsong1986@users.noreply.github.com>
yinsong1986
added a commit
that referenced
this pull request
May 28, 2026
…#51) * feat(isaac): URDF / MJCF / USD loaders → ProceduralRobot (closes #50) Follow-up to the R7 Phase 1 procedural-builder slice (PR #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 #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 #44 pattern). Failure semantics (closes the #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 #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 #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 (#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 #14 Stacks on top of pr3/isaac-procedural (PR #46) since it consumes the ProceduralRobot dataclass introduced there. * fix(isaac): make kinematic-tree guard fail-first, fix G1 topology cagataycali's comment at strands_robots_sim/isaac/tests/test_procedural_kinematic_guard.py:13 on PR #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. * review(isaac): address PR #51 inline comments — real-asset parity tests + docstring cleanup Closes the 3 inline review threads cagataycali opened on PR #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 #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 #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 a commit
that referenced
this pull request
May 28, 2026
…lit) (#48) * docs(isaac): backend reference + Phase 1 status banner (5/5 of #31 split) Part 5 / 5 of the split of #31 — tracked by #42. Branched off PR-1 (`pr1/isaac-foundation` -> #44); rebases cleanly onto `main` once PR-1 merges. Parallel-mergeable with PR-4 (#47, simulation) -- this slice has no code dependency on simulation.py. Adds the user-facing reference doc for the Isaac backend plus the companion test file that pins R2's "Phase 1 status" disclosure banner: - `docs/backends/isaac.md` (208 LOC): Install + Quick Start + API reference + environment variables + R2's Phase 1 status banner before the Installation section. The banner discloses that the Phase 1 skeleton silently no-ops the data plane (`add_robot` procedural branch, `_load_usd_robot`, `_load_urdf_robot`, `add_object`, `add_camera`, `replicate` all return `status: "success"` without instantiating the underlying USD prim or articulation handle, with the observable downstream effect that `get_observation()` returns `{}` and `render()` returns blank frames). R2 reviewer asked for this explicitly so a user following the Quick Start sees the disclaimer before the silent path. - `strands_robots_sim/isaac/tests/test_phase1_doc_banner.py` (~75 LOC, 3 tests): Carved out of cagataycali's original `test_phase1_doc_honesty.py` -- the `TestIsaacDocsPhase1Banner` class only. The companion `TestG1DOFCount` class lives in PR-3 (#46) alongside `procedural.py`. Pin tests: - `test_isaac_docs_file_exists`: truth-source pin for the doc file's location. - `test_phase1_banner_present_before_installation`: pins that "Phase 1 status" appears AND precedes the `## Installation` heading. - `test_phase1_banner_names_the_silent_methods`: pins that the banner enumerates `add_robot`, `replicate`, `get_observation` by name (so a future maintainer who reads only the banner knows which API surfaces are affected). CI signal: lint clean (black / isort / flake8); the 3 doc-banner pin tests pass standalone (`pytest test_phase1_doc_banner.py -v` -> 3 passed). Why split `test_phase1_doc_honesty.py` into G1-DOF (PR-3) + doc- banner (this PR): keeps each parallel-mergeable slice CI-green standalone. Shipping the original combined file in either PR would couple them (PR-3 would CI-red until PR-5 lands, or vice versa). The combined coverage equals the original file. Original work by @cagataycali in #31 (`413ff15..85e180f`); this slice cherry-picks `docs/backends/isaac.md` plus the carved-out banner test file. R2's banner addition (commit `85e180f`) is already baked into this doc. * docs(isaac): align with PR #47 + PR #51 merged to main Updates docs/backends/isaac.md to reflect the actual state of strands_robots_sim.isaac after PR #44 / #45 / #46 / #47 / #51 all landed. The Phase 1 status banner had become stale (claimed all data- plane methods were silent no-op when in fact the procedural builders work, the loaders module ships, and several SimEngine methods now have concrete implementations). Three substantive changes: 1. Phase 1 status banner — split into 'Working today' (config, is_available, lifecycle, procedural builders, loaders module) vs 'Still no-op' (add_object, add_camera, replicate, the per-class _load_usd_robot / _load_urdf_robot stubs, articulation-touching paths under get_observation / send_action / render). The loaders module is the working URDF / MJCF / USD ingestion path today; the IsaacSimulation private no-op stubs remain Phase 2 work. 2. New 'Loading External Description Files (URDF / MJCF / USD)' section between Procedural Robots and Comparison with Newton. Documents the loaders module (PR #51): three load_*() functions producing ProceduralRobot dataclass instances, shared failure semantics (FileNotFoundError / ValueError, never silent phantom robot), and the parity-test pin against the seven robosuite- bundled MJCFs the strands-robots LIBERO adapter consumes. 3. Procedural Robots — added the G1 intermediate-link-bodies explanation and the fail-first kinematic-tree guard contract (PR #51's commit). No env-var escape hatch. Plus housekeeping in the Architecture file-list and Testing section: - Architecture: added _install.py (PR #47's commit — centralised install hints), loaders.py (PR #51), and the actual test files now in main (test_get_observation_diagnostic_logs.py, test_procedural_g1_dof.py, test_procedural_kinematic_guard.py, test_loaders.py) instead of the placeholder list. - Testing: pytest invocations updated to cover the five no-GPU test files now shipped, plus a one-liner for running them all at once (the --ignore pattern that the hatch script uses). The doc was already accurate about the *direction* of Phase 1 vs Phase 2; the update just brings the surface lists in line with what actually shipped. Verification: - pytest strands_robots_sim/isaac/tests/test_phase1_doc_banner.py → 3 passed (the banner-content pin still matches; the rewrite keeps 'Phase 1 status' + the same enumerated method names). - diff stat: docs/backends/isaac.md, +51 / -7 LOC. This change lands inside PR #48 (5/5 of #31 split) since it's the canonical home for docs/backends/isaac.md updates. Top-level README and examples/MIGRATION.md drift (Stage-3-no-longer-future framing) is a separate doc-alignment concern and lands in a follow-up PR rather than widening this slice.
yinsong1986
added a commit
that referenced
this pull request
May 28, 2026
Top-level docs claimed Isaac would 'land in v0.3.0+' as future work, but PR #44 / #45 / #46 / #47 / #51 (R6 + R7 Phase 1) all merged to main on 2026-05-26. Updates the doc-side framing to reflect what's actually shipped vs what's still pending. README.md: 1. Status banner: 'Backend code lands in v0.3.0+ (Isaac)' → explicit Phase 1 / Phase 2 split. Names what works today (entry- point registration, IsaacConfig, lifecycle scaffolding, procedural builders, URDF / MJCF / USD loaders) vs what's still no-op (add_object, add_camera, replicate, per-IsaacSimulation _load_*_robot stubs). 2. Quick-start preamble: 'become copy-paste-runnable when R6/#13 and R7/#14 ship' → 'Isaac Sim Phase 1 has shipped — the snippet below is copy-paste-runnable today on a host with Isaac Sim 2024.x+ installed'. Newton snippets remain gated on R11/#18. 3. Isaac Sim quick-start snippet: fixed the kwarg name (rtx_mode='path_traced' → render_mode='rtx_pathtracing') so it matches the IsaacConfig field actually defined in strands_robots_sim/isaac/config.py. The previous spelling would TypeError at construction. Added a second snippet showing the loaders module (load_urdf / load_mjcf / load_usd) since that's the working URDF / MJCF / USD ingestion path today. 4. Status & roadmap section: - Stage 2 R5: [ ] → [x] (#26 merged). - Stage 3 R6: [ ] → [x] with link to #44. - Stage 3 R7: split into 'Phase 1 [x]' (linking #45 / #46 / #47 / #51) and 'Phase 2 [ ]' so the next-up scope is visible without dropping the historical #14 reference. examples/MIGRATION.md: 1. [isaac] extras line: 'Heavy GPU-only backends ship in later 0.x releases' / commented-out pip install → uncommented; comment notes 'Phase 1 shipped; data-plane in Phase 2'. Newton stays commented (Stage 4 pending). 2. 'After — same task on Isaac Sim' heading: 'Stage 3, future' → 'Stage 3, Phase 1 shipped'. Snippet kwarg fixed (rtx_mode → render_mode; same bug as the README). Added a 2-line comment noting evaluate_benchmark on a real LIBERO scene needs Phase 2 data-plane wiring (matches the disclosure banner on docs/backends/isaac.md, so the three doc surfaces stay in sync). examples/README.md is left as-is: its TBD rows reference the example files (libero/run_isaac.py etc.), which haven't shipped yet — those issue links (R8/#15, R23/#27) are still accurate forward-pointers. Verification: - diff stat: 2 files, +28 / -14 LOC. No code touched. - README + MIGRATION render cleanly under standard markdown parsers (verified by spot-checking the table / heading nesting). - The doc-banner test (test_phase1_doc_banner.py) is on the parallel pr5/isaac-docs branch (PR #48); the docstring banner on docs/backends/isaac.md was already updated there to the same framing — this PR brings README + MIGRATION in line. Cross-references: PR #36 (the previous examples/README.md drift fix) lands the same kind of post-merge alignment for #26's R5 work; this is its R7 Phase 1 sibling.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 3 / 5 of the split of #31 — tracked by
#42.Adds the procedural USD robot builders that
IsaacSimulation(PR-4) consumes. Branched off PR-1 (#44) — rebases cleanly ontomainonce PR-1 merges. Parallel-mergeable with PR-2 (#45) — neither imports the other.What's in this slice
strands_robots_sim/isaac/procedural.pyProceduralRobotnamed-tuple +get_procedural_robot(name)registry + 3 builders (SO-100, Franka Panda, Unitree G1)strands_robots_sim/isaac/tests/test_procedural_g1_dof.pytest_phase1_doc_honesty.py::TestG1DOFCount— pins the 21-DOF claim inprocedural.py's module + builder docstringsWhy split
test_phase1_doc_honesty.pyThe original test file mixed two concerns:
procedural.py. Belongs with the procedural code (this PR).docs/backends/isaac.md. Belongs with the docs (PR-5).Shipping both halves together would couple PR-3 to PR-5 (PR-3 would CI-red until PR-5 lands). Splitting keeps each PR CI-green standalone:
TestG1DOFCount(3 tests)tests/test_procedural_g1_dof.pyTestIsaacDocsPhase1Banner(3 tests)tests/test_phase1_doc_banner.pyThe combined coverage equals the original
test_phase1_doc_honesty.py. Each half passes against the file it actually inspects.R2 review-fix carry-over
R2 (commit
85e180fon #31) corrected the G1 DOF count: module docstring (line 8) and inline comment (line 165) had claimed 29-DOF; actual joint set is 21 (1 torso + 6 left leg + 6 right leg + 4 left arm + 4 right arm). Both sites now read 21-DOF inprocedural.py. The 3 pin tests intest_procedural_g1_dof.pyguard against the docstring drifting back out of sync.Phase-2-deferred defect (acknowledged, not pinned here)
The kinematic-tree topology in
_build_unitree_g1has duplicate(parent_body, child_body)edges (e.g.l_hip_rollandl_hip_pitchboth map bodies 3 → 4). Phase 2 fix needs intermediate massless link bodies. Documented inprocedural.py:165as deferred. Phase 1 does not instantiate the articulation, so the defect is dormant on this branch — same framing R2 captured.CI signal
hatch run lint(black --check/isort --check-only/flake8overstrands_robots_sim/+examples/) — clean.pytest strands_robots_sim/isaac/tests/test_procedural_g1_dof.py -v→ 3 passed.get_procedural_robot('unitree_g1').num_joints == 21) plus pin both docstring sites against the 29-DOF regression.Dependency chain
PR-1 (#44, foundation) → PR-3 (this), parallel with PR-2 (#45, config) → unblocks PR-4 (simulation) which imports
procedural.get_procedural_robotfor theadd_robot(procedural=True)path.Diff: 2 files, +331 / -0 LOC.
Original authorship
Original work by @cagataycali in #31 (
413ff15..85e180f). This slice cherry-picksprocedural.pyplus the carved-out G1-DOF test file. R2's DOF-count fix (commit85e180f) is already baked into the docstrings.