[isaac] R7 Phase 1: umbrella tracker for split of #31

[yinsong1986]

Umbrella tracker — splitting #31 into 5 child PRs

#31 is the canonical R7 Phase 1 'Isaac Sim backend skeleton' PR (12 files changed, +2,827/-6, 9 review threads, 4 R-rounds). It is correct as a single piece of work but operationally hostile to review at this size — simulation.py alone is 1,097 LOC. Splitting it into 5 dependency-ordered child PRs lets reviewers focus on one subsystem at a time, keeps git bisect useful, and follows the same precedent established on strands-labs/robots#195 → #220-#228 (tracked by robots#219).

Scope (recap from #31)

Phase 1 of the Isaac Sim backend (R7): skeleton + entry-point + tests, no GPU required for CI. Full implementation (stages, sensors, replicator, fleet) follows in subsequent phase PRs.

Concern	Module	Phase 1 status
Entry-point discovery	pyproject.toml, isaac/__init__.py	✅ working
Config validation	isaac/config.py (IsaacConfig)	✅ working
Procedural USD builders	isaac/procedural.py (SO-100 / Panda / G1)	✅ builders return shapes; articulation defects deferred to Phase 2
SimEngine lifecycle (init / is_available / cleanup)	isaac/simulation.py	✅ working
World creation / spawning / observation / render	isaac/simulation.py	⚠️ silent-success on procedural add_robot; get_observation returns {} (R2/R4 disclosure documented)
GPU integration	tests/test_gpu_integ.py	gated on STRANDS_GPU_TEST=1
Documentation	docs/backends/isaac.md	✅ install + usage + Phase 1 status banner

Solution shape

PEP 562 lazy imports throughout (zero omni overhead on import strands_robots_sim); SimulationApp singleton; headless=True default; thread-safe via threading.RLock; is_available() returns (bool, str|None); no binary assets (procedural robots built from code); GPU tests gated behind @pytest.mark.gpu + STRANDS_GPU_TEST=1. R1 narrowed is_available() to probe omni.isaac.kit specifically; R1 added unknown-kwarg rejection; R1 dropped unsafe __del__; R1 narrowed 4 bare-except sites; R2 corrected G1 DOF count + added Phase 1 doc disclosure banner; R4 added distinguishable WARNING/DEBUG diagnostic logs across get_observation's four silent-empty modes (typo / forgotten create_world / Phase 1 articulation None / ambiguous robot_name).

Child PRs (in landing order)

• PR-1 (#44) feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) — pyproject.toml ([isaac] extras + entry-points + pytest>=7.0 dev/hatch deps + gpu pytest marker), isaac/__init__.py (PEP 562 lazy stub), isaac/tests/__init__.py. ~68 LOC. Demonstrates entry-point discovery without any simulation code. Lands first.
• PR-2 (#45) feat(isaac): IsaacConfig dataclass with validation (2/5 of #31 split) — isaac/config.py. ~116 LOC. Pure dataclass, no omni deps. R1's "reject unknown kwargs" pin lives here (from_kwargs classmethod). Branched off PR-1.
• PR-3 (#46) feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) — isaac/procedural.py + carved tests/test_procedural_g1_dof.py. ~331 LOC. R2's G1-DOF=21 truth-source pin lives here. Branched off PR-1; parallel-mergeable with PR-2.
• PR-4 (#47) feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) — isaac/simulation.py + remaining tests/test_unit.py + tests/test_entrypoint.py + tests/test_get_observation_diagnostic_logs.py + tests/test_gpu_integ.py. ~2,000 LOC. Carries R1's omni.isaac.kit-specific probe + no-__del__ + narrow except + R4's get_observation diagnostic logging. Branched off PR-3 (depends on PR-2's config.py + PR-3's procedural.py).
• PR-5 (#48) docs(isaac): backend reference + Phase 1 status banner (5/5 of #31 split) — docs/backends/isaac.md + carved tests/test_phase1_doc_banner.py. ~288 LOC. R2's Phase-1-status disclosure banner about silent-success methods. Parallel-mergeable with PR-4.

Dependency graph

PR-1 (#44, entry-point + extras + lazy import)  ──┐
                                                   ├──► PR-2 (#45, config dataclass)        ──┐
                                                   └──► PR-3 (#46, procedural USD builders) ──┤
                                                                                              ├──► PR-4 (#47, IsaacSimulation backend) ──┐
                                                                                              │                                          │
                                                                                  PR-5 (#48, docs)  ───────────────────────────────────────┴──► close #31

PR-1 (#44) lands first.
PR-2 (#45) / PR-3 (#46) land in parallel after PR-1.
PR-4 (#47) needs PR-2 + PR-3.
PR-5 (#48) is parallel-mergeable with PR-4 (docs only, no code dep).
This issue closes when PR-5 (#48) lands and #31 is closed with a redirect comment.

Carved test files (split-specific bookkeeping)

test_phase1_doc_honesty.py from #31 was carved into two files so each parallel slice is CI-green standalone:

Source class on #31	New file	Lands in
TestG1DOFCount (3 tests)	tests/test_procedural_g1_dof.py	PR-3 (#46)
TestIsaacDocsPhase1Banner (3 tests)	tests/test_phase1_doc_banner.py	PR-5 (#48)

Combined coverage equals the original file.

Review-thread bookkeeping

The 9 review threads on #31 stay open until the corresponding child PR closes them; child PRs cite the original #31 thread URL when resolving (mirrors #195's R-round practice). The R1/R2/R3 (merge)/R4 commit chronicle on #31 is the forensic source of truth — child PRs cherry-pick the relevant content from cagataycali/feat/isaac-sim (HEAD befb1e3).

CI expectations

• PR-1 (feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) #44) / PR-2 (feat(isaac): IsaacConfig dataclass with validation (2/5 of #31 split) #45) / PR-3 (feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) #46) / PR-5 (docs(isaac): backend reference + Phase 1 status banner (5/5 of #31 split) #48) — CI green standalone (parallel-mergeable group; PR-2/PR-3/PR-5 depend only on PR-1's foundation).
• PR-4 (feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) #47) — CI red in isolation (simulation.py imports IsaacConfig from PR-2 and procedural.get_procedural_robot from PR-3; test_unit.py has 25 abstract-method failures pre-existing on feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7) #31 against current upstream strands_robots). Will go green after PR-2 + PR-3 merge to main and this branch rebases. Pre-existing abstract-method gap noted in PR-4's body — separate review concern, not introduced by the split.

Closing condition

Closes when PR-5 (#48) lands and #31 is closed with a redirect comment to the 5 child PRs.

---

Tracking child of #31. Following the precedent set by robots#195 → #220-#228.

[yinsong1986]

Filed PR #43 — feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split).

Status of the 5-PR split

• PR-1 — #43 — open, awaiting review. pyproject.toml ([isaac] extras + entry points), isaac/__init__.py (PEP 562 lazy stub), isaac/tests/__init__.py, isaac/tests/test_entrypoint.py. +233/-7, 4 files, 10 tests (9 passed + 1 skipped under hatch's locked venv; 10 passed under pip install -e .). CI green standalone.
• PR-2 — IsaacConfig dataclass with validation. Unblocked once PR-1 merges.
• PR-3 — procedural USD builders (SO-100, Panda, G1). Unblocked once PR-1 merges.
• PR-4 — IsaacSimulation backend (lifecycle + observation + render). Needs PR-2 + PR-3.
• PR-5 — docs/backends/isaac.md reference. Parallel-mergeable with PR-4.

PR-1 scope decisions worth flagging

• The entry points in pyproject.toml (isaac, isaac_sim alias) are intentionally dangling until PR-4 merges — they reference strands_robots_sim.isaac.simulation:IsaacSimulation which doesn't exist yet. PR-1's tests verify declaration via pyproject.toml parsing + the lazy-import contract (zero omni.* modules on import strands_robots_sim.isaac), not loadability of the resolved class.
• Tests covering SimEngine subclassing, abstract-method completeness, is_available() return shape, and no-GPU constructor instantiation were deferred to PR-4 because they require importing the simulation module. Including them in PR-1 would have re-created the cross-PR dependency the split is trying to avoid. Net: the original test_entrypoint.py from feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7) #31 was 8 tests covering both surfaces; PR-1 ships 10 tests scoped to declaration + lazy-import only, PR-4 will ship the runtime-introspection tests.
• Hatch test script was switched from the import-smoke placeholder to pytest strands_robots_sim/isaac/tests/ -v — this is the same change that landed in feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7) #31's merge commit cbd4355. PR-4 will add --ignore=strands_robots_sim/isaac/tests/test_gpu_integ.py once that file lands.
• The gpu pytest marker is declared in PR-1's pyproject.toml (forward-compat with PR-4's @pytest.mark.gpu test class), even though no test uses it yet; declaring it eagerly avoids a PytestUnknownMarkWarning flap when PR-4 lands.

Cherry-pick provenance: PR-1 corresponds to the entry-point + extras + lazy-import slice of #31's first commit (413ff15) plus the pyproject.toml test-script change from #31's merge commit (cbd4355). The R1/R2/R3/R4 commits on #31 don't apply to PR-1's surface; they'll be cherry-picked into PR-3 (R2 G1-DOF) and PR-4 (R1 omni.isaac.kit probe + R1 unknown-kwarg + R1 no-__del__ + R1 narrow-except + R4 diagnostic-logs) where the review-thread anchors still hold.

cc @cagataycali for review on PR-1.

[cagataycali]

Stack-wide review summary (local Thor verification)

Reviewed all 6 PRs in the #31 split (#43, #44, #45, #46, #47, #48) by cloning each branch to local Thor, running pip install -e . pytest, full pytest run, and source-level diff against strands-labs/robots:main's real SimEngine ABC.

Per-PR verdict

PR	What	Tests on Thor	Verdict
#43	PR-1 alt (orphan, feat/isaac-extras-and-lazy-import)	10/10 ✅	Close as superseded by #44, but port the 10 tests + hatch script into #44 first
#44	PR-1 canonical (pr1/isaac-foundation, parent of #45-48)	0 collected ❌	Block on missing tests — port from #43, then merge first
#45	PR-2 IsaacConfig	0 collected (intentional, tested in #47)	✅ Mergeable; PR description claims from_kwargs but code doesn't have it — fix one or the other
#46	PR-3 procedural USD builders	3/3 ✅	✅ Mergeable; G1 has 6 duplicate parent→child edges (acknowledged but no runtime guard — left inline suggestion)
#47	PR-4 IsaacSimulation backend	58/58 ✅	REQUEST_CHANGES — fallback SimEngine ABC stub hides 4 missing abstract methods (list_robots, remove_robot, robot_joint_names, remove_object). Will TypeError in production with real strands_robots>=0.4
#48	PR-5 docs + Phase 1 banner	3/3 ✅	✅ APPROVED — clean, honest, banner correctly placed

Critical merge order

1. Port 10 tests from feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) #43 → into feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) #44 (one new commit on pr1/isaac-foundation)
2. Close feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) #43 as superseded
3. Merge feat(isaac): [isaac] extras + entry-point + PEP 562 lazy import (1/5 of #31 split) #44 (foundation)
4. Merge feat(isaac): IsaacConfig dataclass with validation (2/5 of #31 split) #45 + feat(isaac): procedural USD builders (SO-100, Panda, G1) (3/5 of #31 split) #46 in parallel
5. Fix feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) #47's abstract-method gap (add 4 stub methods to satisfy real SimEngine ABC, ~30 LOC)
6. Merge feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) #47
7. Merge docs(isaac): backend reference + Phase 1 status banner (5/5 of #31 split) #48 (parallel-mergeable with feat(isaac): IsaacSimulation backend (lifecycle + observation + render) (4/5 of #31 split) #47, but easier to merge after)
8. Close feat(isaac): IsaacSimulation backend skeleton + entry-point registration (R7) #31 + close this tracker ([isaac] R7 Phase 1: umbrella tracker for split of #31 #42)

The one critical bug

#47's fallback SimEngine(ABC) stub at simulation.py:41-50 declares zero abstract methods. This means IsaacSimulation() instantiates fine in CI (which is why all 58 tests pass), but in production with strands-robots>=0.4 installed, the real SimEngine ABC declares 14 abstract methods and IsaacSimulation only implements 10. End result: TypeError: Can't instantiate abstract class IsaacSimulation with abstract methods list_robots, remove_object, remove_robot, robot_joint_names.

Left an inline suggestion on #47 with the fix (mirror the abstract surface in the fallback stub, then either implement the 4 missing methods or fail fast at IsaacSimulation() construction).

Nice work overall on the split @yinsong1986 — 5/5 PRs are well-scoped, dependency chain is correct, and the test-carving (splitting test_phase1_doc_honesty.py into the procedural + docs halves) keeps each PR CI-green standalone. The abstract-method gap is the only real blocker.