Repository navigation
test(rccl): clamp isolated-test GPU pool to devices HIP can open - #14944
Merged
Merged
Conversation
…es HIP can open KFD topology sysfs is not namespaced per container, so a partitioned CI slot reported the host's 8 GPUs and ProcessIsolatedTestRunner handed children ordinals HIP could not open. detectGpuPool() now trims the pool to the device count HIP reports, read in a short-lived forked child so the parent never initialises HIP. Trimming is by length, since HIP exposes the longest valid prefix of HIP_VISIBLE_DEVICES. GraphCaptureStreamOrdering's too-few-GPUs guard was unreachable behind a HIP_CHECK on hipErrorNoDevice; initHarness() now folds that error into a count of zero and skips. Co-authored-by: Pi Agent
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Findings
4 more finding(s), not posted inline
projects/rccl/test/common/ProcessIsolatedTestRunner.cpp:266(Low) detectGpuPool() forks a HIP-initialising probe on every executeAllTests() (~880 call sites), including sequential and CPU-only runs that never use the pool; probe only when a GPU pool is needed.projects/rccl/test/host/process-isolated-runner-pool-test.cc:2(Nit) New file carries a 2025 copyright year; use 2026.projects/rccl/test/host/process-isolated-runner-pool-test.cc:18(Nit) gtest include precedes <vector>; put the standard library first as in process-isolated-runner-test.cc.projects/rccl/test/common/ProcessIsolatedTestRunner.cpp:216(Nit) kProbeErrorExit is not aligned with the two constants above it; clang-format will not fix test/**.
Bottom line: Worth settling before approval: the bare fork()+hipGetDeviceCount probe at ProcessIsolatedTestRunner.cpp:227 can hang under rocprofv3 --hip-trace, and the detectGpuPool() probe-to-clamp wiring has no test.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-14944-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14944-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
… image The pool clamp read the device count from a bare fork() child. A forked child that calls HIP deadlocks under rocprofv3 --hip-trace, which is why the earlier getDeviceCount()/getDeviceMode() probes were removed (EnvVars.cpp:292-297), and detectGpuPool() runs on the executeAllTests() path, so a traced run of any RUN_ISOLATED_TEST* suite would hang. GpuCountProbe runs the query in a fork()+execv()'d copy of the test binary and returns the count over a pipe, following EnvVars::DetectGpuInfo(). The pipe also retires the exit-status encoding, which capped the count at 254 and spent 255 on a failure sentinel. Waiting is bounded by a deadline computed once, so repeated EINTR cannot extend it. The probe child re-execs with no arguments and would otherwise run the whole suite, so the fresh image answers from a file-scope initializer, before main(). The test binaries have several distinct mains and linking the TU is the only requirement. Co-authored-by: Pi Agent
The reason the pool must be reconciled with HIP's device count was written out both on clampPoolToDeviceCount() and again atop its test file, so the two could drift apart. The declaration keeps it; the test file points at it. Trimming the declaration's block of notes that restate the signature leaves the part worth reading, which is why the clamp is by length rather than by value. Co-authored-by: Pi Agent
1 task done
#14542 rebuilt the src/transport host tests as one binary per test file, removing TEST_MICRO_NET_IB_CAST_INIT_SOURCE_FILES, which this branch had added GpuCountProbe.cpp to. Those binaries now come from rccl_add_transport_micro_binary(), so the probe is appended there instead, beside the runner it resolves. rccl-UnitTestsMicroTransportNetTelemetry is process-isolated but did not link hip_fakes.cc, and the probe needs hipGetDeviceCount. Added, matching every other PROCESS_ISOLATED binary. Co-authored-by: Pi Agent
pvallem
reviewed
Nov 3, 2026
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: no test asserts gpuCountViaFreshImage()'s result, so a probe that always fails would leave every pool unclamped and still pass (GpuCountProbe.cpp:193).
Comment @argus review to re-run, or @argus re-review after pushing.
hipErrorNoDevice in an isolated child is now only reachable when the pool was not clamped, since a pool too small for a test is handed over whole and the child skips on a real device count. Folding that error into a skip hid it: the runner counts a skipped child as a pass. GraphCaptureStreamOrderingTests.cpp is left as it was. detectGpuPool() warns when the probe fails, naming the pool it is about to use unclamped. Co-authored-by: Pi Agent
Every probe failure path collapses to kGpuCountProbeFailed, which the clamp treats as unknown and passes through, so a probe that never worked would not have shown up in any suite. The re-exec'd image answers from its own hip_fakes.cc, giving a fixed count to assert against without reaching across execv. A second case hooks the seam in the calling process and expects the probe to ignore it, which is what makes the first case's number mean "fresh image" rather than "this process". Co-authored-by: Pi Agent
1 task done
pvallem
approved these changes
Nov 3, 2026
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
atulkulk
approved these changes
Nov 4, 2026
Contributor
pr-rocm-systems-14944-oci-rccl-pr-buildBuilds
FailuresNo failing tests in this run. 189 tests skipped. pr-rocm-systems-14944-oci-rccl-pr-smokeBuilds
Failures
15 tests skipped. |
Contributor
pr-rocm-systems-14944-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-14944-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ProcessIsolatedTestRunnerbuilt its GPU pool from KFD topology sysfs, which is not namespaced per container, so a partitioned CI slot still reported the host's 8 GPUs. The slot manager handed the third isolated child ordinals 4,5, HIP could not open them, andhipGetDeviceCountreturnedhipErrorNoDevice. This has failed every rccl precheckin since develop 455.detectGpuPool()now trims the pool to the device count HIP reports. The count is read in a short-lived forked child so the parent never initialises the HIP runtime, which would otherwise open GPU file descriptors inherited by every forked test child. Trimming is by length rather than by discarding ids above the count, because HIP exposes the longest valid prefix ofHIP_VISIBLE_DEVICES: a pool of4,5,6,7on an 8-GPU host is entirely valid even though HIP reports 4 devices.GraphCaptureStreamOrdering's own too-few-GPUs guard was unreachable, becauseHIP_CHECKraised a fatal failure on thehipErrorNoDevicereturn before the count was examined.initHarness()now folds that one error into a count of zero and skips cleanly; other HIP errors still fail.clampPoolToDeviceCount()is exposed so the pool-sizing decision can be unit-tested without a GPU, covered by the newprocess-isolated-runner-pool-test.cc.Testing: reproduced the inflated pool on a 1-GPU gfx1151 host via
HIP_VISIBLE_DEVICES;GraphCaptureStreamOrdering.*went fromhipErrorNoDevicefailures to clean skips.test/host/run_host_tests.sh allis green across all 56 binaries. The new tests were confirmed red-green by reverting the clamp.JIRA ID : AICOMRCCL-3520