Skip to content

Remove ovstage host copies - #7157

Open
rilei-nvidia wants to merge 2 commits into
isaac-sim:developfrom
rilei-nvidia:ovstage-performance-improvements
Open

Remove ovstage host copies#7157
rilei-nvidia wants to merge 2 commits into
isaac-sim:developfrom
rilei-nvidia:ovstage-performance-improvements

Conversation

@rilei-nvidia

@rilei-nvidia rilei-nvidia commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

Description

ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX ovstage path no longer needs its per-frame host round-trip

  • Write transforms and points straight from their Warp GPU buffers; make_dltensor now folds a producer's trailing axes into the lane count omni:xform and points expect.
  • Order those writes with write_attribute(cuda_stream=...) instead of blocking the host on wp.synchronize_device, as the legacy binding path already does.
  • Drop the _OVSTAGE_AVAILABLE guard: ovstage is an unconditional dependency of isaaclab_ov, so the fallback was unreachable.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Aug 19, 2026
@rilei-nvidia
rilei-nvidia marked this pull request as ready for review August 19, 2026 02:47
@rilei-nvidia
rilei-nvidia requested a review from a team August 19, 2026 02:47
@greptile-apps

greptile-apps Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR centralizes ovstage construction around the GPU_INCREMENTAL hierarchy model and replaces per-frame GPU-to-host transform and point copies with stream-ordered, zero-copy DLTensor writes.

  • Adds shared ovstage stage and tensor-conversion helpers.
  • Migrates OvPhysx and OVRTX stage creation to the shared configuration.
  • Sends object, camera, deformable, and particle data directly from Warp GPU buffers.
  • Updates unit tests and release notes for the new stage and tensor contracts.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code failure established.

Stage creation is consistently centralized on one hierarchy model, while zero-copy buffers remain alive through synchronous operation waits and the tested particle path preserves the expected CUDA pointer and tensor layout.

Important Files Changed

Filename Overview
source/isaaclab_ov/isaaclab_ov/_stage.py Introduces centralized GPU_INCREMENTAL stage creation and DLTensor adapters for host matrices and zero-copy Warp matrices and points.
source/isaaclab_ov/isaaclab_ov/physics/ovphysx_manager.py Routes the OvPhysx-owned stage through the shared stage factory without altering its ownership or cleanup flow.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py Removes host synchronization and copies from the ovstage path, replacing them with zero-copy GPU writes ordered by the producing Warp stream.
source/isaaclab_ov/test/test_ovrtx_deformable_bindings.py Adds CUDA coverage confirming particle slices retain their device pointer and are represented with the expected DLTensor shape and lane count.

Sequence Diagram

sequenceDiagram
    participant Physics as Newton / PhysX state
    participant Warp as Warp GPU stream
    participant Renderer as OVRTXRenderer
    participant Stage as ovstage
    participant OVRTX as OVRTX

    Physics->>Warp: Produce body and particle state
    Renderer->>Warp: Build object and camera transforms
    Renderer->>Stage: write_attribute(DLTensor, cuda_stream)
    Warp-->>Stage: Stream-ordered buffer availability
    Stage->>Stage: Update GPU_INCREMENTAL hierarchy
    Renderer->>Stage: advance_write_floor
    Renderer->>OVRTX: Render updated scene
Loading

Reviews (1): Last reviewed commit: "Remove ovstage host copies and use GPU_I..." | Re-trigger Greptile

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

The shared ovstage stage factory and stream-ordered zero-copy Warp writes are coherent, but the new DLTensor conversion path requires ovstage 0.1.1 while the permitted installation guidance still includes 0.1.0.

  • Design and architecture: Centralizing stage creation in isaaclab_ov._stage consistently applies the process-wide GPU_INCREMENTAL hierarchy model across OVRTX and OvPhysX. The remaining integration issue is ensuring the package dependency matches the ovstage behavior this architecture requires.
  • API: The private helper API does not introduce a public compatibility concern, and making ovstage unconditional is consistent with the stated dependency model. However, the supported ovstage version contract must raise its lower bound from 0.1.0 to 0.1.1 because trailing-axis lane folding and stream-ordered writes depend on 0.1.1 semantics.
  • Implementation: The transform and particle producer paths correctly pass their Warp CUDA stream into write_attribute, and the writes are awaited while parent buffers remain live. The concrete defect is that xform_tensor_from_warp and points_tensor_from_warp use behavior explicitly unavailable in ovstage 0.1.0 without a corresponding dependency-version update, allowing runtime failures for otherwise permitted installations.

Minor fixes needed. Posted 1 actionable finding inline.

Automated review; human maintainers own approval decisions.

Returns:
A :class:`ovstage.DLTensor` with shape ``[N]`` and ``lanes=16``.
"""
return ovstage.make_dltensor(xforms, dtype=OVSTAGE_XFORM_DTYPE)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Warning · Api — Zero-copy writes need unpinned ovstage 0.1.1

These helpers rely on ovstage folding a DLPack producer's trailing axes into lanes=16/lanes=3, and callers pass write_attribute(cuda_stream=...). The code removed in this diff documented that ovstage 0.1.0 rejects exactly these writes and that the accepted install range is ovstage>=0.1.0,<0.2.0. No dependency metadata is updated here, so a permitted 0.1.0 resolution now fails at render time on every transform and points write. Raise the isaaclab_ov ovstage lower bound to 0.1.1.

points: Warp array of shape ``[N]`` and dtype :class:`warp.vec3f`.

Returns:
A :class:`ovstage.DLTensor` with shape ``[N]`` and ``lanes=3``.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: return types Any do not match the documentation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

tensors=xform_tensor_from_warp(object_transforms),
is_array=False,
semantic=ovstage.AttributeSemantic.MATRIX,
cuda_stream=wp.get_stream(self._device).cuda_stream,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we review situation with streams in subsequent PR please? Some helpers would be useful, Instead of wp.get_stream() we store it as a filed and we do cuda_stream=self._stream?

Additionally we have synchronization issues :

  • Rigid xforms (binding.map + wp.launch) - mplicit: write into mapped buffer on Warp’s current stream. No explicit cuda_stream to OVRTX.
  • Particles (binding.write(..., ASYNC)) - Explicit: passes wp.get_stream(...).cuda_stream so OVRTX can GPU-wait.
  • Cameras (wp.launch then wp.copy into map) - Same as xforms — assume mapped memory is consumed later somehow.

Additionally @pv-nvidia for vis.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will address this in a tiny separate PR that can be merged into the release.

Note that because of a particular implementation detail in ovrtx 0.4 (copy-back uses the default NULL stream which syncs against all blocking streams, which includes the warp stream), it seems the current code is currently working fine without changes though

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rilei-nvidia rilei-nvidia changed the title Remove ovstage host copies and use GPU_INCREMENTAL transform hierarchy Remove ovstage host copies Aug 19, 2026
@rilei-nvidia
rilei-nvidia force-pushed the ovstage-performance-improvements branch from 57f0535 to 5f915a8 Compare August 19, 2026 04:42
ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX
ovstage path no longer needs its per-frame host round-trip

- Write transforms and points straight from their Warp GPU buffers;
  make_dltensor now folds a producer's trailing axes into the lane
  count omni:xform and points expect.
- Order those writes with write_attribute(cuda_stream=...) instead of
  blocking the host on wp.synchronize_device, as the legacy binding
  path already does.
- Drop the _OVSTAGE_AVAILABLE guard: ovstage is an unconditional
  dependency of isaaclab_ov, so the fallback was unreachable.
@rilei-nvidia
rilei-nvidia force-pushed the ovstage-performance-improvements branch from 5f915a8 to 8b7d032 Compare August 19, 2026 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants