Repository navigation
Conversation
…ngrade TRT-LLM's requirements-dev.txt pins nvidia-nccl-cu13<=2.28.9,>=2.27.7 which downgrades the NCCL we installed. CMake then finds the downgraded header (2.27.7) and the NCCL_VERSION gate hides NCCLWindowAllocator. Fix: move NCCL install + symlink setup to AFTER requirements-dev.txt, and use --force-reinstall to ensure 2.28+ is the final version. Also add NCCL version diagnostics to catch this in the future. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
📝 WalkthroughWalkthroughReplaces the previous NCCL 2.27-focused CI flow by enforcing/installing NCCL >=2.28, creating a stable NCCL_ROOT with headers/libs, patching CMake find-modules, cloning TRT-LLM with Git LFS, building and caching the wheel, and adding diagnostics and verification. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Runner
participant GH as GitHub (TRT-LLM)
participant Python as Python/pip
participant NCCL as NCCL (pip & system)
participant Build as Build Environment (cmake/make)
CI->>GH: git clone TRT-LLM (with LFS)
CI->>Python: pip install -r requirements (incl. nvidia-cutlass)
CI->>NCCL: ensure NCCL >= 2.28 (pip reinstall + copy headers/libs -> /tmp/nccl-stable)
CI->>NCCL: create `/tmp/nccl-stable/libnccl.so` symlink and symlink `nccl.h` -> /usr/include/nccl.h
CI->>Build: apply patched FindTensorRT.cmake & FindNCCL.cmake (use NCCL_ROOT)
CI->>Build: build TRT-LLM wheel from source
Build->>CI: cache built wheel, update LD_LIBRARY_PATH
CI->>NCCL: run NCCL diagnostics & version checks
CI->>Python: pip install built TRT-LLM wheel
CI->>CI: run post-build verification (gRPC serve check)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a CI build failure in the TensorRT-LLM pipeline by addressing a dependency conflict related to NCCL. The changes ensure that the correct NCCL version (2.28+) is consistently installed and not inadvertently downgraded by other project requirements, which was previously causing issues with Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a CI build failure by reordering the NCCL installation to occur after the TensorRT-LLM requirements are installed, preventing an unintended downgrade. The use of --force-reinstall is appropriate, and the addition of diagnostic checks for the NCCL version is a valuable improvement for future debugging. I have one minor suggestion to enhance the portability of the new diagnostic command.
| ls -la "$NCCL_ROOT/include/" 2>/dev/null || echo "WARNING: NCCL include not found" | ||
| ls -la "$NCCL_ROOT/lib/" 2>/dev/null || echo "WARNING: NCCL lib not found" | ||
| # Verify NCCL version in header matches what we need | ||
| grep "NCCL_MAJOR\|NCCL_MINOR" "$NCCL_ROOT/include/nccl.h" 2>/dev/null | head -3 |
There was a problem hiding this comment.
The use of \| for alternation in grep is a GNU extension for Basic Regular Expressions. For better portability and clarity across different CI environments, it's recommended to use the POSIX standard Extended Regular Expressions (-E).
Additionally, using head -n 2 is more precise if you only need to verify NCCL_MAJOR and NCCL_MINOR, as it will limit the output to the first two matching lines.
| grep "NCCL_MAJOR\|NCCL_MINOR" "$NCCL_ROOT/include/nccl.h" 2>/dev/null | head -3 | |
| grep -E "NCCL_MAJOR|NCCL_MINOR" "$NCCL_ROOT/include/nccl.h" 2>/dev/null | head -n 2 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432368e8e2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # MUST run AFTER requirements-dev.txt which pins nvidia-nccl-cu13<=2.28.9. | ||
| # TRT-LLM PR #12015 requires NCCL 2.28+ for NCCLWindowAllocator. | ||
| # Force-reinstall to override any version TRT-LLM requirements pulled in. | ||
| pip install --no-cache-dir --force-reinstall "nvidia-nccl-cu13>=2.28.0" |
There was a problem hiding this comment.
This forced reinstall uses nvidia-nccl-cu13>=2.28.0 with no upper bound, even though the script notes requirements-dev.txt pins NCCL to <=2.28.9; as soon as PyPI publishes a newer major/minor (for example 2.29+), this line will upgrade beyond the range TRT-LLM currently declares compatible and can reintroduce build/runtime breakage in CI. Keep the post-requirements reinstall, but cap it to the same supported range so we prevent downgrades without drifting to unvalidated NCCL versions.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci_install_trtllm.sh`:
- Around line 198-199: Replace the diagnostic-only grep of NCCL macros with an
explicit numeric check: ensure the header at $NCCL_ROOT/include/nccl.h exists,
parse NCCL_MAJOR and NCCL_MINOR from that nccl.h (the same macros the grep
showed), compute a numeric comparison for >= 2.28 (for example major > 2 or
major == 2 and minor >= 28), and if the check fails, print a clear error and
exit non‑zero; perform this validation at the same spot where the current grep
is executed so downstream steps using $NCCL_ROOT fail fast.
- Around line 163-170: The script currently only clones TensorRT-LLM into
$TRTLLM_DIR if the directory is missing, which can reuse stale sources; update
the logic around $TRTLLM_DIR so each CI run refreshes or pins the checkout: if
the directory exists, run a git fetch and reset to the desired ref (e.g.,
origin/main or a provided pinned commit SHA) and then run git lfs install
--force and git lfs pull; if it does not exist, perform a fresh git clone
(optionally with --depth 1 and --branch main) followed by git lfs install
--force and git lfs pull; ensure the script uses the same variable TRTLLM_DIR
and supports pinning via an env var (e.g., TRTLLM_COMMIT) so CI can
deterministically build a specific commit.
- Around line 182-187: The pip install step currently uses an open-ended NCCL
spec ("nvidia-nccl-cu13>=2.28.0"); change it to the validated, bounded range
used by TRT-LLM (nvidia-nccl-cu13>=2.27.7,<=2.28.9) wherever that package is
installed in the script (the pip install line around the NCCL setup block and
any duplicate occurrences), ensuring the force-reinstall call retains the same
options and the comment about pinned versions remains accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24b2e506-fbb5-4864-be4a-b2f9d35e289d
📒 Files selected for processing (1)
scripts/ci_install_trtllm.sh
| if [ ! -d "$TRTLLM_DIR" ]; then | ||
| echo "Cloning TensorRT-LLM main branch..." | ||
| git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR" | ||
| fi | ||
|
|
||
| cd "$TRTLLM_DIR" | ||
| git lfs install --force | ||
| git lfs pull |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the script refreshes/pins TensorRT-LLM source each run.
rg -nP 'TRTLLM_DIR|git clone|git -C .*fetch|git -C .*checkout|TRTLLM_COMMIT|TRTLLM_REF' scripts/ci_install_trtllm.shRepository: lightseekorg/smg
Length of output: 332
🏁 Script executed:
sed -n '160,175p' scripts/ci_install_trtllm.sh | cat -nRepository: lightseekorg/smg
Length of output: 814
🏁 Script executed:
head -200 scripts/ci_install_trtllm.sh | grep -n -E 'TRTLLM_DIR|rm -rf|rm -r|clean|/tmp/' | head -20Repository: lightseekorg/smg
Length of output: 837
🏁 Script executed:
grep -n 'TRTLLM_DIR' scripts/ci_install_trtllm.shRepository: lightseekorg/smg
Length of output: 332
Refresh or pin the TensorRT-LLM checkout each run.
Lines 163-166 only clone when the directory is missing. Reusing /tmp/tensorrt-llm-src without refresh can build stale code and make CI nondeterministic. Running git lfs pull alone does not fetch new commits from main.
Suggested fix
TRTLLM_DIR="/tmp/tensorrt-llm-src"
-if [ ! -d "$TRTLLM_DIR" ]; then
- echo "Cloning TensorRT-LLM main branch..."
- git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR"
-fi
+TRTLLM_COMMIT="${TRTLLM_COMMIT:-origin/main}" # override with a commit SHA for reproducible builds
+if [ ! -d "$TRTLLM_DIR/.git" ]; then
+ rm -rf "$TRTLLM_DIR"
+ echo "Cloning TensorRT-LLM repository..."
+ git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR"
+fi
+git -C "$TRTLLM_DIR" fetch --depth 1 origin main
+git -C "$TRTLLM_DIR" checkout --detach "$TRTLLM_COMMIT"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -d "$TRTLLM_DIR" ]; then | |
| echo "Cloning TensorRT-LLM main branch..." | |
| git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR" | |
| fi | |
| cd "$TRTLLM_DIR" | |
| git lfs install --force | |
| git lfs pull | |
| TRTLLM_DIR="/tmp/tensorrt-llm-src" | |
| TRTLLM_COMMIT="${TRTLLM_COMMIT:-origin/main}" # override with a commit SHA for reproducible builds | |
| if [ ! -d "$TRTLLM_DIR/.git" ]; then | |
| rm -rf "$TRTLLM_DIR" | |
| echo "Cloning TensorRT-LLM repository..." | |
| git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR" | |
| fi | |
| git -C "$TRTLLM_DIR" fetch --depth 1 origin main | |
| git -C "$TRTLLM_DIR" checkout --detach "$TRTLLM_COMMIT" | |
| cd "$TRTLLM_DIR" | |
| git lfs install --force | |
| git lfs pull |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_trtllm.sh` around lines 163 - 170, The script currently
only clones TensorRT-LLM into $TRTLLM_DIR if the directory is missing, which can
reuse stale sources; update the logic around $TRTLLM_DIR so each CI run
refreshes or pins the checkout: if the directory exists, run a git fetch and
reset to the desired ref (e.g., origin/main or a provided pinned commit SHA) and
then run git lfs install --force and git lfs pull; if it does not exist, perform
a fresh git clone (optionally with --depth 1 and --branch main) followed by git
lfs install --force and git lfs pull; ensure the script uses the same variable
TRTLLM_DIR and supports pinning via an env var (e.g., TRTLLM_COMMIT) so CI can
deterministically build a specific commit.
| # ── NCCL 2.28+ setup ──────────────────────────────────────────────────────── | ||
| # MUST run AFTER requirements-dev.txt which pins nvidia-nccl-cu13<=2.28.9. | ||
| # TRT-LLM PR #12015 requires NCCL 2.28+ for NCCLWindowAllocator. | ||
| # Force-reinstall to override any version TRT-LLM requirements pulled in. | ||
| pip install --no-cache-dir --force-reinstall "nvidia-nccl-cu13>=2.28.0" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For TensorRT-LLM on CUDA 13, what exact nvidia-nccl-cu13 version range is officially tested/supported (including any upper bound)?
💡 Result:
For the official TensorRT‑LLM PyPI package (v1.2.0, released 2026‑03‑12), the declared/officially supported & tested dependency constraint for CUDA 13 is:
nvidia-nccl-cu13 >= 2.27.7, <= 2.28.9(i.e., [2.27.7, 2.28.9] inclusive). [1]
🏁 Script executed:
# Check the actual file content around lines 182-187
cat -n scripts/ci_install_trtllm.sh | sed -n '175,195p'Repository: lightseekorg/smg
Length of output: 1096
🏁 Script executed:
# Search for any TRT-LLM version constraints in the script
rg -A5 -B5 "trt-llm|tensorrt-llm" scripts/ci_install_trtllm.sh -iRepository: lightseekorg/smg
Length of output: 4466
🏁 Script executed:
# Check if there are any other NCCL version constraints in the script
rg -A2 -B2 "nccl" scripts/ci_install_trtllm.sh -iRepository: lightseekorg/smg
Length of output: 4562
Use a bounded NCCL version constraint aligned with TRT-LLM's tested range.
Line 186 uses nvidia-nccl-cu13>=2.28.0 without an upper bound, risking incompatibility with future versions. The comment on line 183 even states the requirement-dev.txt pins <=2.28.9, yet the pip install command contradicts this by using an open-ended range. Official TensorRT-LLM v1.2.0 (2026-03-12) declares tested support for nvidia-nccl-cu13>=2.27.7,<=2.28.9. Replace the unbounded constraint with this validated range:
Suggested fix
-pip install --no-cache-dir --force-reinstall "nvidia-nccl-cu13>=2.28.0"
+pip install --no-cache-dir --force-reinstall "nvidia-nccl-cu13>=2.28.0,<2.29.0"(Also applies to the same unbounded constraint used elsewhere in the script.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_trtllm.sh` around lines 182 - 187, The pip install step
currently uses an open-ended NCCL spec ("nvidia-nccl-cu13>=2.28.0"); change it
to the validated, bounded range used by TRT-LLM
(nvidia-nccl-cu13>=2.27.7,<=2.28.9) wherever that package is installed in the
script (the pip install line around the NCCL setup block and any duplicate
occurrences), ensuring the force-reinstall call retains the same options and the
comment about pinned versions remains accurate.
| # Verify NCCL version in header matches what we need | ||
| grep "NCCL_MAJOR\|NCCL_MINOR" "$NCCL_ROOT/include/nccl.h" 2>/dev/null | head -3 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current behavior: diagnostics print macros but no numeric threshold enforcement.
rg -nP 'set -euo pipefail|NCCL_MAJOR|NCCL_MINOR|grep "NCCL_MAJOR\\\|NCCL_MINOR"|awk .*(NCCL_MAJOR|NCCL_MINOR)|>= 2\.28' scripts/ci_install_trtllm.shRepository: lightseekorg/smg
Length of output: 164
🏁 Script executed:
cat -n scripts/ci_install_trtllm.sh | sed -n '185,215p'Repository: lightseekorg/smg
Length of output: 1759
🏁 Script executed:
rg -n "NCCL_MAJOR|NCCL_MINOR|version|2\.28" scripts/ci_install_trtllm.sh -C 3Repository: lightseekorg/smg
Length of output: 3280
🏁 Script executed:
wc -l scripts/ci_install_trtllm.shRepository: lightseekorg/smg
Length of output: 92
Add explicit NCCL version validation rather than relying solely on pip's version constraint.
Line 199 is a diagnostic-only grep that prints the macros but does not enforce the >=2.28 requirement. While the pip install commands at lines 61 and 186 do enforce nvidia-nccl-cu13>=2.28.0, adding an explicit numeric version check at the point where $NCCL_ROOT is used improves fail-fast behavior and makes the constraint locally visible. This prevents silent configuration issues if the installed package somehow deviates from expectations.
The suggested diff provides a robust check that:
- Validates the header file exists
- Extracts NCCL_MAJOR and NCCL_MINOR values
- Fails immediately with a clear error if the version is below 2.28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_trtllm.sh` around lines 198 - 199, Replace the
diagnostic-only grep of NCCL macros with an explicit numeric check: ensure the
header at $NCCL_ROOT/include/nccl.h exists, parse NCCL_MAJOR and NCCL_MINOR from
that nccl.h (the same macros the grep showed), compute a numeric comparison for
>= 2.28 (for example major > 2 or major == 2 and minor >= 28), and if the check
fails, print a clear error and exit non‑zero; perform this validation at the
same spot where the current grep is executed so downstream steps using
$NCCL_ROOT fail fast.
…build build_wheel.py internally runs pip install -r requirements.txt which pins nvidia-nccl-cu13<=2.28.9,>=2.27.7, downgrading from 2.29.7 to 2.27.7. This undoes our force-reinstall and CMake finds 2.27 headers. Fix: sed-patch the NCCL constraint in requirements.txt and requirements-dev.txt before the build to allow >=2.28.0. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ce1cca1bc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # script internally runs pip install, so we patch the constraint in-place. | ||
| for req_file in requirements.txt requirements-dev.txt; do | ||
| if [ -f "$req_file" ]; then | ||
| sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file" |
There was a problem hiding this comment.
This replacement removes TRT-LLM’s original nvidia-nccl-cu13<=2.28.9,>=2.27.7 cap and leaves an unbounded >=2.28.0 constraint, and the comment immediately above notes build_wheel.py installs from these requirement files. That means once PyPI publishes newer NCCL releases (e.g. 2.29+), CI can start resolving to unvalidated versions during wheel build and reintroduce compatibility breakage. Keep the downgrade-prevention patch, but retain an explicit upper bound in the rewritten constraint.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
scripts/ci_install_trtllm.sh (2)
163-170:⚠️ Potential issue | 🟠 MajorRefresh or pin the TensorRT-LLM checkout each run.
Line 163 only clones when missing, so stale code in
/tmp/tensorrt-llm-srccan be reused across CI runs. Fetch + checkout a target ref/SHA before build to keep runs deterministic.Suggested fix
TRTLLM_DIR="/tmp/tensorrt-llm-src" -if [ ! -d "$TRTLLM_DIR" ]; then - echo "Cloning TensorRT-LLM main branch..." - git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR" -fi - -cd "$TRTLLM_DIR" -git lfs install --force -git lfs pull +TRTLLM_REF="${TRTLLM_REF:-origin/main}" # can be overridden with a pinned SHA +if [ ! -d "$TRTLLM_DIR/.git" ]; then + rm -rf "$TRTLLM_DIR" + echo "Cloning TensorRT-LLM repository..." + git clone --depth 1 https://github.com/NVIDIA/TensorRT-LLM.git "$TRTLLM_DIR" +fi +git -C "$TRTLLM_DIR" fetch --depth 1 origin main +git -C "$TRTLLM_DIR" checkout --detach "$TRTLLM_REF" +git -C "$TRTLLM_DIR" lfs install --force +git -C "$TRTLLM_DIR" lfs pull +cd "$TRTLLM_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci_install_trtllm.sh` around lines 163 - 170, The script currently only clones TensorRT-LLM if $TRTLLM_DIR is missing, allowing stale checkout reuse; change the logic around the clone/checkout step (the block using TRTLLM_DIR plus the subsequent git lfs and pull commands) so that after ensuring the repo exists you explicitly fetch and check out a deterministic ref (use an env var like TRTLLM_REF or a pinned SHA), e.g., run git fetch --all and then git checkout <TRTLLM_REF_or_SHA> followed by git reset --hard <TRTLLM_REF_or_SHA> (or git reset --hard origin/<ref>) before running git lfs install/pull to guarantee each CI run uses the intended commit.
198-199:⚠️ Potential issue | 🟡 MinorMake NCCL validation fail-fast, not diagnostic-only.
Lines 198-199 print macros but do not enforce
>= 2.28. Add a numeric check so misresolved headers fail immediately before build.#!/bin/bash # Verify whether this script currently enforces NCCL >= 2.28 numerically. rg -nP 'NCCL_MAJOR|NCCL_MINOR|grep "NCCL_MAJOR\\\|NCCL_MINOR"|minor.*-lt 28|major.*-gt 2|major.*-eq 2' scripts/ci_install_trtllm.sh -C2Suggested fix
-# Verify NCCL version in header matches what we need -grep "NCCL_MAJOR\|NCCL_MINOR" "$NCCL_ROOT/include/nccl.h" 2>/dev/null | head -3 +# Verify NCCL version in header matches what we need +if [ ! -f "$NCCL_ROOT/include/nccl.h" ]; then + echo "ERROR: Missing NCCL header at $NCCL_ROOT/include/nccl.h" + exit 1 +fi +NCCL_MAJOR=$(awk '/#define NCCL_MAJOR/{print $3}' "$NCCL_ROOT/include/nccl.h" | head -1) +NCCL_MINOR=$(awk '/#define NCCL_MINOR/{print $3}' "$NCCL_ROOT/include/nccl.h" | head -1) +echo "NCCL_MAJOR=$NCCL_MAJOR NCCL_MINOR=$NCCL_MINOR" +if [ -z "$NCCL_MAJOR" ] || [ -z "$NCCL_MINOR" ] || \ + [ "$NCCL_MAJOR" -lt 2 ] || { [ "$NCCL_MAJOR" -eq 2 ] && [ "$NCCL_MINOR" -lt 28 ]; }; then + echo "ERROR: NCCL >= 2.28 is required" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci_install_trtllm.sh` around lines 198 - 199, The script currently only prints NCCL_MAJOR/NCCL_MINOR from "$NCCL_ROOT/include/nccl.h" but should fail-fast if the version is less than 2.28; update the block that runs grep so it reads the macros (or returns an error if nccl.h is missing), extract numeric NCCL_MAJOR and NCCL_MINOR values (e.g., via sed/awk or shell parameter parsing), then perform a numeric check: if NCCL_MAJOR > 2 OR (NCCL_MAJOR == 2 AND NCCL_MINOR >= 28) continue, otherwise print a clear error to stderr and exit 1; reference the existing variables/paths NCCL_ROOT and "$NCCL_ROOT/include/nccl.h" and the grep line so you replace the diagnostic-only code with the extract/compare/exit logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci_install_trtllm.sh`:
- Around line 315-320: The sed replacement loop over req_file (for req_file in
requirements.txt requirements-dev.txt) always prints "Patched NCCL
constraint..." even if no substitution occurred; update the loop to first test
each file for the old pattern (e.g., grep -q
'nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7' "$req_file") and only run sed and echo
when the pattern is found, and otherwise fail the script (exit non-zero) or
print a clear warning so CI fails if the expected patch did not apply; ensure
you reference the same exact pattern when checking and when replacing so the
check and sed operate on the exact token used in the current diff.
---
Duplicate comments:
In `@scripts/ci_install_trtllm.sh`:
- Around line 163-170: The script currently only clones TensorRT-LLM if
$TRTLLM_DIR is missing, allowing stale checkout reuse; change the logic around
the clone/checkout step (the block using TRTLLM_DIR plus the subsequent git lfs
and pull commands) so that after ensuring the repo exists you explicitly fetch
and check out a deterministic ref (use an env var like TRTLLM_REF or a pinned
SHA), e.g., run git fetch --all and then git checkout <TRTLLM_REF_or_SHA>
followed by git reset --hard <TRTLLM_REF_or_SHA> (or git reset --hard
origin/<ref>) before running git lfs install/pull to guarantee each CI run uses
the intended commit.
- Around line 198-199: The script currently only prints NCCL_MAJOR/NCCL_MINOR
from "$NCCL_ROOT/include/nccl.h" but should fail-fast if the version is less
than 2.28; update the block that runs grep so it reads the macros (or returns an
error if nccl.h is missing), extract numeric NCCL_MAJOR and NCCL_MINOR values
(e.g., via sed/awk or shell parameter parsing), then perform a numeric check: if
NCCL_MAJOR > 2 OR (NCCL_MAJOR == 2 AND NCCL_MINOR >= 28) continue, otherwise
print a clear error to stderr and exit 1; reference the existing variables/paths
NCCL_ROOT and "$NCCL_ROOT/include/nccl.h" and the grep line so you replace the
diagnostic-only code with the extract/compare/exit logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe9b8728-ba08-4938-8a69-e099782d1532
📒 Files selected for processing (1)
scripts/ci_install_trtllm.sh
| for req_file in requirements.txt requirements-dev.txt; do | ||
| if [ -f "$req_file" ]; then | ||
| sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file" | ||
| echo "Patched NCCL constraint in $req_file" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Fail when NCCL constraint patch does not apply.
This sed replacement is exact-string based and echo "Patched..." runs even when no replacement happened. If upstream formatting/version text changes, downgrade risk returns silently.
Suggested fix
for req_file in requirements.txt requirements-dev.txt; do
if [ -f "$req_file" ]; then
- sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file"
- echo "Patched NCCL constraint in $req_file"
+ before_checksum=$(sha256sum "$req_file" | awk '{print $1}')
+ sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file"
+ after_checksum=$(sha256sum "$req_file" | awk '{print $1}')
+ if [ "$before_checksum" = "$after_checksum" ]; then
+ echo "ERROR: NCCL constraint patch did not apply in $req_file"
+ exit 1
+ fi
+ echo "Patched NCCL constraint in $req_file"
fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for req_file in requirements.txt requirements-dev.txt; do | |
| if [ -f "$req_file" ]; then | |
| sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file" | |
| echo "Patched NCCL constraint in $req_file" | |
| fi | |
| done | |
| for req_file in requirements.txt requirements-dev.txt; do | |
| if [ -f "$req_file" ]; then | |
| before_checksum=$(sha256sum "$req_file" | awk '{print $1}') | |
| sed -i 's/nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7/nvidia-nccl-cu13>=2.28.0/' "$req_file" | |
| after_checksum=$(sha256sum "$req_file" | awk '{print $1}') | |
| if [ "$before_checksum" = "$after_checksum" ]; then | |
| echo "ERROR: NCCL constraint patch did not apply in $req_file" | |
| exit 1 | |
| fi | |
| echo "Patched NCCL constraint in $req_file" | |
| fi | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_trtllm.sh` around lines 315 - 320, The sed replacement
loop over req_file (for req_file in requirements.txt requirements-dev.txt)
always prints "Patched NCCL constraint..." even if no substitution occurred;
update the loop to first test each file for the old pattern (e.g., grep -q
'nvidia-nccl-cu13<=2\.28\.9,>=2\.27\.7' "$req_file") and only run sed and echo
when the pattern is found, and otherwise fail the script (exit non-zero) or
print a clear warning so CI fails if the expected patch did not apply; ensure
you reference the same exact pattern when checking and when replacing so the
check and sed operate on the exact token used in the current diff.
torch==2.9.1+cu130 has an exact pin on nvidia-nccl-cu13==2.27.7. build_wheel.py runs pip install internally which resolves torch's dep and downgrades NCCL from 2.29 to 2.27, overwriting the headers in-place. Fix: after installing NCCL 2.28+, copy include/ and lib/ to /tmp/nccl-stable/ and point NCCL_ROOT there. This directory is not managed by pip, so subsequent pip installs cannot overwrite it. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
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 `@scripts/ci_install_trtllm.sh`:
- Around line 191-199: The script assumes the NCCL pip package exists at
SITE_PACKAGES/nvidia/nccl (NCCL_PIP_ROOT) and blindly runs cp -a which fails
with confusing errors if missing; add an explicit guard that checks the
existence of NCCL_PIP_ROOT (and at least its expected include/ and lib/ subdirs)
before attempting to copy, and if absent print a clear error and exit non‑zero;
update the block that defines SITE_PACKAGES, NCCL_PIP_ROOT and NCCL_ROOT to test
-d "$NCCL_PIP_ROOT" (and/or -d "$NCCL_PIP_ROOT/include" && -d
"$NCCL_PIP_ROOT/lib") and call echo >&2 with a descriptive message and exit 1
instead of proceeding to the cp commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91f859b5-3c6b-47be-8578-5de404d428e4
📒 Files selected for processing (1)
scripts/ci_install_trtllm.sh
| SITE_PACKAGES=$(python3 -c "import site; print(site.getsitepackages()[0])") | ||
| NCCL_PIP_ROOT="$SITE_PACKAGES/nvidia/nccl" | ||
|
|
||
| # Copy to a stable location that pip won't touch | ||
| NCCL_ROOT="/tmp/nccl-stable" | ||
| rm -rf "$NCCL_ROOT" | ||
| mkdir -p "$NCCL_ROOT/include" "$NCCL_ROOT/lib" | ||
| cp -a "$NCCL_PIP_ROOT/include/"* "$NCCL_ROOT/include/" | ||
| cp -a "$NCCL_PIP_ROOT/lib/"* "$NCCL_ROOT/lib/" |
There was a problem hiding this comment.
Validate NCCL pip package directory exists before copying.
If the pip package structure changes or installation fails silently, cp -a will fail with a confusing error. Add a guard to fail fast with a clear message.
Suggested fix
SITE_PACKAGES=$(python3 -c "import site; print(site.getsitepackages()[0])")
NCCL_PIP_ROOT="$SITE_PACKAGES/nvidia/nccl"
+if [ ! -d "$NCCL_PIP_ROOT/include" ] || [ ! -d "$NCCL_PIP_ROOT/lib" ]; then
+ echo "ERROR: Expected NCCL directories not found at $NCCL_PIP_ROOT"
+ echo " nvidia-nccl-cu13 may have failed to install or changed structure"
+ exit 1
+fi
+
# Copy to a stable location that pip won't touch
NCCL_ROOT="/tmp/nccl-stable"
rm -rf "$NCCL_ROOT"
mkdir -p "$NCCL_ROOT/include" "$NCCL_ROOT/lib"
cp -a "$NCCL_PIP_ROOT/include/"* "$NCCL_ROOT/include/"
cp -a "$NCCL_PIP_ROOT/lib/"* "$NCCL_ROOT/lib/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_trtllm.sh` around lines 191 - 199, The script assumes the
NCCL pip package exists at SITE_PACKAGES/nvidia/nccl (NCCL_PIP_ROOT) and blindly
runs cp -a which fails with confusing errors if missing; add an explicit guard
that checks the existence of NCCL_PIP_ROOT (and at least its expected include/
and lib/ subdirs) before attempting to copy, and if absent print a clear error
and exit non‑zero; update the block that defines SITE_PACKAGES, NCCL_PIP_ROOT
and NCCL_ROOT to test -d "$NCCL_PIP_ROOT" (and/or -d "$NCCL_PIP_ROOT/include" &&
-d "$NCCL_PIP_ROOT/lib") and call echo >&2 with a descriptive message and exit 1
instead of proceeding to the cp commands.
…ngrade (smg-project#779) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
…ngrade (smg-project#779) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
requirements-dev.txtRoot Cause
PR #777 bumped NCCL to 2.28+ but the install happened before
pip install -r requirements-dev.txt. TRT-LLM's requirements pinnvidia-nccl-cu13<=2.28.9,>=2.27.7, which downgraded NCCL. CMake then found the downgraded 2.27.7 header, and the#if NCCL_VERSION_CODE >= NCCL_VERSION(2, 28, 0)gate hidNCCLWindowAllocator.What changed
requirements-dev.txt. Uses--force-reinstallto override whatever version TRT-LLM pulled in. Added NCCL version diagnostics (NCCL_MAJOR/NCCL_MINORfrom header) to catch version mismatches in the future.Test plan
e2e-1gpu-chat (trtllm)job passesNCCL_MAJOR 2andNCCL_MINOR >= 28Summary by CodeRabbit