Repository navigation
Conversation
#14529 and #13098 each defined rcclCeAllReduceEnabled and rcclForceCeAllReduceEnabled in test/host/fakes/rccl_wrap_fakes.cc, so develop no longer compiles the host microtests (redefinition errors in rccl-UnitTestsMicroCollectives and every other target that links the file). Keep the #13098 definitions: rcclCeAllReduceEnabled honors g_rcclParamCeAllReduce, which ce-coll-test.cc sets. Remove the fail-loud copies from #14529. Co-authored-by: Cursor <cursoragent@cursor.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
ARGUS
Findings
1 more finding(s), not posted inline
projects/rccl/test/host/fakes/rccl_wrap_fakes.cc:418(Low) The kept rcclCeAllReduceEnabled fake falls back to true, but production falls back to false on every arch except gfx1250 (rccl_wrap.cc:1073). A follow-up could make the fake default to false. As a follow-up, could the keptrcclCeAllReduceEnabledfake fall back tofalseat line 418? That would matchrcclCeCollArchDefaultinrccl_wrap.cc:1073-1076, which enables CE AllReduce only on gfx1250.ce-coll-test.ccsetsg_rcclParamCeAllReduce, but that value is read throughrcclParamCeAllReduce()(ce_coll.cc:3261) and not through this fake, so no current test depends on its default.
Bottom line: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
Author
|
Closing as a duplicate: #14633 removed the same duplicate CE AllReduce fakes from |
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.
Motivation
developno longer builds the RCCL host microtests. #14529 and #13098 each addedrcclCeAllReduceEnabledandrcclForceCeAllReduceEnabledtotest/host/fakes/rccl_wrap_fakes.cc. Both merged on 2026-10-29, so the Host unit tests (CPU-only) job now fails for every RCCL PR built againstdevelop:Technical Details
Keep the #13098 definitions. Its
rcclCeAllReduceEnabledhonorsg_rcclParamCeAllReduce, whichce-coll-test.ccsets. Remove the fail-loud copies from #14529.rcclForceCeScratchEnabled, which #14529 added next to them, is unchanged.Issue Tracking
N/A (develop build break)
Test Plan
In the CI image (
rocm/dev-ubuntu-22.04:7.2), runrun_host_tests.shphasesdeps,rccl-configure,hipifyandconfigure. Then buildrccl-UnitTestsMicroCollectives(the target that failed in CI),rccl-UnitTestsMicroCeandrccl-UnitTestsMicro, and run the first two.Test Result
Submission Checklist