[#12715][fix] disable NCCL_SYMMETRIC tactic on GB10 (DGX Spark) - #12902
Conversation
📝 WalkthroughWalkthroughAdded a cached GPU platform detection helper to identify GB10 devices. Modified Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/custom_ops/torch_custom_ops.py`:
- Around line 33-37: _is_gb10 currently calls torch.cuda.get_device_name() with
no device and caches a single global result, which can be wrong if the current
CUDA device changes or if CUDA is unavailable. Change _is_gb10 to accept an
optional device argument (e.g., def _is_gb10(device: Optional[int] = None) ->
bool), first check torch.cuda.is_available() and return False when CUDA is
unavailable, resolve device to torch.cuda.current_device() if None, call
torch.cuda.get_device_name(device) with that device id, and keep `@lru_cache` so
results are cached per-device; update call sites to pass the device when they
have a specific CUDA device context (or call with no arg to auto-resolve the
current device).
🪄 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: Pro
Run ID: 3f68adeb-1c67-4833-a08c-0d6d163cff41
📒 Files selected for processing (1)
tensorrt_llm/_torch/custom_ops/torch_custom_ops.py
|
Thanks for the quick fix! |
|
/bot run --disable-fail-fast |
|
PR_Github #42581 [ run ] triggered by Bot. Commit: |
|
PR_Github #42581 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #42715 [ run ] triggered by Bot. Commit: |
|
PR_Github #42716 [ run ] triggered by Bot. Commit: |
|
PR_Github #42715 [ run ] completed with state |
|
PR_Github #42716 [ run ] completed with state |
|
@nv-lschneider — I've been investigating the remaining hang after this PR's WAR fix. Root Cause AnalysisThe WAR correctly disables MNNVL Fabric Check Still Runs
EvidenceThe remaining hang occurs at the "checking fabric" step — after Proposed FixExtend this PR to also set @lru_cache(maxsize=None)
def _is_gb10() -> bool:
"""Return True on GB10 (DGX Spark). NCCL_SYMMETRIC and MNNVL fabric are unsupported."""
return "GB10" in torch.cuda.get_device_name()
# In initialization:
if _is_gb10():
os.environ["NCCL_MNNVL_ENABLE"] = "0" # No NVLink fabric on GB10
Verification
Happy to add this to the PR if it makes sense. |
Thanks for taking a deeper look into this. This helps quite a lot. I will also take the AI and check if this is a NCCL version specific problem, such that it can be resolved in future NCCL versions. Feel free to add your commits to my existing PR. |
f66399e to
80f126d
Compare
|
I tested the findings from @theherrovn to use the MNNVL disable switch and it works together with disabling NCCL_SYMMETRIC. Additionally, I verified that this is fixed in NCCL 2.30.4 and should be automatically fixed with the next update of the NCCL version. This PR submits a work around to set the env var and disables NCCL_SYMMETRIC on gb10 environments. This has been tested on a dual-spark setup. |
80f126d to
ef06316
Compare
|
/bot run |
|
PR_Github #52174 [ run ] triggered by Bot. Commit: |
|
PR_Github #52174 [ run ] completed with state
|
|
@nv-lschneider Great to hear the MNNVL disable switch works alongside the NCCL_SYMMETRIC fix on real dual-Spark hardware. And good to know NCCL 2.30.4 resolves this permanently — the WAR is the right approach until the upgrade lands. Thanks for testing and confirming! |
|
PR_Github #53382 [ run ] triggered by Bot. Commit: |
|
PR_Github #53382 [ run ] completed with state
|
80f91a2 to
d237242
Compare
|
/bot run |
|
PR_Github #53569 [ run ] triggered by Bot. Commit: |
|
PR_Github #53569 [ run ] completed with state
|
d237242 to
5b55103
Compare
|
/bot run |
|
PR_Github #53611 [ run ] triggered by Bot. Commit: |
|
PR_Github #53611 [ run ] completed with state
|
Signed-off-by: Ludwig Schneider <lschneider@nvidia.com>
NCCL_SYMMETRIC and NCCL MNNVL need to be disabled on GB10 when running affected NCCL runtimes. Query ncclGetVersion at runtime and only apply the autotuner filter plus NCCL_MNNVL_ENABLE=0 export for NCCL versions older than 2.30.4. NCCL already fixed this in 2.30.4 and newer, so upgraded installations keep NCCL_SYMMETRIC in the tactic set and do not receive the MNNVL env workaround. Credit to theherrovn for investigating the GB10 hang and identifying the NCCL_MNNVL_ENABLE=0 env export. Investigated-by: theherrovn Signed-off-by: Ludwig Schneider <lschneider@nvidia.com>
5b55103 to
fdd8d33
Compare
|
/bot run |
|
PR_Github #53659 [ run ] triggered by Bot. Commit: |
|
PR_Github #53659 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #53699 [ run ] triggered by Bot. Commit: |
|
PR_Github #53700 [ run ] triggered by Bot. Commit: |
|
PR_Github #53699 [ run ] completed with state |
|
PR_Github #53700 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #53917 [ run ] triggered by Bot. Commit: |
|
PR_Github #53917 [ run ] completed with state |
Summary by CodeRabbit
Bug Fixes
Performance
Description
This PR disables NCCL symmetric on spark compute notes.
#12715 reports that there is a failure on that platform.
Until the issue can be resolved from within NCCL this work around should enable the platform again without impacting other platforms.
We are investigating actively this failure in NCCL and will resolve it, when that happens we can remove this WAR again.
Test Coverage
I do not have access to a dual spark machine setup, so I need help with testing and verifying this fix.
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.