Repository navigation
[None][fix] Keep the family suffix for 10x architectures in DeepEP - #19095
100milliongold wants to merge 1 commit into
Conversation
The architecture filter in cpp/tensorrt_llm/deep_ep/CMakeLists.txt only routes 10x architectures to the family/arch-specific branch when the minor version is 0. Any other member of the 10x family falls through to the final else branch and is emitted without a suffix, even though the comment directly above states that the FP4-related conversion instructions require an arch- or family-specific target. Signed-off-by: 김재억 <gadian88@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe DeepEP CMake configuration now applies the special SM100/SM103 mapping to all CUDA major version 10 architectures instead of only CUDA 10.0. ChangesCUDA architecture handling
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to This is a narrowly scoped build-configuration correction with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Follow-up: this is not only a consistency issue, it is a hard build failure. I hit it while building The build break
Which targets actually work I replayed the exact failing compile (same
The bare Corroboration from upstream a85c26c ("Map SM107 to the sm_100f NVRTC target in the trtllm-gen FMHA kernel"), merged today, applies the same Note that on |
|
@Barry-Delaney @leslie-fang25 Gentle ping. This has had no review since it was opened on Sep 12. It is a build break rather than a consistency nit. Building One thing worth flagging while you are here: the only checks that have ever run on this PR are DCO, If the approach is wrong I am glad to rework it. I would mainly like to know whether the family suffix belongs here, or whether DeepEP is meant to be built for a single architecture. |
BowenFu
left a comment
There was a problem hiding this comment.
Approved — Rubin release fix is correctly scoped.
The change preserves the family suffix for every CUDA major-10 target, so SM107 follows the established 100f compatibility path instead of producing invalid bare sm_107 PTX. The supplied CMake matrix and full-build compile evidence cover the affected 100/103/107 variants. I found no additional blocker.
|
/bot run --disable-fail-fast |
|
PR_Github #74891 [ run ] triggered by Bot. Commit: |
|
Rubin 1.3 approval needed. @Barry-Delaney @leslie-fang25 @NVIDIA/trt-llm-moe-devs: this is a one-line root-cause fix for the SM107 DeepEP build failure. The author supplied exact failing |
|
Automatically added "ci: full pre-merge approved" because this PR has satisfied the required GitHub review approvals. Unresolved review conversations and other required checks remain independent merge requirements. |
|
PR_Github #74891 [ run ] completed with state
|
|
@BowenFu Thanks for the review and for triggering the run. The pipeline came back While waiting I checked what is visible from outside, and I'd rather show my reasoning than just assert it. The same count appears on unrelated PRs. In the most recent ~100 PRs, 17 runs reported a numeric result. Four of them, all based on
The other counts in that sample are 2 (×4), 13 (×3), and 5/10/14/16/19/20 once each. None of those four PRs shares a file with this one. How much of CI this change can reach. It is worth being precise, because the change is not inert in CI. Every build config in What can observe that change is the DeepEP cubin running on SM103 hardware. Across every Five tests cannot produce seventeen failures. What this does not settle. Matching counts are not matching tests — I am comparing numbers, not names. Some of the 17 could still be among those five, and the count argument only bounds the blast radius; it does not empty it. What would settle it, and it is cheap on your side: if you can paste the failing test list, or just say whether any of the five One small thing I noticed while tracing this. With |
|
Short follow-up to my last comment — the situation has moved, and it makes the ask smaller rather than larger. The 17-failure cluster is gone from
No run today reports 17, and #19595 went fully green. In between, #19572 and #19579 ( My run was pipeline #61657 on Sep 22 at 01:38 UTC, which predates all of that. So please disregard the request for the failure list unless it is convenient — a plain If the re-run comes back with failures again, then the list would genuinely help, and the five |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Semantic conflict reviewThe verdict of record is the Latest recorded state: No semantic conflict found (best effort) for head Best-effort AI judgment for the recorded revisions. PASS, FAIL and INCONCLUSIVE may be incomplete or incorrect. PR authors and reviewers should independently verify the evidence and relevant behavior. This semantic review and its status/workflow are advisory, not required merge checks under current repository rules; other merge requirements still apply. Advisory status does not make a confirmed defect safe to ignore.
Processed request and reply comments are minimized to reduce timeline noise; they remain expandable for audit. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Barry-Delaney
left a comment
There was a problem hiding this comment.
Reviewed 496e370. The entire major-10 family now follows the existing 100f path, while the other architecture branches remain unchanged. I executed the actual PR normalization loop for CMake 3.30/3.31 with unsuffixed, -real, and -virtual targets (6 matrix cases), including 80/90/100/103/107/110/120 inputs; all passed. git diff --check passed. No GPU build/runtime validation was performed locally; the existing older-CMake fallback remains unchanged.
Description
cpp/tensorrt_llm/deep_ep/CMakeLists.txtclassifies each entry ofCMAKE_CUDA_ARCHITECTURESand re-emits it for DeepEP. The comment directly above the branch states the requirement:The first branch requires
CUDA_ARCH_MINOR EQUAL 0, so it only matches100. Theelseifalso requiresCUDA_ARCH_MINOR EQUAL 0, so it only matches110,120, and laterx00architectures. Every other member of the 10x family therefore falls through to the finalelsebranch and is emitted with no suffix at all.The regex does not capture the suffix either —
[af]?sits outside the capture groups — so an incoming103a-realor107f-realloses its suffix on the way through.Observed with an unmodified checkout of
main(a76e0b2):CMAKE_CUDA_ARCHITECTURESentry90a-real90-real100f-real100f-real103a-real103-real107f-real107-real110f-real110a-real120f-real120a-real103and107are the two 10x architectures currently reachable throughcuda_configuration.cmake, and both end up with a bare target while their siblings do not.This PR drops the
CUDA_ARCH_MINOR EQUAL 0condition from the first branch so that the whole 10x major is routed to the existing100handling. That matches the stated intent in #17336 that100fkernels are meant to cover the whole10xfamily:Only
103and107change.90,100,110and120are unaffected.Note on older CMake: when
CMAKE_VERSION < 3.31the existing fallback emits100aand103abecause family targets cannot be expressed there. That limitation is unchanged by this PR;107simply follows the same path100already takes instead of getting a bare target.Test Coverage
The branch logic was extracted verbatim into a standalone script and executed with
cmake -Pagainst both the current and the proposed condition, over the architecture stringscuda_configuration.cmakecan produce. The table above is that output; with the change,103a-realand107f-realproduce the same result as100f-real, and every other input is byte-identical to current behavior.A full
build_wheel.pyconfigure with-a "90-real;100-real;103-real;107-real"was also run on the unmodified tree to capture the currentdeep_ep DEEP_EP_CUDA_ARCHITECTURES:line, which is what surfaced the bare107-realentry.I do not have Rubin hardware, so this PR makes no claim about runtime performance on SM107. It only restores the suffix that the surrounding comment requires.
PR Checklist
103, which currently also receives a bare target.Related: #19069 (closed — that approach was wrong, and the reason is written up there), #17336 (original SM107 recognition).
Dev Engineer Review
cpp/tensorrt_llm/deep_ep/CMakeLists.txtnow routes all CUDA major-version10architectures through SM100-family handling. This fixes suffix handling for architectures such as103a-realand107f-real. Architectures90,110, and120retain their existing paths. The change affects DeepEP and NVSHMEM CUDA architecture configuration only. No API, runtime, or performance changes are introduced.QA Engineer Review
No test changes.
Per-File QA Perspective
cpp/tensorrt_llm/deep_ep/CMakeLists.txt: Verify generatedDEEP_EP_CUDA_ARCHITECTURESvalues for100,103, and107inputs with-realand-virtualsuffixes. Confirm existing90,110, and120outputs remain unchanged.