Repository navigation
Add kitless teleoperation and replay with SO101 support - #8788
hougantc-nvda wants to merge 4 commits into
Conversation
e933899 to
4d7c54f
Compare
fe8d8d2 to
8d56932
Compare
293a54c to
bb8944d
Compare
|
@greptile Please review the latest changes in this PR. |
[Medium impact] The optional OVXR route connects teleoperation, simulation, and an external render worker. Update the publication-failure test double before merging; both parameter cases otherwise fail at the new material lookup. Findings
SummaryAdds kitless live teleoperation and MCAP replay, SO101 support, and ordered XR cleanup. Since the last review:
Previous unnumbered threads were rechecked without posting duplicates. The callback uses Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Task changes material] --> B[Mark channel dirty]
B --> C[Check whether publication is needed]
C --> D[Reserve scene ordinal]
D --> E[Write materials and scene]
E --> F[Wait for material reads]
F --> G[Seal scene ordinal]
G --> H[XR worker renders]
Reviews (4) · Last reviewed commit: "Merge updated prerequisite for #8788" · Reviewed by Greptile |
bb8944d to
62a1200
Compare
72a3b68 to
01fc065
Compare
|
@greptile please review this PR again at the latest head ( |
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (2), Scope and over-engineering (5), Compatibility and deprecation (4), Implementation (5), Style consistency (5), Test quality (8). Other reviewed categories look good.
- PR description: The body cites non-head validation and itself as dependency documentation; replace those references, relocate stray template prose, and provide reproducible Newton capture performance evidence or mark that hot-path risk unresolved.
- Design and architecture: Backend preparation uses string-built hooks, while hand presence reaches through private device lifecycle state; declare task preparation explicitly and expose hand presence through the public device API.
- Scope and over-engineering: Minimal implementation: keep render_lease.py for irreducible ownership lifecycle; merge publication_buffers.py into ovrtx_renderer.py; delete unused packing_table.py and action_policy.py, moving required policy into isaac_teleop_cfg.py/entrypoints; keep ov_libraries_xr.py and ovxr_session.py for Capture adaptation/bootstrap. Suggested PR breakdown: (1) shared runtime fixes; (2) renderer lease/publication; (3) live and replay OVXR entrypoints; (4) SO101 behavior and docs, each with focused owner-boundary validation.
- API: API changes look good.
- Compatibility and deprecation: Breaking changes: ordinary teleop imports optional isaaclab_ov, cleanup errors propagate, pipelined resets become edge-triggered, and registered SO101 initialization changes; isolate dependencies or provide complete transition bridges.
- Implementation: Replay injects a local START before consuming recorded controls, advancing pre-START frames and defeating recorded pauses; remove the injection and let MCAP control events exclusively drive activity.
- Style consistency: New teleop modules use absolute intra-package imports contrary to repository style; restore relative imports and address the associated typing, header, stale prose, and private-access violations.
- Test quality: Added tests repeatedly execute mutated AST fragments, grep source, self-compare exports, restate configuration, and duplicate weaker helper boundaries; replace them with imported public or owner-boundary behavior tests.
Needs rework. Posted 29 inline findings.
Automated review; human maintainers own approval decisions.
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (2), Scope and over-engineering (5), Compatibility and deprecation (4), Implementation (5), Style consistency (5), Test quality (8). Other reviewed categories look good.
- PR description: The body cites non-head validation and itself as dependency documentation; replace those references, relocate stray template prose, and provide reproducible Newton capture performance evidence or mark that hot-path risk unresolved.
- Design and architecture: Backend preparation uses string-built hooks, while hand presence reaches through private device lifecycle state; declare task preparation explicitly and expose hand presence through the public device API.
- Scope and over-engineering: Minimal implementation: keep render_lease.py for irreducible ownership lifecycle; merge publication_buffers.py into ovrtx_renderer.py; delete unused packing_table.py and action_policy.py, moving required policy into isaac_teleop_cfg.py/entrypoints; keep ov_libraries_xr.py and ovxr_session.py for Capture adaptation/bootstrap. Suggested PR breakdown: (1) shared runtime fixes; (2) renderer lease/publication; (3) live and replay OVXR entrypoints; (4) SO101 behavior and docs, each with focused owner-boundary validation.
- API: API changes look good.
- Compatibility and deprecation: Breaking changes: ordinary teleop imports optional isaaclab_ov, cleanup errors propagate, pipelined resets become edge-triggered, and registered SO101 initialization changes; isolate dependencies or provide complete transition bridges.
- Implementation: Replay injects a local START before consuming recorded controls, advancing pre-START frames and defeating recorded pauses; remove the injection and let MCAP control events exclusively drive activity.
- Style consistency: New teleop modules use absolute intra-package imports contrary to repository style; restore relative imports and address the associated typing, header, stale prose, and private-access violations.
- Test quality: Added tests repeatedly execute mutated AST fragments, grep source, self-compare exports, restate configuration, and duplicate weaker helper boundaries; replace them with imported public or owner-boundary behavior tests.
Needs rework. Posted 29 inline findings.
Automated review; human maintainers own approval decisions.
|
@isaaclab-review-bot review |
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (3), Scope and over-engineering (3), Compatibility and deprecation (5), Implementation (1), Style consistency (7), Test quality (4). Other reviewed categories look good.
- PR description: Update the body: this diff adds the OVXR adapter, #8788 is self-referential, and performance results use another revision; correct ownership and links, then benchmark the capture-stream change at head or mark it unmeasured.
- Design and architecture: cleanup_context=False retains SimulationContext while launchers still close, leaving live borrowers without their runtime; retain launcher ownership until drain completes and replace backend-name dispatch with typed configuration.
- Scope and over-engineering: Minimal implementation: keep render_lease.py, ov_libraries_xr.py, and ovxr_session.py for irreducible ownership lifecycles; merge publication_buffers.py into ovrtx_renderer.py; delete action_policy.py and its unused hooks; centralize render-admission retry in OvLibrariesXrSession. Suggested PR breakdown: (1) shared Warp/Newton/launcher/Kit fixes; (2) OVRTX lease and CPU publication; (3) live/replay OVXR, SO101, and documentation.
- API: API changes look good.
- Compatibility and deprecation: Breaking changes: replay startup, SO101 reset/calibration, run_teleoperation positional binding, cleanup error propagation, and exported partition visibility changed without deprecation bridges.
- Implementation: prepare_teleop_backend imports optional isaaclab_ov on every teleop launch, breaking Kit-only installations; defer that import until the resolved physics type requires it.
- Style consistency: The README pipeline factory reuses one pipeline instance instead of constructing fresh session state; document a real factory and separate retargeter factory.
- Test quality: The smoke-test module now contains roughly 700 lines of unit tests, obscuring CI ownership; move them to focused unit modules and remove non-discriminating or duplicated reset/export cases.
Needs rework. Posted 23 inline findings.
Automated review; human maintainers own approval decisions.
6f5fd5a to
a0cedbc
Compare
Description
Review stack · 2/5: #8787 → #8788 → #8789 → #8790 → #8791
Review only this layer. These PRs target
main; the comparison excludes predecessors.Connect live Isaac Capture tracking and recorded MCAP input to kitless simulation through #8787. SO101 gains CPU OvPhysX and Newton selection, plus the current full-home, clutch and gripper integration. Its IK task resets to nominal joints without reset noise on Kit and kitless; the separate joint-control task retains randomized resets.
--xr, and kitless uses--ovxr.IsaacTeleopCfg.prepare_environmentcallback finalizes task settings before simulation creation. Dependent Enable G1 and GR1T2 tasks with kitless XR #8789 supplies the humanoid implementation; generic launchers do not infer optional backend hooks.--start_on_replayexplicitly supports recordings without START; reset does not rewind the recording or become terminal STOP. HDF5 workflows remain on Kit.Narrow shared fixes cover empty CPU Warp arrays, nonblocking graph capture in the Newton backend, and simulation-context retention after failed asynchronous drain. Kit retains headless XR updates without changing ordinary headless on-demand rendering.
The runtime guide links canonical workstation, client, network and license prerequisites. In-process control/configuration tests are separate from subprocess launch smoke tests.
Installation: use published
isaaccapture==1.7.93a1from NVIDIA’s Python index with the scoped OVXR installation. The guide creates the optionalovxrselection; project-wide Kit/provider defaults are unchanged.Type of change
Testing
The combined stack passed 877 tests across 33 affected test files, with no failures or skips. This includes the native Newton graph-capture/replay/reset tests, renderer/lease contracts, recorded-control gating, camera cleanup failures, registered task selection and CPU collision checks, plus the inherited upstream noise-reset cases. Formatting, all five changelog boundaries and the warning-as-error documentation build pass. These are cumulative integration results; individual PR boundaries received static/API and changelog checks. Runtime and performance scope is detailed in #8791.
Remaining qualification: native Kit execution and MCAP-file playback were not rerun; control/replay owner tests substitute external I/O. Native consumer-destruction failure is not established by normal shutdown. Cold renderer attachment, intermittent GR1T2 first-Play recovery and SO101 cube retention remain open. Existing USD, render-product GPU-assignment and native runtime warnings are retained; this is not a warning-free qualification.
Checklist