[https://nvbugs/6369411][fix] Port commit 21ea62f24a: add _get_nccl_runtime_version_code() + lru-cached…#15672
Open
tensorrt-cicd wants to merge 3 commits into
Open
[https://nvbugs/6369411][fix] Port commit 21ea62f24a: add _get_nccl_runtime_version_code() + lru-cached…#15672tensorrt-cicd wants to merge 3 commits into
tensorrt-cicd wants to merge 3 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds NCCL runtime detection and an AllReduce constructor hook that disables NVLS/MNNVL on affected platform/version combinations before initialization. Also updates VERL integration install commands to use guarded builds, sudo-based installs, a user install for DeepEP, and revised symlink creation. ChangesNCCL initialization workaround
VERL install command updates
Sequence Diagram(s)sequenceDiagram
participant AR as "AllReduce.__init__"
participant Init as "_init_nccl_init_workaround()"
participant Runtime as "_get_nccl_runtime_version_code()"
participant NCCL as "libnccl"
participant CUDA as "torch.cuda.get_device_name()"
participant Env as "os.environ"
AR->>Init: call before preallocation config
Init->>Runtime: read NCCL runtime version
Runtime->>NCCL: load libnccl and call ncclGetVersion
Init->>CUDA: read device name
Init->>Env: set NCCL_MNNVL_ENABLE / NCCL_NVLS_ENABLE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🤖 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 `@tensorrt_llm/_torch/custom_ops/torch_custom_ops.py`:
- Around line 100-109: The NCCL transport workaround in
_get_nccl_runtime_version_code gating is too broad because a None runtime
version currently falls through and still disables
NCCL_MNNVL_ENABLE/NCCL_NVLS_ENABLE on matching hosts. Update the logic in the
custom ops initialization so the workaround only applies when a known affected
NCCL version is detected, either by adding a reliable fallback to determine the
runtime NCCL version or by explicitly skipping the environment overrides when
the version cannot be resolved.
In `@tests/integration/defs/verl/verl_config.yml`:
- Around line 19-21: The nvshmem install/symlink setup in verl_config.yml is
hard-coded to a specific site-packages path, which can break when the active
Python interpreter differs. Update the setup to derive the nvshmem location from
the same interpreter used by pip/pip3, and use that resolved path when creating
the symlink and setting NVSHMEM_DIR. Also add a small validation in
test_verl_cases.py to verify the resolved nvshmem path before running the
commands, using the existing verl_config and test_verl_cases symbols to locate
the change.
🪄 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: c316d3b9-a0a0-4c48-b5a5-35269e681482
📒 Files selected for processing (3)
tensorrt_llm/_torch/custom_ops/torch_custom_ops.pytensorrt_llm/_torch/distributed/ops.pytests/integration/defs/verl/verl_config.yml
Comment on lines
+100
to
+109
| runtime_version = _get_nccl_runtime_version_code() | ||
| if runtime_version is not None and runtime_version >= 23004: # NCCL 2.30.4 | ||
| return | ||
| on_x86_64 = platform.machine() == "x86_64" | ||
| is_gb10 = not on_x86_64 and "GB10" in torch.cuda.get_device_name() | ||
| if not (on_x86_64 or is_gb10): | ||
| return | ||
| os.environ.setdefault("NCCL_MNNVL_ENABLE", "0") | ||
| if on_x86_64: | ||
| os.environ.setdefault("NCCL_NVLS_ENABLE", "0") |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep the workaround gated on a known affected NCCL version.
When _get_nccl_runtime_version_code() returns None, Line 101 falls through and Lines 107-109 still set NCCL_MNNVL_ENABLE / NCCL_NVLS_ENABLE on matching hosts. That makes detection failure behave like “old NCCL,” so NCCL 2.30.4+ deployments can have transports disabled just because libnccl was not resolved by this helper. Please either add a reliable fallback for the actual runtime NCCL version or skip the workaround when the version is unknown.
🤖 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 `@tensorrt_llm/_torch/custom_ops/torch_custom_ops.py` around lines 100 - 109,
The NCCL transport workaround in _get_nccl_runtime_version_code gating is too
broad because a None runtime version currently falls through and still disables
NCCL_MNNVL_ENABLE/NCCL_NVLS_ENABLE on matching hosts. Update the logic in the
custom ops initialization so the workaround only applies when a known affected
NCCL version is detected, either by adding a reliable fallback to determine the
runtime NCCL version or by explicitly skipping the environment overrides when
the version cannot be resolved.
Comment on lines
+19
to
+21
| - "sudo -H pip install nvidia-nvshmem-cu13==3.3.20" | ||
| # Create nvshmem symlink (needed before DeepEP build) | ||
| - >- | ||
| (cd /usr/local/lib/python3.12/dist-packages/nvidia/nvshmem/lib && | ||
| ln -s libnvshmem_host.so.3 libnvshmem_host.so) | ||
| # Install DeepEP | ||
| - "sudo ln -sf libnvshmem_host.so.3 /usr/local/lib/python3.12/dist-packages/nvidia/nvshmem/lib/libnvshmem_host.so" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the hard-coded path matches the active interpreter in the test image.
python3 - <<'PY'
import site
import sys
print("python:", sys.executable)
print("site_packages:", site.getsitepackages())
print("user_site:", site.getusersitepackages())
PY
grep -n '/usr/local/lib/python3.12/dist-packages' tests/integration/defs/verl/verl_config.ymlRepository: NVIDIA/TensorRT-LLM
Length of output: 574
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the changed config and the consuming test to see whether the path is derived or asserted anywhere.
git ls-files tests/integration/defs/verl/verl_config.yml tests/integration/defs/verl/test_verl_cases.py
printf '\n--- verl_config.yml ---\n'
cat -n tests/integration/defs/verl/verl_config.yml | sed -n '1,120p'
printf '\n--- test_verl_cases.py outline ---\n'
ast-grep outline tests/integration/defs/verl/test_verl_cases.py --view expanded
printf '\n--- references to nvshmem / dist-packages / NVSHMEM_DIR ---\n'
rg -n "nvshmem|dist-packages|NVSHMEM_DIR|pip3?|python setup.py install" tests/integration/defs/verlRepository: NVIDIA/TensorRT-LLM
Length of output: 6953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the changed config and the consuming test to see whether the path is derived or asserted anywhere.
git ls-files tests/integration/defs/verl/verl_config.yml tests/integration/defs/verl/test_verl_cases.py
printf '\n--- verl_config.yml ---\n'
cat -n tests/integration/defs/verl/verl_config.yml | sed -n '1,120p'
printf '\n--- test_verl_cases.py outline ---\n'
ast-grep outline tests/integration/defs/verl/test_verl_cases.py --view expanded
printf '\n--- references to nvshmem / dist-packages / NVSHMEM_DIR ---\n'
rg -n "nvshmem|dist-packages|NVSHMEM_DIR|pip3?|python setup.py install" tests/integration/defs/verlRepository: NVIDIA/TensorRT-LLM
Length of output: 6953
Derive the nvshmem path from the active interpreter. tests/integration/defs/verl/verl_config.yml hard-codes /usr/local/lib/python3.12/dist-packages, but the setup uses pip/pip3 and tests/integration/defs/verl/test_verl_cases.py runs these commands directly, so the symlink and NVSHMEM_DIR can point at the wrong site-packages directory when the container’s Python version differs. Use the same interpreter for the install and path resolution, and add a small check in tests/integration/defs/verl/test_verl_cases.py for the resolved nvshmem path.
🤖 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 `@tests/integration/defs/verl/verl_config.yml` around lines 19 - 21, The
nvshmem install/symlink setup in verl_config.yml is hard-coded to a specific
site-packages path, which can break when the active Python interpreter differs.
Update the setup to derive the nvshmem location from the same interpreter used
by pip/pip3, and use that resolved path when creating the symlink and setting
NVSHMEM_DIR. Also add a small validation in test_verl_cases.py to verify the
resolved nvshmem path before running the commands, using the existing
verl_config and test_verl_cases symbols to locate the change.
Source: Path instructions
trtllm-agent
force-pushed
the
repair-bot-bug6369411
branch
from
bd835bc to
ad7eb8d
Compare
… for verl inter-node The verl test_inter_node_trtllm_rollout test hung on B200 at NCCL communicator init. The last log line was "TP group is intra-node for rank 0" from allreduceOp.cpp::setGroupTopology, immediately before getComm() -> ncclCommInitRank() is called. This is the same root cause already fixed for sibling bugs 6276923 (B200), 6322076 (B300), 6345827 (H20 disagg) and 6373530 (H20 GPT-OSS): on x86_64 hosts without an MNNVL/IMEX fabric, NCCL <2.30.4's static-connection setup for NVLS/MNNVL transports (forced by NCCL_RUNTIME_CONNECT=0 in opUtils.cpp::getComm) hangs/errors during ncclCommInitRank. Port the canonical fix: - Add _get_nccl_runtime_version_code() that resolves libnccl's ncclGetVersion via ctypes. - Add _init_nccl_init_workaround() (lru_cached) gated on NCCL <2.30.4 AND (x86_64 OR GB10) that sets NCCL_MNNVL_ENABLE=0 and (on x86_64) NCCL_NVLS_ENABLE=0 via os.environ.setdefault, honoring any user-supplied values. No-op on NCCL >= 2.30.4. - Call _init_nccl_init_workaround() in AllReduce.__init__ in _torch/distributed/ops.py — the earliest Python entry point that precedes ncclCommInitRank for both the autotuner and executor paths. Also make the verl_config.yml install_commands portable across user contexts (root in CI, LOCAL_USER=1 elsewhere): skip-if-installed guard for gdrcopy with split build/install (sudo for the install step), sudo -H for pip lines, absolute-path sudo ln for the nvshmem symlink, and --user for DeepEP setup.py (NFS root_squash on /code blocks sudo writes). These are no-ops when run as root and let the test setup run consistently under both user contexts. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com>
…P -dlink Follow-up to 8a1c39e. That commit correctly ports the canonical NCCL <2.30.4 NVLS/MNNVL init workaround for the verl inter-node test hang at ncclCommInitRank on x86_64 hosts without an MNNVL/IMEX fabric, but the verify run for that fix could not reach LLM init: it died in the verl_setup pytest fixture at the DeepEP setup.py install step with sm_100 nvlink errors: nvlink error : Undefined reference to '_Z30nvshmemi_transfer_amo_nonfetch...' nvlink error : Undefined reference to 'nvshmemi_device_state_d' ... The system nvshmem tree at /usr/local/cuda/targets/x86_64-linux/lib ships only host libs (symlinks to /usr/lib/x86_64-linux-gnu/nvshmem/13/), so DeepEP's -dlink step cannot resolve nvshmem device symbols there. The pip-installed nvidia-nvshmem-cu13==3.3.20 wheel does ship libnvshmem_device.a alongside libnvshmem_host.so.3, so point NVSHMEM_DIR at the pip site-packages tree so DeepEP's device link finds the archive. Create the libnvshmem_host.so symlink at that location with sudo (the site-packages tree is root-owned). Keep LD_LIBRARY_PATH pointing at /usr/local/cuda/targets/x86_64-linux/lib so torch's libtorch_nvshmem.so still resolves against the v3.6.5 host lib the base image ships -- torch was linked against a versioned symbol (nvshmem_selected_device_transport, v3.6.5) that the pip v3.3.20 host lib does not export, and preempting it via LD_LIBRARY_PATH breaks torch import with an undefined-symbol ImportError. Also add RAY_enable_open_telemetry_on_core_worker=0, needed by Ray in this container (identical env var set by the sibling verl-test fix in 0565b4d [nvbugs/6221457]). This is the same fix already validated on a sibling branch as 0e86ec7 for nvbugs 6221457; it applies unchanged here to unblock the 6369411 verify. The NCCL init workaround from 8a1c39e is retained untouched. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
… user contexts The prior fixes on this bug (b0b4636: NCCL NVLS/MNNVL init workaround, ab43ba9: NVSHMEM_DIR at pip nvshmem tree) address the CI failure at ncclCommInitRank, but the verl_setup pytest fixture dies before reaching that point when the test runs outside root-CI: 1. install_commands wrote to /usr/local/lib and root-owned dist-packages without sudo, so `make prefix=/usr/local lib_install` in the gdrcopy step fails EACCES under LOCAL_USER=1. Port the portable-install pattern already used for sibling verl bug 6221457: skip-if-installed guard on gdrcopy with split build (as user) + `sudo make lib_install` (root), `sudo -H` on system-wide pip installs, `--user` for DeepEP setup.py (the /code source tree is NFS root_squash so sudo writes to it would be blocked). All sudo calls are no-ops when the setup runs as root. 2. _setup_model_symlinks silently skipped every symlink when the hardcoded ci_model_cache path /scratch.trt_llm_data/llm-models was unavailable, leaving /tmp/verl-models/Qwen/* missing and the test dying with a confusing HFValidationError from transformers. Fall back to the standard LLM_MODELS_ROOT env var (already used by all other TRT-LLM integration tests) when the configured cache is missing, so the symlinks get created against the mount that actually exists on the current host. Signed-off-by: tensorrt-cicd <90828364+tensorrt-cicd@users.noreply.github.com> Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
trtllm-agent
force-pushed
the
repair-bot-bug6369411
branch
from
ad7eb8d to
0bdd207
Compare
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
Test plan
Links
Summary by CodeRabbit
Bug Fixes
Chores