feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7)#31
feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7)#31cagataycali wants to merge 8 commits into
Conversation
Implements the Isaac Sim backend for strands-robots-sim, mirroring the Newton PR (strands-labs#30) structure exactly: - strands_robots_sim/isaac/__init__.py: PEP 562 lazy exports - strands_robots_sim/isaac/config.py: IsaacConfig dataclass with validation - strands_robots_sim/isaac/simulation.py: IsaacSimulation(SimEngine) with is_available(), create_world, add_robot, step, render, replicate - strands_robots_sim/isaac/procedural.py: SO-100, Panda, G1 builders - strands_robots_sim/isaac/tests/: 41 mocked unit + entrypoint tests - docs/backends/isaac.md: install + usage documentation - pyproject.toml: [isaac] extras + entry-points (isaac, isaac_sim) Key design decisions: - SimulationApp is a process-wide singleton (never create > 1) - All omni.* imports are lazy (PEP 562) -- zero CUDA overhead on import - headless=True by default (cloud/CI runners) - Thread-safe via threading.RLock - No emojis in user-facing strings - No binary assets in the PR - GPU tests gated on @pytest.mark.gpu + STRANDS_GPU_TEST=1 - Mock-only tests pass on CI without Isaac Sim installed Env vars: STRANDS_ISAAC_HEADLESS, STRANDS_ISAAC_RTX_PATHTRACING, STRANDS_ISAAC_NUCLEUS_URL Closes: cagataycali/strands-gtc-nvidia#334 (Phase 1)
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Phase-1 skeleton for the Isaac Sim backend: lazy-import package, IsaacConfig dataclass with env-var overrides, IsaacSimulation(SimEngine) with method stubs, procedural robot registry, entry-point registration, and 41 mocked tests. CI is green and the no-GPU path is honoured. The structure mirrors the Newton PR (#30) which is good for reviewability.
The scope is deliberately narrow (no real omni calls land), so most concerns below are about contracts that ship with this PR and that subsequent PRs will inherit, rather than about missing physics behavior.
What's good
- PEP 562 lazy attribute access in
isaac/__init__.pydoes cleanly avoid pullingomnionimport strands_robots_sim.isaac. SimulationAppsingleton is module-level +RLock-guarded; the comment explicitly calls out the process-wide constraint.- Per-method
with self._lock:is consistent acrossstep/send_action/get_observation/add_robot/destroy. IsaacConfig.__post_init__validates aggressively (render_mode, device prefix, num_envs, dts, camera dims) and the unit tests cover each branch.- No emojis in user-facing strings; the no-emoji invariant even has its own test class.
- Entry points declared for both
isaacandisaac_simaliases, with a fallback that grep-checkspyproject.tomlifimportlib.metadatalookup fails.
Concerns
is_available()under-checks: only verifies the bareomninamespace +torch.cuda.is_available().omniis a namespace package shared with non-Isaac Omniverse installs, and_get_or_create_simulation_appactually needsomni.isaac.kit.SimulationApp. A user withomni-ui(or a stubomni/__init__.pyleft over from a partial install) will getis_available() == Trueand then a hardImportErrorfromcreate_world(). Inline comment below.except Exception:appears 5 times insimulation.py(lines 332, 364, 754, 807, 1011). Per the repo conventions, this collapses everything fromKeyboardInterrupt-via-BaseExceptionto programming bugs into a logged warning and a generic error dict. Inline comment on the most consequential one (create_worldcleanup path).- Silently dropped kwargs at
simulation.py:169— any unknown keyword toIsaacSimulation(config, foo=...)is filtered out byif k in fieldswith no warning. A typo likeheadles=Falsebecomes a silent default. Inline comment below. __del__->cleanup()->destroy()->self._lock.acquire(): running arbitrary lock acquisition during interpreter teardown is a known Python anti-pattern (modules can beNonewhen__del__fires). The bareexcept Exceptionmasks the symptom but not the underlying race. Inline comment below.- GPU integ tests will not pass against the actual Phase-1 stub:
_load_usd_robot/_load_urdf_robotreturn[], procedural_RobotState.articulationis never populated, andreplicate()is a no-op flag flip.test_add_procedural_robotassertslen(obs) == 6andtest_replicate_fleetasserts"16" in result["content"][0]["text"]— only the second one will pass on a real Isaac box. That's fine for a skeleton PR, but the file should either be skipped wholesale or have its assertions weakened so a future GPU-CI run doesn't redirect attention to spurious failures. - Documentation drift:
docs/backends/isaac.mddocumentssim.add_robot("so100")andsim.render("front_cam")returning real RGB frames, with no "Phase 1: stub" disclaimer. Anyone reading the doc and pointing it at a real Isaac install will get blank arrays back fromrender()and an empty observation fromget_observation(). A one-line "Phase 1 ships scaffolding only; physics/render hooks land in Phase 2/3" banner at the top of the doc would prevent surprises. isaacextra is misleading:[isaac]declares onlyusd-core+warp-lang, butsimulation.pydoesimport numpy as npat module top.pip install strands-robots-sim[isaac]into a clean venv and thenfrom strands_robots_sim.isaac import IsaacSimulationwillImportErroron numpy. Either declare numpy in[isaac](or in core deps), or push the numpy import into the methods that need it.- Procedural registry lazy-build is unguarded (
procedural.pyend of file):_PROCEDURAL_REGISTRYis mutated fromget_procedural_robotwithout a lock. Two threads racing the first call will both seenot _PROCEDURAL_REGISTRYand both build. Idempotent, so not a bug today, but it contradicts the PR's "thread-safe via RLock on all mutable state" claim. add_cameraacceptstargetbut never uses it (line 880). Will silently no-op once Phase 3 lands unless threaded into the actual camera prim setup.
Verification suggestions
# Confirm the no-omni-on-import promise actually holds end-to-end
python -c "import sys; import strands_robots_sim.isaac; from strands_robots_sim.isaac import IsaacSimulation; assert not [m for m in sys.modules if m.startswith('omni')], sorted(m for m in sys.modules if m.startswith('omni'))"
# Confirm the [isaac] extra is self-sufficient
python -m venv /tmp/isaac-extra-test && /tmp/isaac-extra-test/bin/pip install -e '.[isaac]' && /tmp/isaac-extra-test/bin/python -c 'from strands_robots_sim.isaac import IsaacSimulation; IsaacSimulation()'
# Confirm entry-point discovery on installed package
pip install -e . && python -c "from importlib.metadata import entry_points; print([ep.value for ep in entry_points(group='strands_robots.backends')])"| (available, reason_if_not). If available is True, reason is None. | ||
| """ | ||
| try: | ||
| import omni # type: ignore[import-not-found] # noqa: F401 |
There was a problem hiding this comment.
is_available() only probes the bare omni namespace, but _get_or_create_simulation_app (line 92) actually needs omni.isaac.kit.SimulationApp. omni is a PEP 420 namespace package shared with omni.ui, omni.usd, partial Omniverse SDK installs, and Isaac-Lab pre-bootstrap states — any of those gives True here and then a hard ImportError from create_world(). Either probe what you actually need:
import importlib
importlib.import_module("omni.isaac.kit")or, less invasively, use importlib.util.find_spec("omni.isaac.kit"). Keeping the contract as (bool, str|None) while hardening the check would also let test_is_available_false_when_omni_missing actually exercise the negative path (right now it just asserts callable(...) and never validates the side-effect).
| import dataclasses | ||
|
|
||
| fields = {f.name for f in dataclasses.fields(config)} | ||
| overrides = {k: v for k, v in kwargs.items() if k in fields} |
There was a problem hiding this comment.
overrides = {k: v for k, v in kwargs.items() if k in fields} silently drops every unknown kwarg. IsaacSimulation(headles=False) (typo) or IsaacSimulation(num_env=1024) becomes a default-config sim with no warning — exactly the "silently dropped kwargs" anti-pattern called out in the repo conventions.
Suggest:
unknown = set(kwargs) - fields
if unknown:
raise TypeError(f"Unknown IsaacSimulation kwargs: {sorted(unknown)}")
overrides = {k: kwargs[k] for k in kwargs if k in fields}or at minimum a logger.warning listing the dropped names. Same applies to the config is None branch on line 164 — IsaacConfig(**kwargs) will raise on unknown kwargs (since dataclass), so the two branches diverge in strictness, which is itself a footgun.
| {"text": (f"Isaac Sim import failed: {e}. " "Ensure Isaac Sim is installed and accessible.")} | ||
| ], | ||
| } | ||
| except Exception as e: |
There was a problem hiding this comment.
Bare except Exception is forbidden by repo conventions (and by AGENTS.md > Review Learnings #86 — "Resource Cleanup on Partial Failure"). It also collapses real bugs (typos in omni.isaac.core.World kwargs, unexpected None returns) into a logged error dict that the caller can't distinguish from a recoverable failure.
Narrow to what World(...) / set_gravity / add_default_ground_plane / reset actually raise on Isaac (typically RuntimeError, ValueError, or Carb-specific exceptions), and let programming bugs propagate. The same pattern exists at lines 364, 754, 807, and 1011 — most importantly at 364 (destroy), where a stop/clear failure is logged at WARNING and then the world is marked destroyed anyway, hiding handle leaks.
| def __del__(self) -> None: | ||
| try: | ||
| self.cleanup() | ||
| except Exception: |
There was a problem hiding this comment.
__del__ -> cleanup() -> destroy() -> self._lock.acquire() is unsafe during interpreter shutdown: by the time __del__ fires, threading, logger, and np may have been replaced with None, and self._world.stop() can hit a partially torn-down omni module. The bare except Exception masks the symptom but invisible exceptions during finalization still print Exception ignored in: <function ...> warnings.
Drop the __del__ entirely and rely on the __exit__ / explicit destroy() path that callers already use in the docs. If you want a safety net, log a warning when __exit__ is bypassed (e.g. set a flag in create_world and check it in __del__, but do NOT acquire locks there).
| import time | ||
| from typing import Any | ||
|
|
||
| import numpy as np |
There was a problem hiding this comment.
numpy is a hard top-level import here, but the [isaac] extra in pyproject.toml only declares usd-core and warp-lang. A user who runs pip install 'strands-robots-sim[isaac]' into a clean venv (per the doc's quick-start) will hit ImportError: No module named 'numpy' on from strands_robots_sim.isaac import IsaacSimulation — and the lazy-import / PEP 562 promise in the package docstring ("zero omni overhead on import") is undermined by a hard numpy dependency anyway.
Fix one of: (a) add numpy to [project].dependencies (it's already needed transitively by usd-core, so be explicit), or (b) push the import numpy as np calls into the get_observation / send_action / render methods that actually use them. (a) is simpler.
There was a problem hiding this comment.
Verified false positive on the current branch: numpy>=1.21.0 is declared in [project].dependencies at pyproject.toml:36, so it is always installed regardless of which extras the user picks. pip install strands-robots-sim (no extras at all) already pulls numpy via the base dep set. The [isaac] extra layers only the Isaac-specific deps (usd-core, warp-lang) on top of that base.
$ pip install strands-robots-sim # no extras
$ python -c 'import numpy; print(numpy.__version__)'
2.4.6No code change needed for this concern; documenting here so the next reviewer cycle does not re-raise.
The other four concerns from this batch are addressed in R1 commits 65ca6fe / 32ef307 / 29ccfe3 / 5309623 -- see PR description §13 for the thread-to-SHA map and pin-test verification.
cagataycali
added
simulation
The bare ``omni`` namespace is a PEP 420 namespace package shared by
omni.ui, omni.usd, partial Omniverse SDK installs, and Isaac-Lab
pre-bootstrap states. Probing it with ``import omni`` reports True for
any of those, then create_world() fails seconds later with ImportError
on the actual missing dependency (omni.isaac.kit.SimulationApp).
Use importlib.util.find_spec against the specific submodule that
create_world() needs. Wrap in try/except ModuleNotFoundError to handle
the case where the parent ``omni`` package itself does not exist on
the import path (find_spec raises rather than returning None in that
case).
Pin test: test_is_available_probes_omni_isaac_kit_specifically asserts
find_spec("omni.isaac.kit") is called; fails on pre-fix code (verified
via git stash round-trip).
The explicit-config branch silently dropped any kwarg whose name was
not in the IsaacConfig field set. ``IsaacSimulation(IsaacConfig(...),
headles=False)`` (typo) ran with default-config rather than surfacing
the typo at construction time, which is the silently-dropped-kwargs
anti-pattern flagged in repo conventions.
Both construction branches now have symmetric strictness:
- ``config=None``: IsaacConfig dataclass init raises naturally on
unknown kwargs.
- ``config=<obj>``: explicit set-difference check raises TypeError
with the offending kwarg names and the known field set.
Pin tests: test_unknown_kwarg_raises_typeerror_{no_config,with_config}.
The with-config variant fails on pre-fix code (verified via git stash
round-trip); the no-config variant happens to already raise via
dataclass-init validation but is pinned for symmetry against future
refactors.
``__del__`` -> ``cleanup()`` -> ``destroy()`` -> ``self._lock.acquire()`` is unsafe at interpreter shutdown: by the time the finalizer fires, ``threading`` / ``logger`` / partially torn-down ``omni`` modules may leave the lock unacquirable, and the bare-except in the prior __del__ masked the symptom while still printing ``Exception ignored in:`` noise on stderr. GC-driven finalization is also semantically wrong for SimulationApp: the scheduler can defer the finalizer past process exit, leaking the World/USD stage. Callers must invoke ``cleanup()`` explicitly or use the class as a context manager (already supported via __enter__ / __exit__). Docstring on ``cleanup()`` updated to spell out the contract. Pin test: test_no_del_finalizer asserts ``__del__`` not in ``IsaacSimulation.__dict__`` (subclass-redefinition would re-introduce the hazard, so the assertion is on the class dict, not on dir()); fails on pre-fix code (verified via git stash round-trip).
Bare ``except Exception`` swallows programming bugs (AttributeError typos, KeyError on dict access, NameError) into logged error dicts that callers cannot distinguish from recoverable failures. Each site is narrowed to the realistic failure modes for the API it wraps: - create_world() partial-failure cleanup (was line 354): (RuntimeError, ValueError, OSError, AttributeError) covers Carb / sim init failures, gravity vector shape errors, USD/Nucleus IO, and omni surface drift across versions. - destroy() world cleanup (was line 386): (RuntimeError, OSError, AttributeError) covers stop/clear_instance on a partially-initialised or torn-down stage. - get_observation() joint-positions read (was line 776): (RuntimeError, ValueError, AttributeError, TypeError) covers the not-yet-initialised world path, torch tensor surface drift, and numpy coercion failures. - send_action() set_joint_position_targets (was line 829): (RuntimeError, ValueError, AttributeError) covers torn-down articulation handles and shape mismatches. Programming bugs (NameError, KeyError, ImportError-not-already-caught) now propagate. The destroy() WARNING-but-mark-destroyed flow is retained because the world is set to None unconditionally below. Pin test: TestExceptionClauseHygiene. test_no_bare_except_exception_in_simulation_module walks the AST and asserts no ``except Exception`` or bare ``except:`` remains in the module. Fails on pre-fix code listing all 4 offending lines (verified via git stash round-trip).
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Phase 1 skeleton for the Isaac Sim backend: IsaacSimulation(SimEngine), IsaacConfig, procedural robot definitions, entry-point registration, and a mocked unit-test suite. Imports stay zero-cost (PEP 562), is_available() returns a (bool, reason) tuple, the singleton SimulationApp constraint is respected, and the R1 commits land four narrowly-scoped fixes with paired regression pins (tightened is_available() probe, eager unknown-kwarg rejection, drop of the unsafe __del__, and AST-enforced narrow except clauses). The exception-clause hygiene test in particular is a nice durable guard.
What's good
- Lazy-import discipline holds:
import strands_robots_sim.isaacadds zeroomni.*modules tosys.modules(asserted intest_lazy_import_does_not_load_omni). SimEngineimport has a sensible fallback for the strands-robots < 0.4.0 case.- The R1 pin tests are real regression tests: each was verified to fail on pre-fix code via
git stashround-trip, per the description. is_available()now probesomni.isaac.kitspecifically rather than the bareomninamespace package — a real category of false positives that would have bitten Isaac-Lab pre-bootstrap users.
Concerns (summary-level)
- Phase-1 silent-success surface area is large. Several methods return
status: "success"but do nothing —add_robot()(procedural branch never builds the USD prim or articulation),_load_usd_robot/_load_urdf_robot(return[]),add_object,add_camera, andreplicate. On a real Isaac Sim install, the docs/quickstart sequence (create_world→add_robot("so100")→step(100)→get_observation) will return cheerful success strings and an empty observation with no error indication. Either gate these behindis_available()plus aNotImplementedError("Phase 2"), or make them no-op explicitly with astatus: "pending"marker. As-is, the GPU integration tests intest_gpu_integ.pywill pass on a real Isaac Sim install and validate nothing —assert len(obs) == 6will fail (obs is{}), but the failure mode is misleading. - API shape inconsistency in
get_observation. Returns{}on missing world / missing robot / missing name; every other method on the class returns{"status": "error", "content": [...]}. Callers can't distinguish "world not created" from "robot has no joints yet" from "typo in name". _load_usd_robotand_load_urdf_robotaccept untrusted file paths and pass them straight to a (future) loader. Worth adding a path-traversal note before Phase 2 wires up the real loader; the current stubs justlogger.infothe path so it's safe today, but the API contract should be locked down before someone wires upomni.isaac.urdfagainst it.- Procedural G1 kinematic graph has duplicate edges. Multiple joints share the same
(parent_body, child_body)pair on each leg (e.g.l_hip_rollandl_hip_pitchboth connect bodies 3→4). A child link can only have one parent joint in any realArticulationbuilder; this will need a redesign before Phase 2 attempts to instantiate it. The comment also says "29 DOF" while the code declares 21. __init__.pydescription mismatch. The PR description / docs claim G1 is "21-DOF humanoid (simplified)";procedural.pyline 165 comment says "29 DOF total for G1". Pick one.is_available()torch probe is slightly miscalibrated (simulation.py:244-250). Importingtorchto callcuda.is_available()requires torch — which isn't in[project.dependencies]or the[isaac]extra. On a host with Isaac Sim installed but a separate wrapper venv,is_available()will report "PyTorch not installed. Isaac Sim requires torch with CUDA support." even though Isaac's own bundled torch is fine. Consider a torch-free CUDA probe (ctypes.CDLL("libcuda.so.1")) or just trust that theomni.isaac.kitfind_spec already implies CUDA, since Isaac ships its own torch.
Verification suggestions
- On a machine with Isaac Sim installed, run the full
Quick Startfromdocs/backends/isaac.mdend-to-end. Expectation today: theadd_robotline returns success butget_observationreturns{}andrenderreturns blank frames — which matches the test_gpu_integ.py assertions only because they assert success status, not data. Worth either documenting Phase-1 limits indocs/backends/isaac.mdor addingpytest.skip("Phase 2")to the tests that exercise unimplemented behaviour. python -c "from strands_robots_sim.isaac import IsaacSimulation; print(IsaacSimulation.is_available())"on a CPU-only box should print a multi-line message includingomni.isaac.kit not importable— verifies the R1 fix.python -m pytest strands_robots_sim/isaac/tests/test_unit.py::TestExceptionClauseHygiene -v— the AST guard is the most valuable regression test in the suite; confirm it catches a deliberately-introducedexcept Exception(e.g., temporarily editsimulation.pyline 353 and re-run).
| ) | ||
| } | ||
| ], | ||
| } |
There was a problem hiding this comment.
Silent-success on the procedural branch. This block returns status: "success" with a confident message ("Robot 'so100' added (procedural: so100, 6 joints: ...)") but never actually builds a USD prim or attaches an Articulation handle — _RobotState.articulation stays None. Downstream this means:
get_observation(line 772) skips thearticulation.get_joint_positions()block and returns{}.send_action(line 838) silently no-ops the joint targets.test_gpu_integ.py::test_add_procedural_robotassertslen(obs) == 6— that will fail on a real Isaac Sim install.
Three ways to resolve, in increasing fidelity:
- Raise
NotImplementedError("procedural builder lands in Phase 2")and skip the GPU integ test until then. - Return
status: "pending"with a message that the prim wasn't built. - Actually wire up the USD prim build (probably out of scope for this PR).
The Phase-1 scope is fine — but the success status is misleading users who follow the docs' Quick Start. At minimum, please add a note to docs/backends/isaac.md Quick Start that the Phase-1 skeleton silently no-ops the data plane.
There was a problem hiding this comment.
Documentation slice landed in R2 (85e180f): docs/backends/isaac.md now carries a Phase 1 status banner between the Overview and Installation sections that enumerates add_robot (procedural branch), _load_usd_robot, _load_urdf_robot, add_object, add_camera, and replicate as silent-success surfaces, and names the observable downstream effect (get_observation() returns {}, render() returns blank frames). The disclosure precedes the Quick Start so users see it before following the documented call sequence.
The behavioural fix (option 1 NotImplementedError vs option 2 status: "pending" vs option 3 wire up the prim build) is a Phase 2 design call -- the GPU integ test currently asserts success, so any of (1) or (2) breaks that test as you noted. Option 3 is out-of-scope for a Phase 1 skeleton PR. Carved out per PR description S13 R2 carve-out table; the disposition lands with Phase 2 (or earlier with maintainer direction on which option to take).
Autonomous agent response. Strands Agents. Feedback to @cagataycali.
| # bugs propagate. | ||
| logger.debug("Failed to get joint positions: %s", e) | ||
|
|
||
| return obs |
There was a problem hiding this comment.
Inconsistent API contract. Every other public method on this class returns {"status": "error"|"success", "content": [{"text": ...}]}. get_observation instead returns a bare {} for four distinct failure modes (world not created, no robots, ambiguous robot, unknown robot name). Callers can't tell "empty observation because the articulation isn't initialized yet" apart from "typo in robot_name" apart from "forgot create_world()". Suggest either matching the rest of the API, or at least logging at WARNING for the typo / unknown-name case so silent failures show up in logs.
There was a problem hiding this comment.
Closed in R4 (befb1e3) via the at-minimum path -- WARNING-level log lines for the diagnostically-meaningful operator-attention modes, while preserving the dict[str, Any] return contract that downstream callers (RL loops, teleop receive, observation buffers) consume by joint-name keying.
Level discipline (recorded in the docstring + pinned by negative-assertion tests so future contributors can't over-correct):
world not yet created-> DEBUG (feature-detect probe surface; WARNING here would create alert noise on every probe beforecreate_world()).ambiguous(multiple robots, no name) -> WARNING with sorted known-robot list so operators can disambiguate from the log.unknown robot_name-> WARNING with the typo value echoed AND the sorted known-robot list (grep -F 'rob0t0'finds the offending callsite).Articulation handle is None-> DEBUG via the inner narrow except (Phase 1 stub state is documented; WARNING here would fire on every observation tick).
Not taking the other path ("matching the rest of the API" with {"status": ..., "content": ...}) deliberately -- that would be a breaking change to the joint-positions-keyed-by-name contract that all downstream consumers rely on. The shape decision is an API-redesign discussion that belongs in a Phase 2 PR, not a Phase 1 skeleton's diagnostic-log fix.
Pin: tests/test_get_observation_diagnostic_logs.py (9 tests across 5 classes). git stash of simulation.py -> 7/9 fail (no log lines emitted; docstring missing the four-mode enumeration); restore -> 9/9 pass. The 2 tests that pass on pre-fix are the negative-assertion guards (no spurious WARNING during pre-init probe; no spurious WARNING during the documented Phase 1 articulation-None observation path) -- they pin the level discipline against future over-correction.
Full verification table in PR description §13 R4. Round budget after this commit: 3/3 substantive rounds used (R1, R2, R4); R3 was a non-substantive merge commit. The two outstanding R2 carve-outs (simulation.py:627 Phase 2 behavioural and simulation.py:1027 Phase 2 path-traversal) belong with the Phase 2 loader implementation.
Autonomous agent response. Strands Agents. Feedback to @cagataycali.
| JointDef(name="l_hip_pitch", parent_body=3, child_body=4, axis=(0, 1, 0), limit_lower=-1.5, limit_upper=0.5), | ||
| JointDef(name="l_knee", parent_body=4, child_body=5, axis=(0, 1, 0), limit_lower=-0.1, limit_upper=2.5), | ||
| JointDef(name="l_ankle_pitch", parent_body=5, child_body=6, axis=(0, 1, 0), limit_lower=-0.8, limit_upper=0.5), | ||
| JointDef(name="l_ankle_roll", parent_body=5, child_body=6, axis=(1, 0, 0), limit_lower=-0.3, limit_upper=0.3), |
There was a problem hiding this comment.
Invalid kinematic tree topology. Multiple joints share the same (parent_body, child_body) pair on each leg — l_hip_yaw (line 170), l_hip_roll (line 171), and the next two all connect adjacent body pairs in ways where the same child has two parent joints. Same pattern on the right leg and arms. Any real Articulation builder (USD PhysicsRevoluteJoint, MuJoCo, Newton/Warp) requires a tree: each non-root link has exactly one inbound joint.
This will land as a Phase 2 problem when an actual _load_procedural_robot tries to instantiate the USD prim chain. Two paths forward:
- Add intermediate massless link bodies (the standard urdf trick:
l_hip_yaw_link→l_hip_roll_link→l_hip_pitch_link) so each joint has its own dedicated parent/child pair. - Reduce per-leg DOF to something the tree can actually express.
Also: line 165 comment claims "29 DOF total for G1" but the actual joint count is 21 (1 + 6 + 6 + 4 + 4), which matches what __init__.py advertises. Fix the comment for now even if you defer the topology rework.
| # In full implementation: use omni.isaac.core to add USD reference | ||
| # and extract Articulation joint names | ||
| logger.info("Loading USD robot from %s at %s", usd_path, prim_path) | ||
| return [] |
There was a problem hiding this comment.
_load_usd_robot and _load_urdf_robot (line 1029) accept arbitrary user-supplied paths and currently just return []. Today this is harmless. Before Phase 2 wires up omni.isaac.urdf to consume urdf_path, please:
- Validate the path resolves under a configured asset root (or starts with
omniverse://nucleus_url), not arbitrary local FS. - Reject
..segments afterPath(p).resolve(). - Document explicitly in the docstring whether the loader follows symlinks and whether Nucleus URLs are honored.
Mentioning now because the public add_robot(urdf_path=...) signature is being established here and changing the contract later (e.g., adding a required assets_root parameter) is a breaking change. The current stub is the cheap moment to lock down the path-handling contract.
There was a problem hiding this comment.
Agreed on the contract-lockdown framing. The add_robot(urdf_path=...) signature is being established here, and adding a required assets_root parameter post-Phase-2 is a breaking change. Three validation requirements preserved verbatim for the Phase 2 commit:
- Path resolves under a configured asset root (or starts with
omniverse://nucleus_url) -- not arbitrary local FS. - Reject
..segments afterPath(p).resolve(). - Docstring documents whether the loader follows symlinks and whether Nucleus URLs are honored.
Carved out per PR description S13 R2 carve-out table -- pre-Phase-2 lockdown lands with the loader implementation rather than as orphan validation against a stub that still returns []. The validation belongs adjacent to the consumption site so future readers do not have to cross-reference two files to reason about the path-handling contract.
Autonomous agent response. Strands Agents. Feedback to @cagataycali.
R2 review batch surfaced two documentation-honesty drifts that were
addressable as a single surgical commit without changing the Phase 1
behavioural surface (per the round-budget rule, ONE concern -> ONE commit;
the other R2 concerns will land in subsequent commits or carve-outs).
1. G1 DOF count drift (procedural.py)
The module docstring at line 8 and the inline comment at line 165
both claimed "29 DOF" / "29-DOF" for the Unitree G1 procedural
robot. The actual joint set in `_build_unitree_g1` is 21 (1 torso +
6 left leg + 6 right leg + 4 left arm + 4 right arm). The
`__init__.py` package docstring and `docs/backends/isaac.md` already
advertise 21-DOF; only `procedural.py` was stale.
Both sites now read 21-DOF. A note adjacent to the inline comment
acknowledges the duplicate-edge defect on the kinematic graph
(e.g. l_hip_roll and l_hip_pitch both connect bodies 3 -> 4) as a
Phase 2 fix that needs intermediate massless link bodies before
any real USD/MuJoCo articulation builder can instantiate the chain.
Phase 1 does not instantiate the articulation, so the defect is
dormant on this branch.
2. Phase 1 data-plane disclosure missing from docs
Reviewer (R2 thread on simulation.py:627): "At minimum, please add
a note to docs/backends/isaac.md Quick Start that the Phase-1
skeleton silently no-ops the data plane."
`docs/backends/isaac.md` now carries a "Phase 1 status" banner
between the Overview section and the Installation section. The
banner enumerates the affected methods (`add_robot` on the
procedural branch, `_load_usd_robot`, `_load_urdf_robot`,
`add_object`, `add_camera`, `replicate`), names the observable
downstream effect (`get_observation()` returns `{}`, `render()`
returns blank frames -- no exception raised), and frames the
surface as an integration contract rather than a working physics
path. This precedes the Quick Start so a user sees the disclosure
before running the documented call sequence on a real Isaac Sim
install.
Pin: tests/test_phase1_doc_honesty.py (6 assertions)
* test_g1_actual_joint_count_is_21 -- truth-source pin:
the actual joint set
* test_g1_module_docstring_advertises_21_not_29
* test_g1_builder_docstring_advertises_21_not_29
* test_isaac_docs_file_exists
* test_phase1_banner_present_before_installation
* test_phase1_banner_names_the_silent_methods
Pre-fix verification (per AGENTS.md S0 mandatory pin-on-pre-fix
round-trip): with the procedural.py + docs/isaac.md edits stashed and
only the new pin file present, 4 of 6 fail (both module-docstring and
builder-docstring assertions on procedural.py; both banner-presence
and banner-names-methods assertions on the docs page). With the fix
applied, 6 of 6 pass. The two pin tests that pass on pre-fix code
(`test_g1_actual_joint_count_is_21` and
`test_isaac_docs_file_exists`) are intentional truth-source guards
that pin invariants the documentation is supposed to track.
Post-fix Isaac suite: 52 passed (was 46 + 6 new = 52). black, isort,
flake8 clean on the touched files. No emojis in user-facing strings.
Out of scope for this commit (per AGENTS.md mid-review-commit shape,
ONE concern -> ONE commit, plain `git push`, no force):
* Silent-success surface on `add_robot` procedural branch (R2 thread
on simulation.py:627). Documented in the new Phase 1 banner;
the behavioural fix (raise NotImplementedError vs status="pending"
vs wire up the prim build) is a Phase 2 design call that touches
the GPU integ test assertions and is not surgical in this PR.
* `get_observation` returning bare `{}` for 4 distinct failure modes
(R2 thread on simulation.py:791). API-shape change; warrants its
own commit.
* Path-traversal hardening on `_load_usd_robot` / `_load_urdf_robot`
(R2 thread on simulation.py:1027). Pre-Phase-2 lockdown; lands
when the loader actually consumes the path.
* Topology rework on the G1 kinematic graph (intermediate link
bodies). Phase 2 work; the duplicate-edge defect is acknowledged
in the inline NOTE block adjacent to the joint definitions.
…nflict
Conflict was a single line in [tool.hatch.envs.default.scripts]:
- main (placeholder): `test = "python -c 'import strands_robots_sim; ...'"`
with comment 'No tests until backend code lands in R7 (Isaac) / R11 (Newton).'
- this PR (R7): `test = "pytest strands_robots_sim/isaac/tests/ -v ..."`
Resolution keeps the pytest command since this PR is exactly the R7 milestone
the placeholder was waiting for. Main's informative comment about the
`examples/` lint scope is preserved (rephrased + retained alongside the new
test-script line) so future readers still see why lint covers `examples/`.
Main's lint/format scope extension to `examples/` was non-conflicting and is
preserved automatically. No code changes outside pyproject.toml.
Pin: `hatch run test` now resolves to pytest on the isaac/tests/ tree;
`uv run pytest strands_robots_sim/isaac/tests/ -v --ignore=...test_gpu_integ.py`
already passes locally as of HEAD.
…on silent-empty modes
The Phase 1 get_observation returned a bare dict[str, Any] and an empty
dict was indistinguishable across four diagnostically-different conditions:
1. world not yet created
2. robot_name=None with multiple robots present (ambiguous)
3. robot_name not in registered robots (typo / not-yet-added)
4. robot present but Articulation handle not yet initialised
The reviewer's at-minimum recommendation was to log at WARNING for the
typo / unknown-name case so silent failures show up in operational logs.
This commit takes that path while preserving the dict[str, Any] return
contract (callers consume joint positions keyed by name; switching to
the {status, content} mutating-method shape is a breaking change that
belongs to a Phase 2 API-redesign discussion, not a documentation /
log-surface fix).
Level discipline:
* world not yet created -> DEBUG. Many callers feature-detect by
probing before create_world(); WARNING here would create alert
noise on every probe.
* ambiguous (multiple robots, no name) -> WARNING with sorted
known-robot list so operators can disambiguate from the log.
* unknown robot_name -> WARNING with the typo value echoed and the
sorted known-robot list.
* Articulation handle is None -> still DEBUG via the inner narrow
except (Phase 1 stub state, documented; WARNING here would fire
on every observation tick).
Docstring now enumerates all four silent-empty modes and records why
the return shape is preserved, so a future contributor doesn't
'fix' the dict shape and break the joint-positions-keyed-by-name
contract.
Pin: tests/test_get_observation_diagnostic_logs.py (9 tests).
Pre-fix verify: 7/9 fail on the prior commit (no log lines emitted +
docstring missing the four-mode enumeration); restore -> 9/9 pass.
The 2 tests that pass on pre-fix are intentional negative-assertion
guards (no spurious WARNING during pre-init probe; no spurious WARNING
during the documented Phase 1 articulation-None observation path) --
they pin the level discipline against future over-correction.
Test results: 61 passed (was 52 at R2; +9 net new pin tests).
black, isort, flake8: clean on touched files.
AGENTS.md non-ASCII sweep: clean (no emojis in code / docstrings /
log strings).
|
@cagataycali — proposing a 5-way split of this PR following the precedent you set on TL;DR
The bulk slice (4/5) is Re-using your Happy to help with the actual rebase / branch shuffle if useful — but per the #195 precedent the cleanest path is for you to drive it on your fork. Let me know if you want this shape adjusted (more / fewer slices, different cuts) before you start. |
|
Filed the 5-way split per
The closing condition on #42 is "PR-5 lands → close #31 with redirect comment". This PR stays open until the chain completes; review threads here can carry over to whichever child PR consumes the relevant code (mapped in #42 under "Review-thread bookkeeping"). Pre-existing defect surfaced by the split (not introduced by it): |
#31 split) (#44) * feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) Part 1 / 5 of the split of #31 — tracked by #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 #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. * review(isaac): port 10 entry-point + lazy-import tests from #43 (PR #44 R1) Closes @cagataycali's R1 review on #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 #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 #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 #43 branch specifically for the PR-1 surface; cleanly self-contained against this slice.
…#45) * feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) Part 1 / 5 of the split of #31 — tracked by #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 #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. * review(isaac): port 10 entry-point + lazy-import tests from #43 (PR #44 R1) Closes @cagataycali's R1 review on #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 #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 #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 #43 branch specifically for the PR-1 surface; cleanly self-contained against this slice. * feat(isaac): IsaacConfig dataclass with validation (2/5 of #31 split) Part 2 / 5 of the split of #31 — tracked by #42. Branched off PR-1 (`pr1/isaac-foundation` -> #44); rebases cleanly to `main` once PR-1 merges. Adds the configuration dataclass that `IsaacSimulation` (PR-4) consumes: - `strands_robots_sim/isaac/config.py` (116 LOC): `IsaacConfig` dataclass with: - `num_envs`, `device` (default `"cuda:0"`), `headless` (default `True` -- safe for cloud/CI runners), `render_mode`, `physics_dt`, `gravity`, `ground_plane`, `camera_width`, `camera_height`, `rtx_pathtracing`, `nucleus_url`. - environment-variable overrides via `STRANDS_ISAAC_*` (`HEADLESS`, `RTX_PATHTRACING`, `NUCLEUS_URL`). - dataclass-level validation in `__post_init__` so e.g. `IsaacConfig(num_envs=0)` raises `ValueError` at construction. - `from_kwargs` classmethod that PR-4's `IsaacSimulation.__init__` uses to validate caller-supplied kwargs and surface unknown keys as `TypeError` (the R1 "reject unknown kwargs" pin). No `omni` / Isaac Sim imports. Pure-Python dataclass; can be imported on any host without a GPU. Tests for this module ship in PR-4 alongside `test_unit.py::TestIsaacConfig` (cleaner than carving a test slice for ~120 LOC of dataclass). CI signal: lint clean (black / isort / flake8); the lazy `__init__.py` from PR-1 now resolves `IsaacConfig` to a real class when accessed -- the `from strands_robots_sim.isaac import IsaacConfig` import path becomes live in this slice. Original work by @cagataycali in #31 (`413ff15`); this slice cherry-picks just `config.py`. R1's `from_kwargs` validation pin (commit `32ef307`) is already baked into this file. * fix(isaac): address review comment on PR #45 cagataycali's comment at strands_robots_sim/isaac/config.py:116 flagged that from_kwargs was promised by the PR description but missing from the slice. Added the classmethod so PR-4 (#47) can DRY its unknown-kwarg validation against IsaacConfig.from_kwargs rather than inlining dataclasses.fields() reflection at simulation.py:174-180.
…split) (#46) * feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) Part 1 / 5 of the split of #31 — tracked by #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 #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. * review(isaac): port 10 entry-point + lazy-import tests from #43 (PR #44 R1) Closes @cagataycali's R1 review on #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 #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 #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 #43 branch specifically for the PR-1 surface; cleanly self-contained against this slice. * feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) Part 3 / 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. 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 #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. * fix(isaac): add Phase-1 fail-fast guard for duplicate kinematic edges 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.
3 participants
Summary
Adds the Isaac Sim GPU simulation backend for
strands-robots-sim, mirroring the exact structure of the Newton PR (#30).This is Phase 1 (skeleton + entry-point + tests) -- no GPU required for CI. The full implementation (stages, sensors, replicator, fleet) follows in subsequent PRs.
What's Included
isaac/__init__.pyomnioverhead on import)isaac/config.pyIsaacConfigdataclass with validationisaac/simulation.pyIsaacSimulation(SimEngine)-- main backend classisaac/procedural.pyisaac/tests/test_unit.pyisaac/tests/test_entrypoint.pyisaac/tests/test_gpu_integ.pySTRANDS_GPU_TEST=1)docs/backends/isaac.mdpyproject.toml[isaac]extras + entry-pointsTotal: 11 files, ~2.5K LOC, 61 tests passing (R4)
Key Design Decisions
import strands_robots_simhas zero CUDA overheadis_available()classmethod -- returns(bool, str|None)tuple with precise error messageheadless=Trueby default -- safe for cloud/CI runnersthreading.RLockon all mutable state@pytest.mark.gpu+STRANDS_GPU_TEST=1Entry Points
Environment Variables
STRANDS_ISAAC_HEADLESSSTRANDS_ISAAC_RTX_PATHTRACINGSTRANDS_ISAAC_NUCLEUS_URLTest Results (CI)
All tests pass without Isaac Sim installed (fully mocked).
black --check,isort --check-only,flake8all clean.Acceptance Criteria (Phase 1)
from strands_robots_sim.isaac import IsaacSimulationworksIsaacSimulation.is_available()returns friendly error on CIimportlib.metadataIsaacSimulationsubclassesSimEngineomnimodules loaded on importNext Steps (Future PRs)
omni.isaac.cloner.Cloner§13 Round-N changelog
R1 -- 4 surgical commits closing review feedback (
413ff15..5309623)Each commit closes one previously-flagged concern with a pin test that fails on pre-fix code (verified via
git stashround-trip). Plaingit push, no force, no amend on already-published commits.65ca6feis_available()probed bareomninamespace -- a PEP 420 namespace package shared byomni.ui/omni.usd/ partial Omniverse installs / Isaac-Lab pre-bootstrap. Probe gave(True, None)thencreate_world()raised ImportError seconds later.test_is_available_probes_omni_isaac_kit_specifically(uses monkeypatch onimportlib.util.find_specto capture call args; asserts the specific submodule string is queried)git stash-> 1 fail (captured == []); restore -> pass32ef307config=<obj>branch (set(kwargs) & fieldsfilter).IsaacSimulation(IsaacConfig(num_envs=4), num_env=8)ran with default-config rather than surfacing the typo.test_unknown_kwarg_raises_typeerror_{no_config,with_config}(the with-config variant is the actual silent-drop pin; no-config happens to already raise via dataclass init -- pinned for symmetry)git stash-> 1 fail (DID NOT RAISE TypeError); restore -> 2/2 pass29ccfe3__del__->cleanup()->destroy()->self._lock.acquire()is unsafe at interpreter shutdown. Bare-except masked the symptom but still printedException ignored in:noise. GC-defer also leaks the World/USD stage past SimulationApp shutdown.test_no_del_finalizerasserts__del__not inIsaacSimulation.__dict__(subclass would re-introduce, so check is on class dict notdir()).cleanup()docstring spells out the explicit-cleanup contract.git stash-> 1 fail (__del__present in class dict); restore -> pass5309623except Exceptionswallowed programming bugs. Narrowed to(RuntimeError, ValueError, OSError, AttributeError, TypeError)(per site, only the ones the wrapped API can realistically raise). Programming bugs (NameError, KeyError) now propagate.TestExceptionClauseHygiene.test_no_bare_except_exception_in_simulation_modulewalks the module AST and asserts noexcept Exceptionor bareexcept:remains.git stash-> 1 fail listing all 4 offending lines; restore -> passR1 verification
pytest strands_robots_sim/isaac/tests/ --ignore=test_gpu_integ.pyblack --check strands_robots_simisort --check-only strands_robots_simflake8 strands_robots_simgit stashround-trip on eachReviewer concern not addressed in R1 (with rationale, not deferred)
simulation.py:32(numpynot in[isaac]extra)pip install 'strands-robots-sim[isaac]'into a clean venv will hitImportError: No module named 'numpy'"numpy>=1.21.0is declared in[project].dependencies(pyproject.toml:36), so it is always installed regardless of which extras the user picks.pip install strands-robots-sim(no extras) already pulls numpy. The[isaac]extra layers only the Isaac-specific deps (usd-core,warp-lang) on top. Replied inline on the thread with the line reference.R2 -- 1 surgical commit closing the addressable-without-API-change subset (
85e180f)R2 review batch (2026-05-25T06:52:52Z) raised 4 substantive concerns. ONE concern -> ONE commit per the AGENTS.md mid-review-commit shape; this commit closes the documentation-honesty slice that does not require an API or behavioural change. The remaining 3 concerns are addressed in the carve-out table below.
85e180fprocedural.pymodule docstring (line 8) and inline comment (line 165) claimed 29-DOF; the actual joint set in_build_unitree_g1is 21 (1 torso + 6 left leg + 6 right leg + 4 left arm + 4 right arm).__init__.pyanddocs/backends/isaac.mdalready advertised 21-DOF; onlyprocedural.pywas stale. Both sites now read 21-DOF. The kinematic-graph duplicate-edge defect (l_hip_rollandl_hip_pitchboth map bodies 3 -> 4) is acknowledged in an inline NOTE block as Phase-2 work; Phase 1 does not instantiate the articulation, so the defect is dormant on this branch. (b) Phase 1 data-plane disclosure:docs/backends/isaac.mdnow carries a "Phase 1 status" banner between Overview and Installation that enumerates the silent-success methods (add_robotprocedural branch,_load_usd_robot,_load_urdf_robot,add_object,add_camera,replicate), names the observable downstream effect (get_observation()returns{},render()returns blank frames), and frames the surface as an integration contract -- precedes the Quick Start so the disclosure happens before the user follows the documented call sequence.tests/test_phase1_doc_honesty.py(6 assertions): truth-source pin (g1.num_joints == 21), procedural.py module-docstring + builder-docstring DOF assertions, banner-presence assertion ordered before Installation, banner-enumerates-methods assertion.git stashof the procedural.py + docs/isaac.md fixes -> 4/6 fail; restore -> 6/6 pass. Two pin tests (test_g1_actual_joint_count_is_21,test_isaac_docs_file_exists) intentionally pass on pre-fix code as truth-source guards against future regressions.R2 carve-out -- concerns deferred with rationale
simulation.py:627-- silent-success on proceduraladd_robotstatus: "success"but never builds the USD prim or articulation handle. Three fix shapes proposed by reviewer: (1)NotImplementedError("Phase 2"), (2)status: "pending", (3) actually wire up the prim build.success(reviewer acknowledges this). Choosing (3) is out-of-scope for a Phase 1 PR. The documentation slice (R2 (b) above) is the surgical fix that lands now; the behavioural change is a Phase 2 design decision that needs a maintainer call on which option to take. Documentation disclosure now in place so users following the docs see the disclaimer before the silent path.simulation.py:791--get_observationreturns bare{}{}with no log line. Inconsistent with the{"status": ...}shape of every other public method on this class.befb1e3).simulation.py:1027-- path-traversal hardening on_load_usd_robot/_load_urdf_robotR3 -- merge commit to resolve
pyproject.tomlconflict against main (cbd4355)Main advanced past this branch's merge-base while review was in flight (latest main:
4d82bcf, this branch's prior tip:85e180f). The single conflict was in[tool.hatch.envs.default.scripts]: main introduced a placeholdertest = "python -c 'import strands_robots_sim; ...'"with comment "No tests until backend code lands in R7 (Isaac) / R11 (Newton)." This PR is exactly the R7 milestone the placeholder was waiting for, so the resolution keeps the pytest command (test = "pytest strands_robots_sim/isaac/tests/ -v --ignore=...test_gpu_integ.py") and folds main's informational comment about theexamples/lint-scope extension above it.Resolved as a merge commit (not a rebase) deliberately: rebasing would rewrite all R1/R2 commit SHAs and orphan the inline review threads anchored to those SHAs (
65ca6fe,32ef307,29ccfe3,5309623,85e180f). Plaingit push, no force. Mergeability is nowMERGEABLE(wasCONFLICTING).python3 -c "import tomllib; tomllib.loads(open('pyproject.toml','rb').read().decode())"pyproject.tomlMERGEABLE(wasCONFLICTING)No behavioural change. The pytest command line is the same one that already ran in R2 verification (52 passed).
R4 -- 1 surgical commit closing the
get_observationsilent-empty diagnostic gap (befb1e3)The last remaining R2 carve-out (the
get_observationsilent-{}API surface). The reviewer offered two options: (a) match the rest of the API by switching to{"status": ..., "content": ...}(a breaking change to the joint-positions-keyed-by-name contract that all callers consume), or (b) at minimum log at WARNING for the typo / unknown-name case so silent failures show up in operational logs. R4 takes path (b); path (a) is a Phase-2 API redesign discussion that does not belong in a Phase 1 skeleton PR.befb1e3get_observationreturned bare{}across four diagnostically-different conditions (world not created / ambiguous robot_name / unknown robot_name / Articulation None) with no log line. Operators could not distinguish a typo from a forgottencreate_world()from a Phase 1 stub state.tests/test_get_observation_diagnostic_logs.py(9 tests across 5 classes): DEBUG-level pin for the pre-init probe, WARNING pins for ambiguous + unknown-name (with content assertions on known-set surfacing + typo-echo), no-spurious-WARNING pin for the documented Phase 1 articulation-None path, and a docstring-contract pin enumerating all four silent-empty modes so future contributors can't "fix" the dict shape without first resolving this conversation.git stashofsimulation.py-> 7/9 fail (no log lines emitted; docstring missing the four-mode enumeration); restore -> 9/9 pass. The 2 tests that pass on pre-fix are the negative-assertion guards (no spurious WARNING during pre-init probe; no spurious WARNING during the documented Phase 1 articulation-None observation path) -- they pin level discipline against future over-correction.Level discipline rationale (recorded in the docstring + pinned by the negative-assertion tests):
world not yet created-> DEBUG. Many callers feature-detect by probing beforecreate_world(); WARNING here would create alert noise on every probe.ambiguous(multiple robots, no name) -> WARNING, with sorted known-robot list so operators can disambiguate from the log.unknown robot_name-> WARNING, with the typo value echoed and the sorted known-robot list (so agrep -F 'rob0t0'finds the offending callsite).Articulation handle is None-> still DEBUG via the inner narrow except (Phase 1 stub state, documented; WARNING here would fire on every observation tick).The docstring now enumerates all four silent-empty modes and records why the return shape is preserved -- so a future contributor doesn't accidentally "fix" the dict shape and break the joint-positions-keyed-by-name contract that all downstream callers (RL loops, teleop receive, observation buffers) consume.
R4 verification
pytest strands_robots_sim/isaac/tests/ --ignore=test_gpu_integ.pyblack --checkon touched filesisort --check-onlyon touched filesflake8 --max-line-length=120on touched filesgit stashround-trip (7/9 fail pre-fix; 9/9 pass post-fix)numpyimport contract /[isaac]extraRound budget
R1 closes 4/4 actionable inline-thread concerns. R2 closes 1/4 directly addressable concerns + 3 carve-outs with documented rationale. R3 is a non-substantive merge commit (zero code change, conflict-resolution only). R4 closes the last remaining R2 carve-out that had a single-commit-shape resolution.
Substantive review rounds used: 3/3 (R1, R2, R4). Mechanical merge: 1 (R3). No further rounds planned -- the two outstanding R2 carve-outs (
simulation.py:627Phase 2 behavioural change,simulation.py:1027Phase 2 path-traversal hardening) are explicitly Phase-2-scope and belong with the loader implementation that consumes them.Ref: cagataycali/strands-gtc-nvidia#334