Repository navigation
fix(samples): respect CUDA_VISIBLE_DEVICES in multi-GPU cuda.core samples - #3220
mangeshraut712 wants to merge 4 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
cuda.core
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Update multi-GPU sample device counting samples/cuda_core/simpleP2P/simpleP2P.py, samples/cuda_core/simpleMultiGpu/simpleMultiGpu.py, samples/cuda_core/multiGPUGradientAverage/multiGPUGradientAverage.py, samples/cuda_core/simpleP2P/README.md, samples/cuda_core/simpleMultiGpu/README.md, cuda_core/tests/example_tests/test_sample_verification.py |
All three samples count devices with len(Device.get_all_devices()). The README text describes the API, and the test checks that the samples use it instead of get_num_devices. |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
Count devices available to the process in the three multi-GPU samples [#3215] |
✅ | |
Update the README descriptions of device counting [#3215] |
✅ |
Possibly related PRs
- NVIDIA/cuda-python#3198 — Both changes use process-visible device enumeration. This PR applies it to sample device counts.
- NVIDIA/cuda-python#3126 — Both changes concern visible-device mapping. This PR changes sample device counts.
Suggested labels: bug
Suggested reviewers: danielfrg
Priority:
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
decf5
-
The requested device-counting behavior and documentation updates are aligned, with no remaining merge-blocking risk.
Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Signed-off-by: Mangesh Raut <mbr63@drexel.edu>
|
I checked the review feedback and local lint. The Codex review did not publish a concrete finding and CodeRabbit reported no actionable comments; I only fixed the Ruff formatting violation in the sample-verification test. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
README.md:135
samples/cuda_core/simpleP2P/README.md:135
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winsuggestion: Update the expected output label.
The sample now prints
CUDA-visible device count, but the README still documentsCUDA-capable device count. Update the expected output to match the sample.Suggested fix
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 590a8bf
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
1aaa474b-1907-4383-bd88-538c5da36f9c
-
samples/cuda_core/multiGPUGradientAverage/multiGPUGradientAverage.pysamples/cuda_core/simpleMultiGpu/simpleMultiGpu.pysamples/cuda_core/simpleP2P/simpleP2P.py
-
- samples/cuda_core/multiGPUGradientAverage/multiGPUGradientAverage.py
- samples/cuda_core/simpleP2P/simpleP2P.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
|
@0z5a, I noticed in #3215 that you already have a fix ready. To avoid duplicating your work, could you and the maintainers advise how this PR should be coordinated with that change? I’ll hold further changes pending your direction. The runner bot also indicates that contributor vetting is pending. Please let me know if there is a contributor-side step I should complete. |
|
/ok to test decf58d |
|
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
Thanks, the change is right and complete. Device.get_all_devices() goes through cuDeviceGetCount, so it honors CUDA_VISIBLE_DEVICES, while system.get_num_devices() is the NVML count and does not; every later enumeration in the three samples follows from the new count (device construction, peer access, stream placement), and the CI runner's own GPU gate uses the driver count too, so the samples and the min_gpus gate now share one visibility rule. On the h100 x2 and t4 x2 rows the two simple samples and the three new tests pass.
One change before merge:
samples/cuda_core/simpleP2P/README.mdline 134: the Expected Output block still showsCUDA-capable device count: 2, but the script now printsCUDA-visible device count: N. Please update the line; CodeRabbit flagged it as an outside-diff comment.
Two optional cleanups in multiGPUGradientAverage.py, since this sample is not run in CI (mpi4py is not installed on the runners) and nothing exercises them: the num_gpus == 0 branch can no longer fire, because Device.get_all_devices() raises CUDA_ERROR_NO_DEVICE from cuInit before the count exists; and the try: Device(dev_id) except ...: Device(0) fallback no longer guards anything, since rank % num_gpus is always a valid ordinal now.
Let's wait to hear @0z5a's preference and then either move forward with this change or theirs. |
|
Thanks for checking and for holding off while we coordinated. I had already prepared and tested a fix for #3215 on B300, and I would prefer to proceed with my PR, #3277. I appreciate your work on this and the additional CI coverage. I will also run further tests on B300, including the MPI sample that was skipped in the earlier run. |
|
Thanks for the fix, @mangeshraut712, and for holding off while we sorted this out. @0z5a had already prepared and tested a fix for #3215 and prefers to go ahead with #3277, so we are closing this PR in favor of that one. Your review round here is not wasted: the points it raised (the stale README output line, the dead code in the MPI sample) carry over to the review of #3277. |
Closes #3215.
Summary
Updated
simpleP2P,simpleMultiGpu, andmultiGPUGradientAverageto size device enumeration withlen(Device.get_all_devices()), which follows CUDA runtime visibility includingCUDA_VISIBLE_DEVICES. Corrected the two affected READMEs and added a focused regression test. Keptsystem.get_num_devices()unchanged for NVML system inventory use.Files
samples/cuda_core/simpleP2P/simpleP2P.pysamples/cuda_core/simpleP2P/README.mdsamples/cuda_core/simpleMultiGpu/simpleMultiGpu.pysamples/cuda_core/simpleMultiGpu/README.mdsamples/cuda_core/multiGPUGradientAverage/multiGPUGradientAverage.pycuda_core/tests/example_tests/test_sample_verification.pyTip
b462a85c70f6184c928a21dff11a19f50bd49d91Test plan
git diff --check