feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split)#46
feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split)#46yinsong1986 wants to merge 4 commits into
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.
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.
…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
|
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 |
| ) | ||
|
|
||
|
|
||
| def _build_so100() -> ProceduralRobot: |
There was a problem hiding this comment.
can we find a way to convert all robots-definitions into procedural robot instead of defining in the code?
There was a problem hiding this comment.
Good call — agree the long-term shape should be a generic loader (URDF / MJCF / USD → ProceduralRobot) rather than three hardcoded _build_* functions.
Treating it as out-of-scope for this PR though: PR #46 is the R7 Phase 1 procedural-builder slice (3/5 of #31), explicitly non-instantiating, and asset loaders are deferred to Phase 2 — wiring them in here would widen the slice past its 331-LOC budget, couple it to PR #47, and duplicate parser-design work that needs to happen on the Newton side too (see #33).
Filed as a follow-up: #50 — proposes strands_robots_sim/isaac/loaders.py covering all three formats, parity-tested against the procedural builders, gated under #14 (R7 epic) so it lands once the Phase 1 split closes. The current _build_* functions can stay as a zero-dep fallback so unit tests run without pxr / urdfpy / mujoco installed, and loaders layer on top.
Happy to fold any of the design knobs in #50 into PR #46 if you'd rather treat it as in-scope — let me know and I'll re-shape this PR. Otherwise leaving the thread open for you to close when you're satisfied with the follow-up framing.
There was a problem hiding this comment.
Update: #51 now actively implements #50 — converting the deferral framing above into a live review surface.
PR #51 (feat/isaac-loaders) ships:
| Loader | Backend | LOC (impl) | Tests | Notes |
|---|---|---|---|---|
load_urdf(path) |
stdlib xml.etree.ElementTree |
~145 | URDF round-trip + 7 error cases | mirrors the Newton-side parser semantics on PR #30 |
load_mjcf(path) |
stdlib XML walk | ~120 | round-trip + 4 error cases | handles LIBERO-style nested <body> |
load_usd(path) |
lazy pxr.Usd / pxr.UsdPhysics |
~80 | round-trip + import-guard | gated behind [isaac] extra |
Plus a parity test slice that builds an SO-100-shaped URDF and asserts loader output matches get_procedural_robot('so100') joint-count + joint-type distribution. The hardcoded _build_* functions in procedural.py stay as the zero-dep testable fallback (per #50's "Out of scope" recommendation), so unit tests can run on a clean environment with no pxr / urdfpy / mujoco installed.
CI on #51: 27 passed + 1 skipped (the skip is pxr-gated when the extra isn't installed).
If the implementation looks right when reviewed on #51, this thread can resolve and PR #46 stays at its 331-LOC Phase-1 budget. If the loader work should land in PR #46 instead of as a follow-up PR, let me know and I'll fold #51 into this branch (the dataclass shape is identical, so the merge is mechanical).
2 participants
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.