Skip to content

[None][test] Fix nvshmem path in verl_config.yml for DLFW upgrade - #14563

Open
Superjomn wants to merge 51 commits into
NVIDIA:mainfrom
Superjomn:emma/upgrade_dlfw_2603
Open

[None][test] Fix nvshmem path in verl_config.yml for DLFW upgrade#14563
Superjomn wants to merge 51 commits into
NVIDIA:mainfrom
Superjomn:emma/upgrade_dlfw_2603

Conversation

@Superjomn

@Superjomn Superjomn commented May 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Changed NVSHMEM_DIR from /usr/local/lib/python3.12/dist-packages/nvidia/nvshmem to /usr/local/cuda/targets/x86_64-linux — the system CUDA nvshmem installation that libtorch_nvshmem.so is built against
  • Updated the nvshmem symlink step to use the same CUDA path, with an idempotency guard ([ -e libnvshmem_host.so ] ||) to avoid errors if the symlink already exists

Root Cause

After the DLFW upgrade, the nvshmem pip package is no longer installed at the system Python dist-packages path. The old NVSHMEM_DIR caused LD_LIBRARY_PATH to prepend a mismatched libnvshmem_host.so.3 (missing the nvshmem_selected_device_transport@@NVSHMEM symbol) ahead of the correct system library, crashing torch import with:

ImportError: libtorch_nvshmem.so: undefined symbol: nvshmem_selected_device_transport, version NVSHMEM

Test plan

  • test_async_generate passes locally on the upgraded DLFW container (verified 3 runs, ~95s each)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added librosa and msgpack as new dependencies.
  • Bug Fixes

    • Fixed CUDA API initialization compatibility issues.
    • Improved build compatibility for Clang and NVSHMEM compilation.
  • Chores

    • Updated CUDA from 13.1.1 to 13.2.1.
    • Updated PyTorch from 2.10.0 to 2.11.0.
    • Updated TensorRT to version 10.16.1.
    • Updated Docker base images to version 26.04.

Review Change Stack

Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
… 2.11.0a0

Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
EmmaQiaoCh and others added 16 commits May 8, 2026 12:34
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
…ompiler error

Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: EmmaQiaoCh <qqiao@nvidia.com>
Signed-off-by: Emma Qiao <qqiao@nvidia.com>
After the DLFW upgrade, the nvshmem pip package is no longer installed
at the system Python dist-packages path. Point NVSHMEM_DIR to the
system CUDA nvshmem installation instead, which has the correct symbol
versions that libtorch_nvshmem.so expects. Also update the symlink
step to the correct path with an idempotency guard.

The old NVSHMEM_DIR caused libtorch_nvshmem.so to load a mismatched
libnvshmem_host.so missing the `nvshmem_selected_device_transport`
symbol, crashing torch import in verl tests.

Signed-off-by: Chunwei Yi <chunweiy@nvidia.com>
Signed-off-by: Superjomn <328693+Superjomn@users.noreply.github.com>
@Superjomn
Superjomn requested a review from a team May 26, 2026 08:58
@Superjomn
Superjomn requested review from a team as code owners May 26, 2026 08:58
@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR updates TensorRT-LLM to use CUDA 13.2.1, PyTorch 2.11.0, and TensorRT 10.16.1, with corresponding Docker, build system, and CI infrastructure changes. It includes C++ struct initializer fixes for CUDA API compatibility, CMake toolchain adjustments for Clang/GCC compilation, a switch to PyTorch's compile_fx_inner backend, and enhancements to the Docker image naming script and test waivers.

Changes

Version upgrades and build infrastructure

Layer / File(s) Summary
Dependency version pins and requirements
README.md, requirements.txt, constraints.txt, docs/source/legacy/reference/support-matrix.md
README badges, requirements.txt, and constraints.txt pin CUDA 13.2.1, PyTorch 2.11.0, TensorRT 10.16.1; new dependencies librosa and msgpack are added; support-matrix docs updated to reference 26.04 container and 10.16 TensorRT release notes.
Docker base images and installation scripts
docker/Dockerfile.multi, docker/Makefile, docker/common/install_*.sh
Base image tags advance from 26.02-py3 to 26.04-py3; CUDA base tags update to 13.2.1-devel variants; install scripts pin CUDA 13.2.1, PyTorch 2.11.0, and TensorRT 10.16.1; Dockerfile constraints installation simplified to direct pip install.

CUDA virtual memory API initialization fixes

Layer / File(s) Summary
CUDA virtual memory struct initializers
cpp/include/tensorrt_llm/runtime/virtualMemory.h, cpp/tensorrt_llm/runtime/virtualMemory.cpp, cpp/tests/unit_tests/common/cudaDriverWrapperTest.cpp, cpp/tests/unit_tests/runtime/virtualMemoryTest.cpp
CUmemLocation and CUmemAccessDesc struct initializers are updated to use brace-nested device field syntax ({device}) in virtual memory allocation, manager setup, and test construction to align with CUDA API expectations.

CMake build compatibility for Clang/GCC CUDA compilation

Layer / File(s) Summary
CMake CUDA compiler flag compatibility
cpp/tensorrt_llm/deep_ep/CMakeLists.txt, cpp/tensorrt_llm/flash_mla/CMakeLists.txt
CMake scripts strip -Xcompiler=-fclang-abi-compat flags from CMAKE_CUDA_FLAGS to prevent Clang-specific flags from reaching g++ during CUDA host compilation; deep_ep adds NVSHMEM vendored header include path with BEFORE priority.

PyTorch compilation backend migration

Layer / File(s) Summary
Torch FX Inductor compilation backend
tensorrt_llm/_torch/compilation/backend.py, tensorrt_llm/_torch/compilation/piecewise_optimizer.py
Imports and function calls switch from compile_fx to compile_fx_inner for GraphModule and PiecewiseRunner Inductor compilation paths.

CI, testing, and infrastructure updates

Layer / File(s) Summary
Docker image naming script enhancement
scripts/rename_docker_images.py
Script adds support for GitHub (commit SHA) and GitLab (branch name) image tagging modes via mutually exclusive --commit and --src-branch CLI arguments; new get_current_commit() helper retrieves current git HEAD short SHA.
Jenkins pipeline and image tag configuration
jenkins/Build.groovy, jenkins/L0_Test.groovy, jenkins/current_image_tags.properties
Build.groovy Triton tag updates (r26.02→r26.04); L0_Test.groovy updates DLFW image tag, SLURM MPI mode (pmi2→pmix), Triton server version, PY312 image selection, and bare-metal CUDA/PyTorch CUDA wheel versions; current_image_tags.properties updates all five image tag variants with new framework/CUDA/TRT versions.
Integration test infrastructure and waivers
tests/integration/defs/verl/verl_config.yml, tests/integration/test_lists/waives.txt
Verl CI config redirects NVSHMEM installation to CUDA targets directory; waives.txt adds SKIP entries for DeepSeek V3 Lite multi-GPU, Llama3.1 FLASHINFER+torch-compile, Nemotron-H CUDA graph overlap, and visual-gen/MoE auto-deploy model tests with associated nvbug references.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • NVIDIA/TensorRT-LLM#9960: Updates constraints installation workflow in docker/Dockerfile.multi similar to this PR's simplification of pip install logic.
  • NVIDIA/TensorRT-LLM#14523: Directly overlaps on DeepSeek V3 Lite waiver entries in tests/integration/test_lists/waives.txt with similar test skip definitions.
  • NVIDIA/TensorRT-LLM#14516: Modifies the same tests/integration/test_lists/waives.txt file by adding new SKIP waiver entries for integration test cases.

Suggested reviewers

  • jieli-matrix
  • mzweilz
  • chzblych
  • StanleySun639
  • nv-guomingz
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the specific fix: addressing nvshmem path configuration in verl_config.yml as part of a DLFW upgrade.
Description check ✅ Passed The PR description includes clear sections covering the summary of changes, root cause analysis, and test coverage validation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch emma/upgrade_dlfw_2603

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
tests/integration/defs/verl/verl_config.yml (1)

8-43: QA list update looks unnecessary for this change scope.

This is an infra/path correction in existing verl setup, not a new/expanded integration test definition, so no tests/integration/test_lists/qa/* addition is needed in this PR.

As per coding guidelines, "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."

🤖 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 8 - 43, The change
only fixes infra/path settings in verl_config.yml (the install_commands and
env_vars blocks) and does not expand integration test scope, so explicitly mark
QA list updates as unnecessary: update the PR description and/or commit message
to state that no addition to tests/integration/test_lists/qa is required, and if
any QA list files were accidentally added, remove them; reference the config
file name "verl_config.yml" and the keys "install_commands" and "env_vars" when
making this note.
🤖 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 `@jenkins/current_image_tags.properties`:
- Line 20: LLM_SBSA_WHEEL_DOCKER_IMAGE is pinned to the older cuda-13.1.0 /
trt10.15.1.29 tag causing mixed-toolchain CI; update the value of
LLM_SBSA_WHEEL_DOCKER_IMAGE to the same newer CUDA/TRT tag used by adjacent
image entries (match their cuda and trt versions) so all SBSA wheel images use a
consistent toolchain, then run a quick CI smoke to verify no mismatched
dependencies.

In `@jenkins/L0_Test.groovy`:
- Line 1: The Jenkinsfile currently pins the shared library to a personal branch
reference `bloom-jenkins-shared-lib@emma/update_nsc_login_node`; change this to
a stable ref such as `bloom-jenkins-shared-lib@main` or an immutable release
tag/commit (or commit SHA) to avoid brittleness. Update the library declaration
line that contains `bloom-jenkins-shared-lib@emma/update_nsc_login_node` to use
`@main` or a release/tag/commit and ensure any consuming code still works with
that stable version.
- Around line 4383-4388: Update the misleading "CUDA 13.2" wording to match the
actual wheel channel (+cu130) being installed: change the echo string "######
Extra PyTorch CUDA 13.2 install Start ######" and the preceding comment that
mentions "Extra PyTorch CUDA 13.2 install" to reference CUDA 13.0 (or "CUDA 13.0
/ cu130") so it matches the pip install of torch==2.11.0+cu130; ensure the echo
and comment surrounding the trtllm_utils.llmExecStepWithRetry(...) call are
updated accordingly.

In `@scripts/rename_docker_images.py`:
- Around line 77-80: In get_current_commit(), avoid PATH hijacking by resolving
the git executable with shutil.which (e.g., git_path = shutil.which("git")) and
use that absolute path as the first element passed to _sp.run (replace ["git",
...] with [git_path, ...]); if shutil.which returns None, raise a clear error
(or fallback to "git" with a logged warning) so you don't inadvertently call an
unexpected binary. Ensure the unique symbols referenced are get_current_commit,
shutil.which, and the existing _sp.run invocation.

---

Nitpick comments:
In `@tests/integration/defs/verl/verl_config.yml`:
- Around line 8-43: The change only fixes infra/path settings in verl_config.yml
(the install_commands and env_vars blocks) and does not expand integration test
scope, so explicitly mark QA list updates as unnecessary: update the PR
description and/or commit message to state that no addition to
tests/integration/test_lists/qa is required, and if any QA list files were
accidentally added, remove them; reference the config file name
"verl_config.yml" and the keys "install_commands" and "env_vars" when making
this note.
🪄 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: f43e98bc-712b-4d6a-8b12-f8f6de7e8345

📥 Commits

Reviewing files that changed from the base of the PR and between 0c44d2d and ff9b14d.

📒 Files selected for processing (23)
  • README.md
  • constraints.txt
  • cpp/include/tensorrt_llm/runtime/virtualMemory.h
  • cpp/tensorrt_llm/deep_ep/CMakeLists.txt
  • cpp/tensorrt_llm/flash_mla/CMakeLists.txt
  • cpp/tensorrt_llm/runtime/virtualMemory.cpp
  • cpp/tests/unit_tests/common/cudaDriverWrapperTest.cpp
  • cpp/tests/unit_tests/runtime/virtualMemoryTest.cpp
  • docker/Dockerfile.multi
  • docker/Makefile
  • docker/common/install_cuda_toolkit.sh
  • docker/common/install_pytorch.sh
  • docker/common/install_tensorrt.sh
  • docs/source/legacy/reference/support-matrix.md
  • jenkins/Build.groovy
  • jenkins/L0_Test.groovy
  • jenkins/current_image_tags.properties
  • requirements.txt
  • scripts/rename_docker_images.py
  • tensorrt_llm/_torch/compilation/backend.py
  • tensorrt_llm/_torch/compilation/piecewise_optimizer.py
  • tests/integration/defs/verl/verl_config.yml
  • tests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
  • constraints.txt

LLM_SBSA_DOCKER_IMAGE=urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm:pytorch-26.04-py3-aarch64-ubuntu24.04-trt10.16.1.11-skip-tritondevel-202605140823-12643
LLM_ROCKYLINUX8_PY310_DOCKER_IMAGE=urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm:cuda-13.2.1-devel-rocky8-x86_64-rocky8-py310-trt10.16.1.11-skip-tritondevel-202605140823-12643
LLM_ROCKYLINUX8_PY312_DOCKER_IMAGE=urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm:cuda-13.2.1-devel-rocky8-x86_64-rocky8-py312-trt10.16.1.11-skip-tritondevel-202605140823-12643
LLM_SBSA_WHEEL_DOCKER_IMAGE=urm.nvidia.com/sw-tensorrt-docker/tensorrt-llm:cuda-13.1.0-devel-ubuntu24.04-sbsa-ubuntu24.04-py312-trt10.15.1.29-skip-tritondevel-202605140823-12643

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

LLM_SBSA_WHEEL_DOCKER_IMAGE appears left on the old stack.

Line 20 still points to cuda-13.1.0 / trt10.15.1.29 while adjacent images were moved to the newer versions. If unintentional, this can cause mixed-toolchain CI behavior.

🤖 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 `@jenkins/current_image_tags.properties` at line 20,
LLM_SBSA_WHEEL_DOCKER_IMAGE is pinned to the older cuda-13.1.0 / trt10.15.1.29
tag causing mixed-toolchain CI; update the value of LLM_SBSA_WHEEL_DOCKER_IMAGE
to the same newer CUDA/TRT tag used by adjacent image entries (match their cuda
and trt versions) so all SBSA wheel images use a consistent toolchain, then run
a quick CI smoke to verify no mismatched dependencies.

Comment thread jenkins/L0_Test.groovy
@@ -1,4 +1,4 @@
@Library(['bloom-jenkins-shared-lib@main', 'trtllm-jenkins-shared-lib@main']) _
@Library(['bloom-jenkins-shared-lib@emma/update_nsc_login_node', 'trtllm-jenkins-shared-lib@main']) _

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid pinning Jenkins shared libs to a personal feature branch.

Line 1 references bloom-jenkins-shared-lib@emma/update_nsc_login_node. This creates a brittle CI dependency (branch force-push/delete risk). Pin to @main or an immutable release tag/commit.

🤖 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 `@jenkins/L0_Test.groovy` at line 1, The Jenkinsfile currently pins the shared
library to a personal branch reference
`bloom-jenkins-shared-lib@emma/update_nsc_login_node`; change this to a stable
ref such as `bloom-jenkins-shared-lib@main` or an immutable release tag/commit
(or commit SHA) to avoid brittleness. Update the library declaration line that
contains `bloom-jenkins-shared-lib@emma/update_nsc_login_node` to use `@main` or
a release/tag/commit and ensure any consuming code still works with that stable
version.

Comment thread jenkins/L0_Test.groovy
Comment on lines +4383 to +4388
// Extra PyTorch CUDA 13.2 install for all bare-metal environments (Default PyTorch is for CUDA 12.8)
if (values[6]) {
echo "###### Extra PyTorch CUDA 13.0 install Start ######"
echo "###### Extra PyTorch CUDA 13.2 install Start ######"
// Use internal mirror instead of https://download.pytorch.org/whl/cu130 for better network stability.
// PyTorch CUDA 13.0 package and torchvision package can be installed as expected.
trtllm_utils.llmExecStepWithRetry(pipeline, script: "pip3 install torch==2.10.0+cu130 torchvision==0.25.0+cu130 --extra-index-url https://urm.nvidia.com/artifactory/api/pypi/pytorch-cu128-remote/simple --extra-index-url https://download.pytorch.org/whl/cu130")
trtllm_utils.llmExecStepWithRetry(pipeline, script: "pip3 install torch==2.11.0+cu130 torchvision==0.26.0+cu130 --extra-index-url https://urm.nvidia.com/artifactory/api/pypi/pytorch-cu128-remote/simple --extra-index-url https://download.pytorch.org/whl/cu130")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix CUDA wording to match the actual wheel channel/version.

The comment says “CUDA 13.2 install”, but Line 4388 installs +cu130 wheels. Please align the wording to avoid operator confusion during incident/debug workflows.

🤖 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 `@jenkins/L0_Test.groovy` around lines 4383 - 4388, Update the misleading "CUDA
13.2" wording to match the actual wheel channel (+cu130) being installed: change
the echo string "###### Extra PyTorch CUDA 13.2 install Start ######" and the
preceding comment that mentions "Extra PyTorch CUDA 13.2 install" to reference
CUDA 13.0 (or "CUDA 13.0 / cu130") so it matches the pip install of
torch==2.11.0+cu130; ensure the echo and comment surrounding the
trtllm_utils.llmExecStepWithRetry(...) call are updated accordingly.

Comment on lines +77 to +80
return _sp.run(["git", "rev-parse", "--short=7", "HEAD"],
check=True,
text=True,
capture_output=True).stdout.strip()[:7]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
FILE="scripts/rename_docker_images.py"
if [ ! -f "$FILE" ]; then
  echo "Missing $FILE" >&2
  exit 1
fi
python3 - <<'PY'
import itertools
path="scripts/rename_docker_images.py"
with open(path,'r',encoding='utf-8') as f:
    lines=f.readlines()
for i in range(1, min(len(lines), 130)+1):
    if 60 <= i <= 100:
        print(f"{i:4d}: {lines[i-1].rstrip()}")
PY
echo "---- context around git invocation ----"
sed -n '60,95p' scripts/rename_docker_images.py
echo "---- imports at top ----"
sed -n '1,60p' scripts/rename_docker_images.py

Repository: NVIDIA/TensorRT-LLM

Length of output: 4986


Harden git invocation in get_current_commit() against PATH hijacking.

subprocess.run(["git", ...]) still relies on PATH resolution. Resolve git to an absolute path (e.g., via shutil.which) and pass that to _sp.run.

Suggested fix
+import shutil as _sh
+
 def get_current_commit() -> str:
     """Return the 7-char short SHA of the current git HEAD."""
-    return _sp.run(["git", "rev-parse", "--short=7", "HEAD"],
+    git_bin = _sh.which("git")
+    if git_bin is None:
+        raise FileNotFoundError("git executable not found in PATH")
+    return _sp.run([git_bin, "rev-parse", "--short=7", "HEAD"],
                    check=True,
                    text=True,
                    capture_output=True).stdout.strip()[:7]
🧰 Tools
🪛 Ruff (0.15.14)

[error] 77-77: Starting a process with a partial executable path

(S607)

🤖 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 `@scripts/rename_docker_images.py` around lines 77 - 80, In
get_current_commit(), avoid PATH hijacking by resolving the git executable with
shutil.which (e.g., git_path = shutil.which("git")) and use that absolute path
as the first element passed to _sp.run (replace ["git", ...] with [git_path,
...]); if shutil.which returns None, raise a clear error (or fallback to "git"
with a logged warning) so you don't inadvertently call an unexpected binary.
Ensure the unique symbols referenced are get_current_commit, shutil.which, and
the existing _sp.run invocation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants