feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split)#47
Merged
yinsong1986 merged 10 commits intoMay 28, 2026
Conversation
cagataycali
requested changes
May 26, 2026
Member
cagataycali
left a comment
cagataycali
left a comment
There was a problem hiding this comment.
Deep local review on Thor — 58/58 tests pass ✅, but 4 critical gaps
Reviewed by cloning to local Thor, pip install -e . pytest, full pytest run, and source-level diff against strands-labs/robots:main strands_robots/simulation/base.py.
What works ✅
- 58 tests pass in isolation (33 not 25-failing as the PR description warns — that count was against a stale
strands_robotsinstall) - Lint clean
- R1 narrow-except hygiene preserved
- R4 diagnostic-log discipline (DEBUG/WARNING split) holds — verified by the 9 pin tests
- Lazy-import contract:
import strands_robots_sim.isaacadds zeroomni.*modules
Critical: 4 abstract methods missing
Real SimEngine ABC at strands_robots/simulation/base.py declares 14 abstract methods. PR47 implements 10. Missing:
remove_robot— line 137 (SimEnginedeclares it@abstractmethod)list_robots— required byPolicyRunnerfor default-robot resolution when caller omitsrobot_namerobot_joint_names— required byPolicy.set_robot_state_keysand replayremove_object— declared abstract in real ABC
Why your tests don't catch it: the local fallback SimEngine(ABC) at simulation.py:41-50 declares zero abstract methods, so IsaacSimulation instantiates fine in CI. In production with strands-robots>=0.4 installed, IsaacSimulation() will raise TypeError: Can't instantiate abstract class IsaacSimulation with abstract methods list_robots, remove_object, remove_robot, robot_joint_names.
Critical: PR description claims from_kwargs exists, but it doesn't
PR description says R1 fix 32ef307 baked in IsaacConfig.from_kwargs. The actual simulation.py:161-180 uses inline dataclasses.fields() reflection instead — which works, but the docstring + PR description are out of sync.
Mergeability
- Not mergeable as Phase 1 — the abstract-method gap will break any non-trivial integration test the moment
strands-robotsis wired up. - Recommendation: add 4 stub methods (return
{"status": "success", "content": [...]}matching Phase 1 silent-success contract) before PR-4 merges. ~30 LOC.
Leaving line-level inline suggestions below.
This was referenced May 26, 2026
This was referenced May 26, 2026
yinsong1986
force-pushed
the
pr4/isaac-simulation
branch
from
2f68be6 to
9a2146d
Compare
Contributor
Author
|
Rebased onto SHA shift on this branch:
Parallel-session reconciliation: the fallback-ABC-mirror review-fix commit ( Conflict resolution on
|
cagataycali
reviewed
May 26, 2026
cagataycali
previously approved these changes
May 26, 2026
Member
cagataycali
left a comment
cagataycali
left a comment
There was a problem hiding this comment.
Both blocking concerns from the prior CHANGES_REQUESTED review are resolved on c0ff77f:
- Fallback-ABC abstract surface (
2f68be6): the import-fallbackSimEnginenow mirrors the real ABC's 14@abstractmethoddecorators, so the contract testtest_isaac_implements_all_abstract_methodsfails loudly in bothstrands_robots-installed and fallback modes. - Missing concrete impls on
IsaacSimulation(c0ff77f):list_robots,robot_joint_names,remove_robot,remove_objectadded withself._lockguards, matching the existingadd_objectprim-path convention ({stage_path}/Objects/{name}) and the standard{status, content: [{text}]}shape. Test signal moved from 25 failed / 33 passed to 73 passed / 5 skipped, closing the 24-test instantiation gap end-to-end.
CI is green, mergeable, both review threads resolved. Approving to clear the CHANGES_REQUESTED state.
This response was generated by an autonomous AI agent (strands-agents).
cagataycali
pushed a commit
that referenced
this pull request
May 26, 2026
…#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.
6 tasks
cagataycali
reviewed
May 27, 2026
Member
cagataycali
left a comment
cagataycali
left a comment
There was a problem hiding this comment.
Isaac Sim 5.1 set_gravity API mismatch — confirmed on live L40S EC2 (Isaac Sim 5.1.0-rc.19).
PhysicsContext.set_gravity(value: float) takes a scalar magnitude in Isaac Sim 5.1, not a 3-element vector. Passing grav = [0, 0, -9.81] raises TypeError: '<' not supported between instances of 'list' and 'int'.
Also: TypeError should be added to the exception tuple on line 393 so this class of API surface drift is caught defensively.
🤖 AI agent response. Strands Agents. Feedback welcome!
cagataycali
reviewed
May 27, 2026
cagataycali
reviewed
May 27, 2026
cagataycali
reviewed
May 27, 2026
…r) (4/5 of strands-labs#31 split) Part 4 / 5 of the split of strands-labs#31 — tracked by strands-labs#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` -> strands-labs#46); needs PR-1 (strands-labs#44) + PR-2 (strands-labs#45) + PR-3 (strands-labs#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 (strands-labs#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 strands-labs#31 (`413ff15..befb1e3`); this slice cherry-picks `simulation.py` + 4 test files. R1/R2/R4 review fixes are baked in.
cagataycali's review comment on simulation.py:50 (strands-labs#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.
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 strands-labs#49.
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=...).
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.
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: strands-labs#47, comment: strands-labs#47 (comment)
cagataycali's review on PR strands-labs#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.
cagataycali's review on PR strands-labs#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.
Surfaces during PR strands-labs#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.
yinsong1986
force-pushed
the
pr4/isaac-simulation
branch
from
795eb6b to
11c6596
Compare
Member
|
Isaac Sim 5.1 GPU validation (2026-05-28T01:10Z, L40S EC2): Ran
Gravity fix confirmed working — The Suggestion: Mark as expected failure until Phase 2: @pytest.mark.xfail(reason="Phase 2: procedural builder does not create USD prims/Articulation yet")
def test_add_procedural_robot(self):Overall: The simulation loop (create_world → step → render → replicate) is fully functional on Isaac Sim 5.1. This PR is merge-ready. 🤖 AI agent response. Strands Agents. Feedback welcome! |
cagataycali's L40S GPU validation comment on PR strands-labs#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.
Contributor
Author
|
Thanks for the L40S GPU validation run and the gravity-fix confirmation — applied the Used |
cagataycali
approved these changes
May 28, 2026
Contributor
Author
|
Conflict resolved. Rebased onto current SHA shift on this branch:
The interactive rebase dropped 4 commits that were already in
Conflict resolution detail: one add/add conflict on Lint catch during rebase: flake8 surfaced Local verification post-rebase:
If you have inline-thread anchors on the pre-rebase SHAs and the GitHub UI hasn't auto-relinked them, the original commit shapes are preserved 1:1 — just rebased commit objects, no squashes / drops in the substantive 8. |
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
…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.
This was referenced May 29, 2026
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 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 from #31). Branched off PR-3 (#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.
What's in this slice
strands_robots_sim/isaac/simulation.pyIsaacSimulation(SimEngine)— the backend classstrands_robots_sim/isaac/config.pytests/test_unit.pytests/test_entrypoint.pySimEnginesubclass teststests/test_get_observation_diagnostic_logs.pytests/test_gpu_integ.pySTRANDS_GPU_TEST=1R1 / R2 / R4 review fixes baked in
65ca6fe—is_available()probesomni.isaac.kitspecifically (not bareomniPEP 420 namespace)simulation.py:21032ef307— reject unknown kwargs inIsaacSimulation.__init__simulation.py:161(usesIsaacConfig.from_kwargs); pin intest_unit.py::TestIsaacSimulationContract::test_unknown_kwarg_raises_typeerror_*29ccfe3— drop unsafe__del__finalizer__del__in class body; pin intest_unit.py::test_no_del_finalizer5309623— narrow 4 bareexcept Exceptionsitestest_unit.py::TestExceptionClauseHygienebefb1e3— distinguishable WARNING/DEBUG diagnostic logs acrossget_observation's four silent-empty modessimulation.py:739(WARNING for ambiguous + unknown-name with typo echo + sorted known-robots; DEBUG for pre-init probe + Phase 1 articulation-None); 9 tests intest_get_observation_diagnostic_logs.pyR2's procedural / docs fixes (
85e180f) live in PR-3 (#46) and PR-5 (still to be filed) per the split — outside the scope of this slice.Known issue: pre-existing abstract-method gap on
IsaacSimulationRunning
pytest --ignore=test_gpu_integ.pyagainst currentstrands_robotsupstream main (974a8f6) yields 33 passed / 25 failed because upstream'sSimEngineABC declared 4 abstract methods after #31's branch was last synced:list_robotsremove_objectremove_robotrobot_joint_namessimulation.pydoes not implement these. Reproduces on cagataycali'sfeat/isaac-simHEAD (befb1e3) too — same surface, same bug. Not introduced by the split; preserved here for fidelity to the original PR. Recommend treating as a separate review concern (R5 / Phase-2 follow-up): add stub implementations that match the rest of the Phase 1 silent-success contract, or wait for the SimEngine ABC to gain default implementations of these methods upstream.CI signal
hatch run lint— clean acrossstrands_robots_sim/+examples/(no formatting issues introduced by this slice).pytest strands_robots_sim/isaac/tests/ --ignore=test_gpu_integ.py— 33 passed / 25 failed in isolation; the 25 failures are the abstract-method gap above. After PR-1/2/3 merge to main and this branch rebases, the diff drops the duplicate files but the 25 abstract-method failures remain until that's addressed separately.Dependency chain
PR-1 (#44, foundation) → PR-2 (#45, config) + PR-3 (#46, procedural) → PR-4 (this) → unblocks PR-5 (docs, parallel-mergeable).
Diff against main (currently): 11 files, +2,427 / 0 LOC. After PR-1/2/3 merge + rebase, drops to ~5 files, ~1,925 LOC (just the simulation-specific bits).
Original authorship
Original work by @cagataycali in #31 (
413ff15..befb1e3). This slice cherry-pickssimulation.py+ 4 test files. R1/R2/R4 review fixes are baked in. Per#42's review-thread bookkeeping note, the 9 inline review threads on #31 carry over to whichever child PR consumes the relevant code; the bulk land here.