Remove ovstage host copies - #7157
Conversation
Greptile SummaryThe 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.
Confidence Score: 5/5The 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
Sequence DiagramsequenceDiagram
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
Reviews (1): Last reviewed commit: "Remove ovstage host copies and use GPU_I..." | Re-trigger Greptile |
There was a problem hiding this comment.
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._stageconsistently applies the process-wideGPU_INCREMENTALhierarchy 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 thatxform_tensor_from_warpandpoints_tensor_from_warpuse 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) |
There was a problem hiding this comment.
🟡 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``. |
There was a problem hiding this comment.
nit: return types Any do not match the documentation.
| tensors=xform_tensor_from_warp(object_transforms), | ||
| is_array=False, | ||
| semantic=ovstage.AttributeSemantic.MATRIX, | ||
| cuda_stream=wp.get_stream(self._device).cuda_stream, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
57f0535 to
5f915a8
Compare
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.
5f915a8 to
8b7d032
Compare
Description
ovstage 0.1.1 fixes nvbug 6490020 so the OVRTX ovstage path no longer needs its per-frame host round-trip
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there