Repository navigation
test(rccl): Add symmetric memory tests in testbed framework - #11953
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: neither the enabled nor the flag-off sweeps assert anything that depends on NCCL_SINGLE_PROC_MEM_REG_ENABLE, so they cannot show the feature is doing anything.
Comment @argus review to re-run, or @argus re-review after pushing.
mch
left a comment
There was a problem hiding this comment.
Would be good to address at least some of the argus comments. A few suggestions from me.
0f8a553 to
b518a40
Compare
07a28f2 to
b2ca727
Compare
|
@argus re-review |
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the - cleanup at test_categories.yaml:157 and :163 newly excludes AllReduce.ManagedMemUserBufferRegistration, AllReduce.ROCTX and AllGather.ManagedMemUserBufferRegistration from all six full_pow2_* categories.
Comment @argus review to re-run, or @argus re-review after pushing.
|
Passes all tests on gfx942 and gfx950 CI failure is probably due to missing ulimit -n 65536 |
44832a3 to
0d00589
Compare
|
No undocumented RCCL or NCCL environment-variable changes. |
|
@cppccppccppc @sunguobao PTAL |
9048319 to
df173ed
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 1,129 lines across 26 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Nothing new worth raising; still open from earlier: SingleProcMemRegTestUtils.hpp:59. Worth clearing before approval.
Comment @argus review to re-run, or @argus re-review after pushing.
cd4a3e5 to
6e33154
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
This reverts commit 65c3b11.
Co-authored-by: Cursor <cursoragent@cursor.com>
Temporary diagnostic to surface the failing HIP VMM call behind the AlltoAll.SingleProcMemReg teardown failure on gfx942 CI. Revert before merge. Co-authored-by: Cursor <cursoragent@cursor.com>
…workers Revert before merge. Preloads libc_malloc_debug with MALLOC_CHECK_=3 and MALLOC_PERTURB_=165 in the exec'd TestBed workers so the gfx942 CI run aborts at the first detected heap corruption instead of at an unrelated later malloc. Co-authored-by: Cursor <cursoragent@cursor.com>
Revert before merge. TestBed workers print the fatal signal and a backtrace before dying, so a silent worker crash is no longer reported only as 'Child N pipe closed unexpectedly'. Single-proc mem reg tests also log NCCL INFO for ALLOC,REG to show the last VMM operation before the crash. Co-authored-by: Cursor <cursoragent@cursor.com>
…G INFO logs" This reverts commit 64e14dc.
…mem reg workers" This reverts commit 60ef679.
… reg tests" This reverts commit e0d396c.
This reverts commit 7c36a3f.
…er dies A worker that crashed (for example SIGSEGV inside the HIP runtime) closed its pipe, and PIPE_READ called exit(1). That ended rccl-UnitTests in the middle of a test: no failure was recorded for it, the remaining tests never ran, and the parent never said how the worker died. In multi-process configurations the parent could instead hang reading a surviving worker that was blocked in a collective on the dead rank. Now a closed pipe, short read or failed write fails the current test with the worker's exit status, kills and reaps every worker of that TestBed, and leaves it with no active children, so the sweep winds down and the next test starts fresh workers. Reads also watch every active worker's pipe, so a peer's death is noticed while waiting on a different worker. RunSimpleSweep now also stops after a failed DeallocateMem; it used to carry on into the next configuration, which with no active ranks divides by zero. Co-authored-by: Cursor <cursoragent@cursor.com>
6e33154 to
c1a4367
Compare
TheRock Submodule Bump ActivityNewest first
|
Motivation
This PR adds changes to testing framework to test symmetric memory for all collectives.
Technical Details
Add Single-Process Memory-Registration Coverage
Branches
users/apotnuru/single-proc-mem-reg-testsusers/apotnuru/testbed-process-infrastructure0f8a5531f49144ec315b27f6eee29f0f4ca93d47Overall summary
This PR adds focused TestBed coverage for
NCCL_SINGLE_PROC_MEM_REG_ENABLEacross eight collectives.The diff contains only unit tests and shared test helpers; it does not modify production RCCL behavior.
The enabled-path tests use symmetric-window allocations and exercise multiple data types, deliberately unaligned element counts, in-place and out-of-place layouts where supported, and HIP graph and non-graph execution. Compact flag-disabled smoke tests run the same allocation path with single-process memory registration disabled, guarding the fallback behavior.
A shared helper owns the environment setup, runtime-version prerequisite, TestBed sweep configuration, and cleanup. Environment changes are scoped and restored after each test so they do not leak into unrelated unit tests.
This PR is intentionally stacked on the TestBed infrastructure PR because it requires
MEM_ALLOC_SYMMETRIC_WIN, grouped window registration, and HIP runtime-version detection added there.Common test behavior
UT_COMM_POOL=0so each environment configuration receives fresh workers.NCCL_CUMEM_ENABLE=1.NCCL_SINGLE_PROC_MEM_REG_ENABLEto1for enabled-path coverage and0for disabled smoke coverage.71260540.MEM_ALLOC_SYMMETRIC_WIN.ncclSum; non-reduction collectives ignore the reduction operation.File-wise summary
projects/rccl/test/AllGatherTests.cppUint64,Uint32,Bfloat16, andUint8.projects/rccl/test/AllReduceTests.cppFloat64,Float32,Float16,Bfloat16, FP8 E4M3, and FP8 E5M2.projects/rccl/test/AllToAllTests.cppUint8,Bfloat16,Uint32, andUint64at 1, 4,096, and 4,314 elements.projects/rccl/test/BroadcastTests.cppUint8,Bfloat16,Uint32, andUint64.projects/rccl/test/GatherTests.cppUint8,Bfloat16,Uint32, andUint64with root rank 1.projects/rccl/test/ReduceScatterTests.cppFloat64,Float32,Float16,Bfloat16, FP8 E4M3, and FP8 E5M2.projects/rccl/test/ReduceTests.cppFloat64,Float32,Float16,Bfloat16, FP8 E4M3, and FP8 E5M2 with root rank 1.projects/rccl/test/ScatterTests.cppUint8,Bfloat16,Uint32, andUint64.projects/rccl/test/common/ScopedEnvVar.hppprojects/rccl/test/common/SingleProcMemRegTestUtils.hppRunSimpleSweepwith symmetric-window allocation.Dependency
The PR base must remain
users/apotnuru/testbed-process-infrastructureuntil that PR merges. Afterward, this branch can be rebased ontodevelopand the PR target can be changed todevelop.Issue Tracking
JIRA ID : AICOMRCCL-1169
JIRA ID : AICOMRCCL-1533
Test Plan
Test Result
Submission Checklist