[#12715][fix] Disable NCCL window buffers on GB10#15559
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughNCCL window buffer support detection is refactored from an inline header implementation and a Python-side workaround into C++ helper functions with compile-time gating, runtime ChangesNCCL Window Buffer Support Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cpp/tensorrt_llm/common/ncclUtils.h (1)
167-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Doxygen comments for the new header API.
These declarations are externally visible, but the comments use plain
//, so generated API docs will not pick them up consistently.Documentation fix
-// Returns true if NCCL window buffers (ncclMemAlloc / ncclCommWindowRegister) -// are supported for the given real SM version, integrated-device flag, and runtime NCCL version. -// Exposed for focused unit testing of platform/version gates. +//! Returns true if NCCL window buffers (ncclMemAlloc / ncclCommWindowRegister) +//! are supported for the given real SM version, integrated-device flag, and runtime NCCL version. +//! Exposed for focused unit testing of platform/version gates. bool isNcclWindowSupportedForPlatform(int realSmVersion, bool isIntegrated, int ncclRuntimeVersion); -// Returns true if the compile-time and runtime NCCL versions support window buffers -// and the current CUDA device is not in a known-unsupported platform/version set. +//! Returns true if the compile-time and runtime NCCL versions support window buffers +//! and the current CUDA device is not in a known-unsupported platform/version set. bool isNcclWindowSupported();As per coding guidelines, “Follow Doxygen rules for documenting class interfaces and function prototypes; use
//!for C++ style comments and//!<for member comments.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/common/ncclUtils.h` around lines 167 - 174, The comment blocks for the public function declarations isNcclWindowSupportedForPlatform and isNcclWindowSupported are using plain C++ style comments (//), but Doxygen documentation requires the //! format to be picked up by documentation generation tools. Convert all plain // comments immediately preceding these two function declarations to use the //! format instead to ensure they are properly documented in generated API documentation.Source: Coding guidelines
cpp/tensorrt_llm/common/ncclUtils.cpp (1)
167-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the production gate through the tested helper.
Line 167 reconstructs the GB10 predicate instead of using
isNcclWindowSupportedForPlatform(), so the unit test can drift from production behavior. Reuse the helper after the version/platform queries.Refactor to keep production and tests coupled
- bool const supported = !isGb10Platform(realSmVersion, isIntegrated != 0); + bool const supported = isNcclWindowSupportedForPlatform( + realSmVersion, isIntegrated != 0, version);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/common/ncclUtils.cpp` around lines 167 - 191, The code at line 167 reconstructs the GB10 platform predicate logic directly instead of reusing the isNcclWindowSupportedForPlatform() helper function, which causes production and test logic to diverge. Replace the line that creates the supported variable by calling isNcclWindowSupportedForPlatform(realSmVersion, isIntegrated != 0, version) and using its return value directly, ensuring the helper function is invoked after version validation checks to keep the production gate logic centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tensorrt_llm/common/ncclUtils.cpp`:
- Around line 131-164: The isNcclWindowSupported() function makes local
decisions about NCCL window support based on CUDA queries that may fail
independently on different ranks, causing those ranks to return different
values. When requestBuffer() returns early at line 405 based on this decision,
some ranks may skip reaching the collective ncclAllReduce at line 574, causing
other ranks to deadlock waiting on it. To fix this, add a cross-rank
synchronization barrier before the early return in requestBuffer() at line 405
to ensure all ranks agree on the NCCL window support decision, or alternatively
move the synchronization logic into isNcclWindowSupported() itself so the
consensus is reached before any rank returns.
---
Nitpick comments:
In `@cpp/tensorrt_llm/common/ncclUtils.cpp`:
- Around line 167-191: The code at line 167 reconstructs the GB10 platform
predicate logic directly instead of reusing the
isNcclWindowSupportedForPlatform() helper function, which causes production and
test logic to diverge. Replace the line that creates the supported variable by
calling isNcclWindowSupportedForPlatform(realSmVersion, isIntegrated != 0,
version) and using its return value directly, ensuring the helper function is
invoked after version validation checks to keep the production gate logic
centralized.
In `@cpp/tensorrt_llm/common/ncclUtils.h`:
- Around line 167-174: The comment blocks for the public function declarations
isNcclWindowSupportedForPlatform and isNcclWindowSupported are using plain C++
style comments (//), but Doxygen documentation requires the //! format to be
picked up by documentation generation tools. Convert all plain // comments
immediately preceding these two function declarations to use the //! format
instead to ensure they are properly documented in generated API documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8a24e9d6-847c-48eb-ac0a-8f99c9c56d18
📒 Files selected for processing (5)
cpp/tensorrt_llm/common/ncclUtils.cppcpp/tensorrt_llm/common/ncclUtils.hcpp/tests/unit_tests/multi_gpu/ncclUtilsTest.cpptensorrt_llm/_torch/custom_ops/torch_custom_ops.pytensorrt_llm/_torch/distributed/ops.py
💤 Files with no reviewable changes (1)
- tensorrt_llm/_torch/distributed/ops.py
Signed-off-by: Ludwig Schneider <lschneider@nvidia.com>
nv-lschneider
force-pushed
the
fix/gb10-nccl-window-guard
branch
from
June 23, 2026 22:23
6c4718c to
004ac9b
Compare
nv-lschneider
changed the title
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #55334 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #55334 [ run ] completed with state
|
Collaborator
Author
|
/bot run --disable-fail-fast |
5 tasks
Collaborator
|
PR_Github #55510 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #55510 [ run ] completed with state |
1 task
BrianLi23
pushed a commit
to BrianLi23/TensorRT-LLM
that referenced
this pull request
Jul 9, 2026
Signed-off-by: Ludwig Schneider <lschneider@nvidia.com>
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
3 participants
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.
Summary by CodeRabbit
Release Notes
Improvements
Tests
Description
PR #12902 avoided the NCCL symmetric allreduce path, but the same unsupported NCCL window registration could still be reached through other callers, including the fused GEMM output path:
FP4GemmRunner::runGemm -> createNCCLWindowTensor -> ncclCommGroupRegisterSymmetricThis change makes the workaround more holistic by guarding the native NCCL window allocator itself. On GB10/integrated SM 121 platforms with NCCL runtime versions older than 2.30.4, TRT-LLM now skips NCCL window buffer registration and falls back to regular tensors. The platform/version check is cached process-wide, so the normal path remains cheap after the first query,
and newer NCCL versions avoid the platform probe entirely.
The Python-side GB10 workaround is removed so the behavior is centralized in the C++ NCCL window path used by all callers.
Test Coverage
Added unit coverage for the GB10/NCCL runtime-version gate in
cpp/tests/unit_tests/multi_gpu/ncclUtilsTest.cpp.Verified
git diff --check.Verified Python syntax with
python3 -m py_compilefor the edited Python files.Reproduced the crash on unpatched rc18 with dual-node serving:
ncclCommGroupRegisterSymmetric.Verified patched rc18 dual-node serving:
Please check this after reviewing the above items as appropriate for this PR.