Select the renderer device by CUDA index - #7057
Conversation
CUDA_VISIBLE_DEVICES renumbers devices for CUDA but not for the graphics stack. AppLauncher passed the same masked index to both /physics/cudaDevice and /renderer/activeGpu, so whenever the visible devices did not begin at zero the renderer requested a device outside the visible set, failed to create it, and the run aborted with CUDA error 700. Translate only the renderer index to a physical one. physics_gpu stays masked because /physics/cudaDevice is resolved by CUDA itself. Masks that already begin at zero are unaffected, and UUID or MIG masks fall back to the previous behavior since no physical index can be derived from them.
/renderer/activeGpu indexes the graphics device list, which CUDA_VISIBLE_DEVICES does not filter, so passing the CUDA device index there selected the wrong GPU whenever the visible devices did not start at zero. The renderer then failed to create a device and the run aborted with CUDA error 700 from a Warp launch. Select the device through /renderer/multiGpu/activeCudaGpus instead, which takes CUDA indices and is translated by the renderer itself, and stop setting activeGpu -- the translation is only applied when no explicit graphics index is given. physics_gpu is unchanged, since /physics/cudaDevice is resolved by CUDA.
…derer-active-gpu-physical-index
Setting active_gpu to None was redundant: SimulationApp already defaults it to None and only emits --/renderer/activeGpu when it is set, so not assigning it leaves the renderer at its own -1 default, which is what the CUDA index translation requires. It also stops overriding an active_gpu passed by the caller. Move the device selection tests into the existing app launcher argv suite, which already covers the arguments handed to Kit.
Greptile SummaryThe PR changes AppLauncher to select the renderer through its CUDA-indexed GPU setting while retaining the CUDA index for physics.
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue identified. The launcher consistently forwards the resolved CUDA index to physics and the renderer’s CUDA-indexed setting, leaves the conflicting graphics-index setting unset, and preserves existing extra arguments with focused regression coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["AppLauncher device (CUDA index)"] --> B["/physics/cudaDevice"]
A --> C["/renderer/multiGpu/activeCudaGpus"]
C --> D["Renderer CUDA-to-graphics index translation"]
D --> E["Selected graphics GPU"]
B --> F["Selected PhysX CUDA GPU"]
Reviews (1): Last reviewed commit: "Leave activeGpu unset and fold device te..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The renderer is now selected through CUDA-indexed /renderer/multiGpu/activeCudaGpus, preserving masked-device behavior and caller-provided extra_args. However, the launcher does not remove an explicitly supplied active_gpu, allowing both renderer selectors to reach SimulationApp and defeating the intended CUDA-index translation.
- Design and architecture: Using the CUDA-indexed renderer setting aligns renderer placement with CUDA-visible indices used by physics, torch, and Warp. The selection path must also ensure the graphics-indexed
active_gpusetting is absent, as the documented translation is only applied when no explicit graphics index is provided. - API: No public CLI flag or default is renamed, and
extra_argsis added to the recognizedSimulationAppconfiguration types. The existing acceptedactive_gpuinput now has an ambiguous interaction with the new internally installed selector and should be cleared or explicitly rejected. - Implementation: The comma-terminated renderer argument and non-destructive
extra_argsmerge are covered by focused tests. A missing case is caller-suppliedactive_gpu:_resolve_device_settingsleaves it inlauncher_args, soactiveGpuandactiveCudaGpuscan be forwarded together and the masked-device fix may not take effect.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| # ``activeGpu`` unset -- the translation is only applied when no explicit graphics index is given. | ||
| launcher_args["physics_gpu"] = self.device_id | ||
| launcher_args["active_gpu"] = self.device_id | ||
| extra_args = list(launcher_args.get("extra_args") or []) |
There was a problem hiding this comment.
🟡 Warning · Api — Caller-supplied active_gpu no longer neutralized
active_gpu is still an accepted launcher setting (_SIM_APP_CFG_TYPES), and this path no longer overwrites it with device_id. A caller passing active_gpu now forwards an explicit /renderer/activeGpu alongside activeCudaGpus; per the invariant documented just above, the CUDA translation is then skipped, so the renderer can silently diverge from physics_gpu and the masked-device failure returns. Consider clearing or rejecting active_gpu when installing the CUDA-indexed selector.
The comma was already asserted by the exact-string check, and the duplicate-append it also guarded is unreachable from a single call site. Fold the physics assertion into the same case: both devices are selected by CUDA index from one call, and asserting them together is what shows why the two are routed differently.
AppLauncher already injects Kit settings in _resolve_kit_args, guarding each against a value the caller supplied. Use that instead of a new extra_args config key: the key was a second route to the same argv, and it appended unconditionally, so a caller-specified device was duplicated rather than respected. Emit it only when Isaac Lab has pinned the process to one GPU. activeCudaGpus also fills the renderer's active-device list, and a one-element list caps the device count at one, which would silently disable multi-GPU rendering in a single process.
…derer-active-gpu-physical-index
| if not any(arg.partition("=")[0] == setting for arg in sys.argv + self._kit_args): | ||
| self._kit_args.append(argument) | ||
|
|
||
| # Select the renderer device by CUDA index, but only where this process is pinned to one |
There was a problem hiding this comment.
can you reduce the comment verbosity?
|
|
||
| def test_both_devices_selected_by_cuda_index(monkeypatch): |
There was a problem hiding this comment.
can you combine these test all together? some of them seems can be pytest parameters
It selects a graphics device, which cannot be resolved correctly under a device mask -- the defect this fixes. Nothing overwrote it any more, so a caller passing one reintroduced the mismatch silently. The renderer device now derives from the simulation device, as physics_gpu does; kit_args still covers explicit control.
|
merge this as the rendering failure is exactly the motion vector due to acidently version bump which already fixed |
Review Map
1. Summary
Multi-GPU rendering fails whenever
CUDA_VISIBLE_DEVICESdoes not begin at zero (1,2,4,5,6,7, …), aborting withCUDA error 700.AppLauncherpassed the masked CUDA index to/renderer/activeGpu, which indexes the graphics device list that the mask never filtered. Select the device by CUDA index instead — 4 lines, 17 configurations validated at 2-8 ranks.2. Type of change
3. Design notes
CUDA_VISIBLE_DEVICESrenumbers devices for CUDA but not for the graphics stack, so everything selecting through CUDA was always correct: torch, Warp, PhysX via/physics/cudaDevice, and the kitlessovrtxrenderer./renderer/multiGpu/activeCudaGpustakes CUDA indices and is translated by the renderer itself (Kit MR !42520);activeGpuis no longer set, since that translation only applies when no explicit graphics index is given.physics_gpuis unchanged.Dropping
activeGpuentirely, or passing Kit's-1auto value, also stops the crash — but every rank then auto-selects the same device and the renderers stack: 380% memory spread at 8 ranks, 26.4 GB on GPU0 against 6.9 GB per peer. Auto-selection is per-process and cannot know the rank.4. Validation
8x L40S, same command throughout, varying only the mask and rank count:
17 configurations, all pass. Ranks 2-8; offset, interleaved (
0,4,1,5) and reversed (7,6,5,4,3,2,1,0) masks, each of which fails before this change; identity masks at 2/3/4/8 ranks as regression checks. Per-GPU memory spread is 99-102% throughout (100% is even) and host RAM is flat at ~6.4 GB/rank, both matching the identity controls — the change costs nothing on either axis.5. Screenshots
None.
6. Checklist
pre-commitchecks with./isaaclab.sh --formatsource/isaaclab/changelog.d/;config/extension.tomlis left to the version-bump job perAGENTS.mdCONTRIBUTORS.mdor my name already exists there