[None][fix] Refactor UCX, NIXL, ETCD installations into install.sh #10531
Conversation
Signed-off-by: Shobhit Verma <[email protected]>
v-shobhit
changed the title
📝 WalkthroughWalkthroughThe changes introduce four modular feature flags (constraints, ucx, nixl, etcd) to the install script with corresponding command-line options, while refactoring the Dockerfile to use a consolidated installation approach via the --all flag and a CONSTRAINTS_FILE build argument. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
v-shobhit
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @docker/common/install.sh:
- Around line 157-160: The script uses the CONSTRAINTS_FILE variable in the
install block (the if [ $constraints -eq 1 ] branch) without validating it or
confirming the file exists; update that branch to first check that
CONSTRAINTS_FILE is set and non-empty and that the referenced file exists and is
readable before calling pip3 install --no-cache-dir -r $CONSTRAINTS_FILE, and if
the checks fail emit a clear error via echo and exit non-zero (preserve the
existing behavior when valid).
In @docker/Dockerfile.multi:
- Line 47: The Dockerfile.multi COPY instruction references a non-existent
constraints file (the COPY entry that includes the constraints file), which
breaks builds; fix it by removing that constraints entry from the COPY list in
Dockerfile.multi (or alternatively create the missing
docker/common/constraints.txt), but since there is already a root-level
constraints.txt copied later, prefer deleting the extra constraints entry from
the COPY command to avoid the missing-file build failure.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/Dockerfile.multidocker/common/install.sh
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T15:04:42.885Z
Learnt from: dbari
Repo: NVIDIA/TensorRT-LLM PR: 7095
File: docker/Dockerfile.multi:168-168
Timestamp: 2025-08-20T15:04:42.885Z
Learning: In docker/Dockerfile.multi, wildcard COPY for benchmarks (${CPP_BUILD_DIR}/benchmarks/*Benchmark) is intentionally used instead of directory copy because the benchmarks directory contains various other build artifacts during C++ builds, and only specific benchmark executables should be copied to the final image.
Applied to files:
docker/Dockerfile.multi
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Pre-commit Check
🔇 Additional comments (6)
docker/common/install.sh (4)
19-22: LGTM: Clean flag declarations.The new feature flags follow the established pattern and are initialized correctly.
62-77: LGTM: Consistent option parsing.The new command-line options are correctly implemented following the existing pattern.
88-91: LGTM: Proper --all flag extension.The --all option correctly includes the new installation flags.
172-175: No action needed.install_etcd.shdownloads from Google Cloud Storage (https://storage.googleapis.com/etcd), not GitHub, so it does not require theGITHUB_MIRRORenvironment variable. The inconsistency flagged is not applicable here.Likely an incorrect or invalid review comment.
docker/Dockerfile.multi (2)
56-56: LGTM: Clean consolidation with CONSTRAINTS_FILE support.The addition of
CONSTRAINTS_FILEARG with a sensible default and the switch to--allflag simplifies the installation process while maintaining flexibility through build arguments.Also applies to: 65-66
77-79: LGTM: Complete cleanup of installation artifacts.The cleanup properly removes all new installation scripts and the constraints file, maintaining a lean final image.
Signed-off-by: Shobhit Verma <[email protected]>
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #31009 [ run ] triggered by Bot. Commit: |
|
PR_Github #31009 [ run ] completed with state
|
Signed-off-by: Shobhit Verma <[email protected]>
v-shobhit
force-pushed
the
dev/shobhitv/enroot_build_fix
branch
from
c1ba59a to
c20cc56
Compare
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #31124 [ run ] triggered by Bot. Commit: |
|
| echo "Error: Constraints file not found: $CONSTRAINTS_FILE" >&2 | ||
| exit 1 | ||
| fi | ||
| pip3 install --no-cache-dir -r $CONSTRAINTS_FILE |
There was a problem hiding this comment.
A constraints.txt is not meant to be passed to the -r option with pip. It does not express what needs to be installed. The intention is to specify version constraints only if a certain package needs to be installed. It should be passed with the -c option to pip when the corresponding requirements.txt, which is passed to -r, is installed.
Do we even still need this constraints.txt file? Please check whether the vulnerability that it addresses has been resolved in the latest DLFW images. If it is still necessary, we should combine it with the pip install -r requirements.txt command instead of handling it separately.
There was a problem hiding this comment.
Thanks - it seems like we are currently installing this as per here - in that case, we should perhaps move urllib3>=2.6.0 from the constraints into the requirements file
Also CC: @ZhanruiSunCh since this was added as part of their commit here
There was a problem hiding this comment.
We will not install requirement.txt in the devel image of CI/NGC. However, constraints.txt is added to fix nspect's check of image, so it needs to be installed separately in image in addition to requirement.txt .
| ARG NCCL_VER | ||
| ARG CUBLAS_VER | ||
| ARG TORCH_INSTALL_TYPE="skip" | ||
| ARG CONSTRAINTS_FILE="constraints.txt" |
There was a problem hiding this comment.
Whey do we introduce this argument? we have hardcoded constraints.txt in the COPY and the rm commands below and above, respectively. So, what is the purpose of this?
| ./ | ||
|
|
||
| # Name install.sh conflicts with UCX installation | ||
| COPY docker/common/install.sh ./install_master.sh |
There was a problem hiding this comment.
Instead of renaming the script within the Dockerfile, why not just rename the source file in the repository to install_master.sh. This would simplify the Dockerfile.
There was a problem hiding this comment.
Whenever we change Dockerfile.multi, we should rebuild and update the Docker images for Jenkins in the same PR.
|
PR_Github #31124 [ run ] completed with state
|
6 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Description
Following up from !7383, urefactoring the dependancy installation into the single bash script so that the same may be used to build devel images in enroot sqsh files directly.
Test Coverage
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
/bot [-h] ['run', 'kill', 'skip', 'reuse-pipeline'] ...Provide a user friendly way for developers to interact with a Jenkins server.
Run
/bot [-h|--help]to print this help message.See details below for each supported subcommand.
Details
run [--reuse-test (optional)pipeline-id --disable-fail-fast --skip-test --stage-list "A10-PyTorch-1, xxx" --gpu-type "A30, H100_PCIe" --test-backend "pytorch, cpp" --add-multi-gpu-test --only-multi-gpu-test --disable-multi-gpu-test --post-merge --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx" --detailed-log --debug(experimental)]Launch build/test pipelines. All previously running jobs will be killed.
--reuse-test (optional)pipeline-id(OPTIONAL) : Allow the new pipeline to reuse build artifacts and skip successful test stages from a specified pipeline or the last pipeline if no pipeline-id is indicated. If the Git commit ID has changed, this option will be always ignored. The DEFAULT behavior of the bot is to reuse build artifacts and successful test results from the last pipeline.--disable-reuse-test(OPTIONAL) : Explicitly prevent the pipeline from reusing build artifacts and skipping successful test stages from a previous pipeline. Ensure that all builds and tests are run regardless of previous successes.--disable-fail-fast(OPTIONAL) : Disable fail fast on build/tests/infra failures.--skip-test(OPTIONAL) : Skip all test stages, but still run build stages, package stages and sanity check stages. Note: Does NOT update GitHub check status.--stage-list "A10-PyTorch-1, xxx"(OPTIONAL) : Only run the specified test stages. Examples: "A10-PyTorch-1, xxx". Note: Does NOT update GitHub check status.--gpu-type "A30, H100_PCIe"(OPTIONAL) : Only run the test stages on the specified GPU types. Examples: "A30, H100_PCIe". Note: Does NOT update GitHub check status.--test-backend "pytorch, cpp"(OPTIONAL) : Skip test stages which don't match the specified backends. Only support [pytorch, cpp, tensorrt, triton]. Examples: "pytorch, cpp" (does not run test stages with tensorrt or triton backend). Note: Does NOT update GitHub pipeline status.--only-multi-gpu-test(OPTIONAL) : Only run the multi-GPU tests. Note: Does NOT update GitHub check status.--disable-multi-gpu-test(OPTIONAL) : Disable the multi-GPU tests. Note: Does NOT update GitHub check status.--add-multi-gpu-test(OPTIONAL) : Force run the multi-GPU tests in addition to running L0 pre-merge pipeline.--post-merge(OPTIONAL) : Run the L0 post-merge pipeline instead of the ordinary L0 pre-merge pipeline.--extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx"(OPTIONAL) : Run the ordinary L0 pre-merge pipeline and specified test stages. Examples: --extra-stage "H100_PCIe-TensorRT-Post-Merge-1, xxx".--detailed-log(OPTIONAL) : Enable flushing out all logs to the Jenkins console. This will significantly increase the log volume and may slow down the job.--debug(OPTIONAL) : Experimental feature. Enable access to the CI container for debugging purpose. Note: Specify exactly one stage in thestage-listparameter to access the appropriate container environment. Note: Does NOT update GitHub check status.For guidance on mapping tests to stage names, see
docs/source/reference/ci-overview.mdand the
scripts/test_to_stage_mapping.pyhelper.kill
killKill all running builds associated with pull request.
skip
skip --comment COMMENTSkip testing for latest commit on pull request.
--comment "Reason for skipping build/test"is required. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.reuse-pipeline
reuse-pipelineReuse a previous pipeline to validate current commit. This action will also kill all currently running builds associated with the pull request. IMPORTANT NOTE: This is dangerous since lack of user care and validation can cause top of tree to break.