Skip to content

[Backport] PR #7347 to release/3.0.0 - #7353

Merged
kellyguo11 merged 3 commits into
release/3.0.0from
backport/release/3.0.0/pr-7347
Aug 27, 2026
Merged

[Backport] PR #7347 to release/3.0.0#7353
kellyguo11 merged 3 commits into
release/3.0.0from
backport/release/3.0.0/pr-7347

Conversation

@isaaclab-bot

@isaaclab-bot isaaclab-bot Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Backports #7347 to release/3.0.0.

The original cherry-pick conflicted. An NVIDIA inference model proposed this resolution, and deterministic validation confirmed that it changes no paths outside the original PR. Because conflict resolution cannot be certified as an exact patch replay, this PR is intentionally a draft and requires release-maintainer review.

Field Commit
Original merged change a2db45885453779bdec71f57b3c5d91f958b713f
Release base used def4b80288de2a8bac3abad4d2e7cdb80b849a4f
Proposed backport 7db3a4902c045a56b31345c231e4afb430f2da29

IsaacLab creates its OVRTX `RenderProduct` without authoring
`deviceIds`. OVRTX therefore picks the render device itself, places the
render product on CUDA 0, and correctly returns a CUDA-0 DLPack buffer
from `render_var.map(device=Device.CUDA)`. The camera tile extraction
kernels, however, launch with `device=self._device` — the simulation
device. On a multi-GPU machine running with `--device cuda:1`,
`extract_all_tiles_kernel` then reads a CUDA-0 buffer from CUDA 1. On a
machine without peer access between the two GPUs that is an illegal
memory access and the run aborts with CUDA error 700; where peer access
is available it silently reads across the link instead of failing.

This authors `uint[] deviceIds = [<cuda index>]` on the render product,
resolving the index with the same helper the attribute mappings use
(`ovrtx_mapping.cuda_device_id`, promoted from `_cuda_device_id` so both
callers share one parser). Both initialization paths — legacy and
ovstage — pass it, so every OVRTX buffer the renderer reads is allocated
on the device its kernels launch on. The generated prim matches the form
OVRTX documents in its own `doc-pin-render-product-to-gpu-0-usda`
snippet:

```
def RenderProduct "RenderProduct" ( ... ) {
    rel camera = [</World/envs/env_0/Camera>]
    uint[] deviceIds = [1]
    ...
```

`device_id=None` keeps OVRTX's automatic assignment, so the USD builders
behave exactly as before for any caller that does not pass a device.

This also resolves the `FIXME` in `_prepare_ppisp_hdr_source`, whose
stated precondition ("until render var mapping can be constrained like
transform bindings") is now met. Its `wp.clone` stays as a fallback for
the case OVRTX logs as *"deviceIds ... not in the active device set"*
and reverts to automatic assignment; the comment is updated to say so.

Fixes # (issue)

- Bug fix (non-breaking change which fixes an issue)

- [x] <!-- backport-active-release --> Backport this pull request to the
active release branch after it merges into `develop`

```
uv run python -m pytest source/isaaclab_ov/test/test_ovrtx_usd.py \
  source/isaaclab_ov/test/test_ovrtx_renderer_kernels.py \
  source/isaaclab_ov/test/test_ovrtx_renderer_contract.py \
  source/isaaclab_ov/test/test_ovrtx_clone_plan.py \
  source/isaaclab_ov/test/test_ovrtx_deformable_bindings.py

uv run python -m pytest source/isaaclab/test/renderers/test_renderer_factory.py
```

Three tests added, each verified to fail on the unfixed sources and pass
with the fix:

* `test_render_product_pins_device_ids_to_the_requested_cuda_device` —
parses the authored snippet through `Sdf.Layer` and asserts `deviceIds`
is a `UIntArray` equal to `[1]`.
* `test_render_product_omits_device_ids_when_no_device_is_given` —
nothing authored when no device is given.
* `test_create_render_data_pins_the_render_product_to_the_spec_device` —
covers the wiring: a spec on `cuda:1` produces a render product pinned
to device 1.

`_make_ovrtx_renderer_without_backend` bypasses `__init__`, so it now
sets `_device` to the same default `__init__` uses.

Not verified here: the multi-GPU, no-peer-access machine from the
report. The change is validated at the USD authoring and renderer wiring
level only.

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have added a changelog fragment under
`source/<pkg>/changelog.d/` for every touched package
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

(cherry picked from commit a2db458)
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Aug 26, 2026
@kellyguo11
kellyguo11 marked this pull request as ready for review August 26, 2026 05:03
@kellyguo11
kellyguo11 requested a review from a team August 26, 2026 05:03

@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

This backport adds optional OVRTX render-product device pinning through the USD deviceIds attribute, shares CUDA ordinal parsing with attribute mappings, and includes focused USD-generation and renderer-path tests plus a changelog fragment.

  • Design and architecture: The reviewed design extends the render-scope builders with an optional device ordinal while preserving OVRTX automatic device selection when none is supplied. Reusing cuda_device_id for mapping and render-product selection is intended to keep their CUDA ordinals aligned; the supplied renderer test demonstrates that a cuda:1 render spec produces deviceIds = [1].
  • API: Both USD-builder parameters are appended with None defaults, preserving existing positional and keyword callers. The omission test confirms that default calls do not author deviceIds. Renaming _cuda_device_id exposes a formerly private helper under a public-looking name, but no unsupported compatibility break is demonstrated.
  • Implementation: The generated property is valid USD uint[] according to the Sdf-based test, and the renderer-path test checks propagation from a cuda:1 spec into the combined stage. Bare "cuda" still resolves to ordinal 0 as explicitly documented pre-existing behavior. The large ovrtx_renderer.py change was supplied without patch or current-file content, limiting direct inspection of that implementation path, but the available evidence does not support a concrete finding.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

@greptile-apps

greptile-apps Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The backport pins OVRTX render-product buffers to the CUDA device used by the renderer, preventing cross-device access during tile extraction.

  • Exposes a shared CUDA-device-index parser for mappings and render-product creation.
  • Authors the selected index as the render product’s deviceIds attribute.
  • Adds USD-generation and renderer-contract coverage for indexed and automatic device selection.
  • Includes the corresponding changelog entry.

Confidence Score: 5/5

The PR appears safe to merge, with the render product and consuming Warp kernels consistently assigned to the normalized simulation CUDA device.

The changed initialization passes the same indexed renderer device used by runtime kernels into OVRTX buffer allocation, and no reachable regression remains in the reviewed renderer lifecycle or USD contract.

Important Files Changed

Filename Overview
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_mapping.py Makes CUDA-device parsing public so mapping and render-product allocation use the same indexed device.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_renderer.py Passes the renderer’s normalized CUDA index into both render-product initialization paths; no actionable lifecycle regression was established.
source/isaaclab_ov/isaaclab_ov/renderers/ovrtx_usd.py Optionally authors a UIntArray deviceIds attribute while preserving automatic OVRTX assignment for callers that omit the device.
source/isaaclab_ov/test/test_ovrtx_clone_plan.py Verifies create_render_data propagates an indexed specification device into the generated render-product USD.
source/isaaclab_ov/test/test_ovrtx_usd.py Verifies the deviceIds attribute’s USD type and value, plus omission when no device is supplied.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Spec[CameraRenderSpec device] --> Parse[cuda_device_id]
    Parse --> USD[RenderProduct deviceIds]
    USD --> Buffers[OVRTX render-var buffers]
    Spec --> Warp[Warp extraction kernels]
    Buffers --> Warp
Loading

Reviews (1): Last reviewed commit: "Fix OVRTX renderer formatting" | Re-trigger Greptile

@kellyguo11
kellyguo11 merged commit 093e624 into release/3.0.0 Aug 27, 2026
47 of 48 checks passed
@kellyguo11
kellyguo11 deleted the backport/release/3.0.0/pr-7347 branch August 27, 2026 06:32
kellyguo11 pushed a commit that referenced this pull request Sep 4, 2026
…used buffer (#7233) (#7566)

# Description

Backport of #7233 (*Replace map transforms with pre-owned and reused
buffer*) to `release/3.0.0`.

This supersedes #7300, which was the original cherry-pick by @pbarejko.
That PR was approved on 2026-08-24 but has been unmergeable
(`mergeable=false`, `dirty`) ever since, so the fix never reached the
release branch. Authorship of the commit is preserved as @pbarejko.

## Why it conflicted

`ovrtx_renderer.py` diverged structurally between `develop` and
`release/3.0.0`. The backport of #7347 (landed as #7353) was not a
line-for-line cherry-pick — on `develop` #7347 changed 11 lines of that
file, while the release-side version changed 1348 (240+/1108−) and
restructured code that now only exists in that shape on the release
branch:

- `_close_legacy` was rewritten from explicit per-binding unbinds into a
`for attr, name in (...)` loop
- `except Exception as e` was renamed to `as exc`
- the close dispatch was restructured

The three-week-old cherry-pick was written against the pre-#7353 shape,
hence the conflicts.

## Conflict resolution

- The four renderer conflicts covering the buffer-write change itself
take the #7233 side.
- `_close_legacy` keeps **the release branch's** loop form and adds only
#7233's semantic contribution (`self._object_transform_buffer = None`).
Taking `develop`'s shape here would have silently reverted #7353.
- The test-file conflict was an add/add at the same line; all three
tests are kept.
- The follow-up `Reformat` commit was a single blank line already
covered by the resolution, so it drops out as a no-op.

## One deliberate change from the original PR

#7233 predates #7169, so it wrote
`cuda_stream=wp.get_stream(self._device).cuda_stream`. `release/3.0.0`
has since taken #7169 and standardized on
`self._warp_device.stream.cuda_stream` — that caching *is* #7169's fix.
Applying the original patch verbatim would have reintroduced a per-call
device lookup into the per-frame transform path, partially undoing a fix
already on the branch.

Both new call sites and the two new tests are aligned to the release
idiom instead. This is the one substantive delta from what was approved
on #7300 and is the part most worth a reviewer's attention.

## Type of change

- Bug fix (non-breaking change which fixes an issue)

## Release backport

This PR targets `release/3.0.0` directly; the change is already on
`develop` via #7233.

## Testing

- `source/isaaclab_ov/test/test_ovrtx_deformable_bindings.py` — 19
passed
- `source/isaaclab_ov/test/test_ovrtx_renderer_contract.py` — 37 passed
- Both new tests (`test_update_transforms_writes_caller_owned_buffer`,
`test_update_camera_writes_without_mapping`) were confirmed to **fail**
against the unpatched release renderer and pass with the change.

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have added a changelog fragment under
`source/<pkg>/changelog.d/` for every touched package
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

Co-authored-by: Piotr Barejko <pbarejko@nvidia.com>
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