Repository navigation
Add GPU selection for OVXR rendering and scene updates - #8791
hougantc-nvda wants to merge 12 commits into
Conversation
github-actions
Bot
added
documentation
5a0d2b1 to
9220c9f
Compare
348c042 to
c9708f6
Compare
28cd0dc to
d731266
Compare
bba6b90 to
b593108
Compare
|
@greptile Please review the latest changes in this PR. |
 [High impact] The PR adds a new OVXR workflow across shared rendering, simulation, and contributed robot tasks. Update the renderer double in the publication regression test before merging. Findings
SummaryThis PR adds separate GPU choices for physics, OVXR rendering and scene copies. It also adds leased rendering, camera panels and live/replay lifecycle support. The latest changes:
The earlier cleanup, camera-layout, waist-action and Kit-anchor findings remain addressed. The replay Reset finding was withdrawn in the supplied thread. All supplied previous threads were unnumbered, so none require numbered ledger entries. This review used code inspection; it did not run tests or qualify native GPU/headset output. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Physics state] --> B[Scene publisher]
M[Dirty materials] --> B
B --> C[Completed scene writes]
C --> D[Seal scene version]
D --> E[OVXR render worker]
E --> F[Headset and optional camera panels]
Reviews (4) · Last reviewed commit: "Merge updated prerequisite for #8791" · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
b593108 to
78a5ede
Compare
0c0d7c3 to
ed67611
Compare
ed67611 to
c2d0eba
Compare
|
@greptile please review this PR again at the latest head ( |
| writer_ref = self._renderer._visual_material_writer_ref | ||
| writer = writer_ref() if writer_ref is not None else None |
There was a problem hiding this comment.
Both cases of test_host_reconciles_failed_publication_without_reusing_generation use a SimpleNamespace renderer without _visual_material_writer_ref. The first host.publish_complete_scene("previous") now raises AttributeError before reaching the publication checks. The renderer also lacks _publish_visual_materials, which the new write path calls next.
Update the test double for both additions, or use the real renderer, so these cases can still check failed scene publication.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Design and architecture (2), Compatibility and deprecation (4), Implementation (2), Style consistency (4), Test quality (1). Other reviewed categories look good.
- PR description: The title covers only GPU selection while the diff adds OVXR lifecycle, task backends, and unrelated fixes; retitle, correct changed-default claims, and add reproducible split-GPU timing and memory evidence.
- Design and architecture: Backend preparation is duplicated between HumanoidTeleopEnv and prepare_teleop_backend, while spawn wrappers use undeclared config markers; centralize dispatch and declare preparation state under the task configuration owner.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: Breaking changes: fixed-base G1 adds default image observations, registered task classes change, SO101 reset/home defaults change, and task configs hard-import undeclared backends; preserve old contracts or add complete deprecation bridges.
- Implementation: The G1 Newton per-step path performs repeated device scalar synchronizations and a Python environment loop with torch.stack; reduce validation to one transfer and batch cached limits.
- Style consistency: Fixed-base G1 leaves image_obs_list as shared, unserialized class state; annotate it, expose cross-module helpers consistently, and remove redundant forwarding and imports.
- Test quality: Several teleop tests compile private functions from source and inject globals, coupling coverage to file shape; test imported workflow boundaries instead.
Significant concerns. Posted 13 inline findings.
The full PR diff was reviewed; some supplemental surrounding file context was omitted.
Automated review; human maintainers own approval decisions.
| params={"left_eef_link_name": "left_wrist_yaw_link", "right_eef_link_name": "right_wrist_yaw_link"}, | ||
| ) | ||
|
|
||
| robot_pov_cam = ObsTerm( |
There was a problem hiding this comment.
🔴 Critical · Compatibility — Task gains default image observations
The unchanged task ID now always adds three 640x480 RGB terms to its policy group and requires camera rendering, breaking existing policies and recorded schemas. Preserve the current schema and expose cameras through an opt-in…
|
|
||
| """Backend selection for humanoid teleoperation tasks with task-owned preparation.""" | ||
|
|
||
| from isaaclab_newton.physics import MJWarpSolverCfg, NewtonCfg |
There was a problem hiding this comment.
🔴 Critical · Compatibility — Task configs hard-import optional backends
Importing default task configs now requires isaaclab_newton, isaaclab_ov, and isaaclab_physx, none declared in isaaclab_tasks dependencies, so installed wheels can fail before backend selection. Defer these imports or declare the dependencies.
| gym.register( | ||
| id="IsaacContrib-PickPlace-Locomanipulation-G1-Abs", | ||
| entry_point="isaaclab.envs:ManagerBasedRLEnv", | ||
| entry_point="isaaclab_tasks.contrib.teleop_env:HumanoidTeleopEnv", |
There was a problem hiding this comment.
🟡 Warning · Compatibility — Registered entry point type changed
These existing task IDs (and the GR1T2 IDs in pick_place/init.py) now construct HumanoidTeleopEnv instead of ManagerBasedRLEnv, changing the runtime type registry consumers receive. Keep the old entry points or register new IDs.
| # MDP settings | ||
| terminations: TerminationsCfg = TerminationsCfg() | ||
| observations: ObservationsCfg = ObservationsCfg() | ||
| image_obs_list = ["robot_pov_cam", "left_wrist_cam", "right_wrist_cam"] |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Annotate image_obs_list as config field
Without an annotation this is a plain class attribute shared by every instance, excluded from copy/to_dict, so caller appends leak globally. Sibling tasks declare image_obs_list: list[str]; add the annotation.
| flat = action.detach().flatten() | ||
| if flat.numel() != EXPECTED_ACTION_DIM: | ||
| return f"unexpected G1 controller action size {flat.numel()} (expected {EXPECTED_ACTION_DIM})" | ||
| if not bool(torch.isfinite(flat).all()): |
There was a problem hiding this comment.
🟡 Warning · Implementation — Repeated device syncs per validation
Each bool, .cpu(), and float scalar read synchronizes the device, and validation runs twice per control frame. Compute one device-side status and transfer it once before formatting the reason.
|
|
||
| def _apply_kitless_ovphysx_drive_gains(robot_cfg: ArticulationCfg) -> None: | ||
| """Restore stable actuator coverage around the Kit-tuned arm drives.""" | ||
| from isaaclab.actuators import ImplicitActuatorCfg |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Local import duplicates module-level import
This PR adds from isaaclab.actuators import ImplicitActuatorCfg at module level and uses it from there in the Newton path. Remove the redundant function-local re-import.
| def __post_init__(self): | ||
| # post init of parent | ||
| super().__post_init__() | ||
| # The clutch resets to a configured full pose, so the arm must return to the same |
There was a problem hiding this comment.
🟡 Warning · Compatibility — SO101 reset distribution changed
The existing SO101 IK task now forces reset joint noise from 0.02 to zero and changes its clutch home pose, altering initial-state distribution and first commands for all callers. Keep the prior defaults and make…
| backend = None | ||
| if isinstance(physics, OvPhysxCfg): | ||
| backend = "ovphysx" | ||
| elif getattr(getattr(physics, "solver_cfg", None), "solver_type", None) == "mujoco_warp": |
There was a problem hiding this comment.
🟡 Warning · Design Architecture — Backend detection duplicated across packages
This OvPhysx/mujoco_warp sniff and stabilize dispatch is copied from isaaclab_teleop.action_policy.prepare_teleop_backend added in the same PR. Give backend selection one owner and have the other call it.
| default = isaacsim_physx | ||
|
|
||
|
|
||
| def _kitless_backend_prepared(env_cfg, backend: str | None) -> bool: |
There was a problem hiding this comment.
🟡 Warning · Style Consistency — Private helper used across modules
_kitless_backend_prepared is imported by teleop_env.py and three task configs, making it a package contract rather than a file-local helper. Rename it without the leading underscore.
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture |
There was a problem hiding this comment.
🟡 Warning · Test Quality — AST-extracted entrypoint tests assert source shape
This fixture and the similar ones in test_teleop_replay_bootstrap.py, test_teleop_scripts_smoke.py, and test_target_frame_rebase.py compile private functions from source and supply their globals, so behavior-preserving reorganization fails without a product regression. Exercise the imported workflow boundary.
Description
Review stack · 5/5: #8787 → #8788 → #8789 → #8790 → #8791
Review only this PR’s changes. The PRs target
main; the comparison isolates this layer from its predecessors.Allow live OVXR teleoperation to place physics and rendering on different GPUs, with explicit scene-publication placement. This optional path supports GPU-heavy workloads; basic single-GPU and CPU-physics teleoperation remain available.
--deviceselects physics,--ovrtx_active_cuda_gpusselects rendering and--geometry_publication_deviceselects transfer destinations. The latter two require live--ovxr; MCAP replay does not expose them.publication_buffers.pycopies foreign-GPU rigid transforms and deformable points into reusable destination buffers. It retains producer storage and exact streams until consumption, and drains pending work before failure/close cleanup.No automatic CPU staging is enabled. PCIe peer transfers require synchronized value-integrity checks and a supported IOMMU configuration; host preparation belongs to an administrator with idle GPUs. Application startup does not change drivers, IOMMU or host services. Physics fidelity and pacing defaults are unchanged.
See GPU placement and peer-transfer requirements.
Dependencies: public
isaaccapture==1.7.93a1from NVIDIA’s Python index, using the optional XR selection documented in #8788. The stack includes upstream's merged provider upgrade #8858; the installation recipe reuses its OVRTX, OVStage, OvPhysX and schema declarations. Project-wide Kit/provider defaults are unchanged.Lease release quiesces publication and drains retained producer/transfer work before clearing native-owner guards. Failed synchronization retains those guards, preventing subsequent registry cleanup from destroying resources still in use.
Type of change
--step_hzoptionally caps live control updates while rendering runs independently; it does not change physics dt or decimation.Testing
Focused follow-up validation at
dd375570e1bf17d076b18bee47835d77a6647e7e: 154 passed and 3 cross-device cases skipped across the renderer contract, material synchronization, and affected task suites. The task tests exercise registeredgym.make()andinstantiate(cfg)through backend preparation, stopping before native simulation construction; they also cover copied settings, serialization and optional dependencies. Six additional probes of the public Capture 1.7.93a1 admission contract and Lab publication/anchor code passed with native I/O substituted. Formatting/lint and changelog checks pass at the affected PR boundaries. This follow-up does not add IWER, headset, performance or full Kit qualification.Rebased onto upstream main
90a16f06685dbf7948262eafa628c470b4cdc78c, including the merged provider upgrade #8858 and Newton hard-reset changes.Combined-stack validation at
c2d0ebad5b3eb9a5f5a6bd1263961162c90fec3a:1.7.93a1selection resolves from the declared source metadata. Locked synchronization and the installed dependency check passed.This is combined-stack validation, not a full-repository or independent runtime qualification of each PR. IWER scene/control/mirror and performance comparison were deferred. Earlier headset/IWER acceptance remains a preservation baseline; it is not transferred to these heads.
Native Kit XR/Stop-AR, current split-GPU/PiP output and performance remain unverified. Cold renderer attachment, intermittent GR1T2 first-Play recovery and SO101 cube retention remain open. Failure-injection tests establish the exercised ownership/error policy, not behavior during an actual driver fault. Hosted-CI and arm64 results are not claimed.
Checklist
source/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bump package versions — CI handles that)CONTRIBUTORS.mdor my name already exists thereThe warnings checkbox remains open: native-runtime warnings and the separate Kit dependency selection are not fully qualified by this run.