[None][feat] Add TRTLLM_PRECOMPILED_FROM_MAIN env var for installing from post-merge builds#12039
Open
tomeras91 wants to merge 6 commits intoNVIDIA:mainfrom
Open
[None][feat] Add TRTLLM_PRECOMPILED_FROM_MAIN env var for installing from post-merge builds#12039tomeras91 wants to merge 6 commits intoNVIDIA:mainfrom
tomeras91 wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
…ng commit on main Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
…ove unused workspace param Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughThis change introduces a new precompiled build workflow using Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Setup as setup.py
participant GitHub as GitHub API
participant Artifactory
participant System
User->>Setup: Run setup with TRTLLM_PRECOMPILED_FROM_MAIN=1
Setup->>Setup: Check environment variables
alt from_main_value is "1"
Setup->>Setup: Run git merge-base to find common ancestor
Setup->>GitHub: Query merge-base commit SHA
else from_main_value is commit SHA
Setup->>Setup: Use provided commit SHA directly
end
Setup->>Artifactory: Query GenPostMergeBuilds folders
Artifactory-->>Setup: Return list of builds
Setup->>Artifactory: Check build_info.txt for matching commit
Artifactory-->>Setup: Confirm build match
Setup->>Setup: Construct tarball URL with architecture info
Setup->>Artifactory: Download precompiled tarball
Artifactory-->>Setup: Return tarball
Setup->>System: Extract and use precompiled binaries
System-->>User: Build complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 4
🧹 Nitpick comments (1)
scripts/package_trt_llm.py (1)
11-11: Prefer importing the helper module, not the constant.This keeps the dependency namespaced and avoids another direct-symbol import in module scope.
♻️ Minimal refactor
-from get_wheel_from_package import NVIDIA_ARTIFACTORY_URL +import get_wheel_from_package as wheel_pkg @@ - f'{NVIDIA_ARTIFACTORY_URL}/sw-tensorrt-generic/llm-artifacts/LLM/main/L0_PostMerge/1379/' + f'{wheel_pkg.NVIDIA_ARTIFACTORY_URL}/sw-tensorrt-generic/llm-artifacts/LLM/main/L0_PostMerge/1379/'As per coding guidelines, "When importing in Python, always maintain the namespace. Import the module, not individual classes or functions."
Also applies to: 177-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/package_trt_llm.py` at line 11, The file currently imports the constant directly (from get_wheel_from_package import NVIDIA_ARTIFACTORY_URL); change this to import the module (import get_wheel_from_package) and update all usages to reference get_wheel_from_package.NVIDIA_ARTIFACTORY_URL (also apply the same replacement where the direct constant import appears again around the other occurrence), keeping the symbol namespaced to the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/source/installation/build-from-source-linux.md`:
- Around line 264-279: State in the "Using the Latest Post-Merge Build" section
that when both environment variables are set, TRTLLM_PRECOMPILED_LOCATION takes
precedence over TRTLLM_PRECOMPILED_FROM_MAIN (this behavior comes from setup.py
which checks TRTLLM_PRECOMPILED_LOCATION first), and add a short note advising
users to unset TRTLLM_PRECOMPILED_LOCATION if they intend to use
TRTLLM_PRECOMPILED_FROM_MAIN (or show a quick example: unset
TRTLLM_PRECOMPILED_LOCATION && TRTLLM_PRECOMPILED_FROM_MAIN=... pip install -e
.).
In `@setup.py`:
- Around line 460-463: precompiled_from_main currently treats an empty
TRTLLM_PRECOMPILED_FROM_MAIN="" as enabled while later logic treats it as
falsey; normalize and reject empty values early by converting
precompiled_from_main = os.getenv("TRTLLM_PRECOMPILED_FROM_MAIN") into None when
it's an empty or all-whitespace string (e.g., set to None if not value.strip()),
then compute use_precompiled using the normalized precompiled,
precompiled_location, and precompiled_from_main variables; additionally, where
download_precompiled(tempdir, None) would be called (the code path using
download_precompiled), add an explicit check and raise a clear setup/config
error if TRTLLM_PRECOMPILED_FROM_MAIN was provided but empty instead of
attempting a download.
- Around line 22-23: The code masks real fetch/parse errors by only catching
URLError in storage_url and using bare excepts in the build_info.txt loop;
change these to catch specific exceptions (e.g., urllib.error.HTTPError,
urllib.error.URLError and json.JSONDecodeError) and only treat a
404/missing-metadata response as a skip; for all other HTTP errors or JSON parse
failures re-raise or abort immediately. Update imports to include HTTPError and
json.JSONDecodeError, replace bare except: blocks inside storage_url and the
build_info.txt processing loop with except HTTPError as e: check for 404 (handle
skip) else raise, except URLError as e: raise, and except json.JSONDecodeError
as e: raise so only the true "missing metadata" case is swallowed.
- Around line 285-305: The current loop picks the first descending build whose
commit starts with target_commit, which can silently pick the wrong build for
short SHA prefixes; instead iterate over the candidate window
(build_numbers[:MAX_BUILDS_TO_CHECK]) and collect all build_nums whose
build_commit matches the target_commit prefix, then require that the set of
matching build_commits is exactly one unique commit and exactly one matching
build_num; if zero or multiple distinct commits match, raise/exit with a clear
error; otherwise set matched_build to that single matching build_num. Update
logic around matched_build, build_commit, build_numbers and MAX_BUILDS_TO_CHECK
to perform the full-scan-and-validate before selecting a build.
---
Nitpick comments:
In `@scripts/package_trt_llm.py`:
- Line 11: The file currently imports the constant directly (from
get_wheel_from_package import NVIDIA_ARTIFACTORY_URL); change this to import the
module (import get_wheel_from_package) and update all usages to reference
get_wheel_from_package.NVIDIA_ARTIFACTORY_URL (also apply the same replacement
where the direct constant import appears again around the other occurrence),
keeping the symbol namespaced to the module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 40db3dc6-f868-4ce6-a81b-65443c881312
📒 Files selected for processing (4)
docs/source/installation/build-from-source-linux.mdscripts/get_wheel_from_package.pyscripts/package_trt_llm.pysetup.py
- Document TRTLLM_PRECOMPILED_LOCATION precedence in docs - Narrow exception handling: only skip build_info.txt on 404, fail on other errors - Catch json.JSONDecodeError on Artifactory storage listing - Reject empty TRTLLM_PRECOMPILED_FROM_MAIN early with clear error Signed-off-by: Tomer Asida <57313761+tomeras91@users.noreply.github.com>
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #38287 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #38287 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #38312 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #38312 [ run ] completed with state |
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
2 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 by CodeRabbit
New Features
TRTLLM_PRECOMPILED_FROM_MAINenvironment variableDocumentation
Chores
Description
Adds a new
TRTLLM_PRECOMPILED_FROM_MAINenv var that allows developers to install TensorRT-LLM using precompiled C++/CUDA binaries from the latest compatibleGenPostMergeBuildsCI build on internal Artifactory (added in #11895), similar to vLLM'sVLLM_USE_PRECOMPILEDmechanism. This enables fast editable installs (pip install -e.) without compiling from source, using binaries that match the developer's current branch state via git merge-base commit correlation. Requires NVIDIA network/VPN access.This enhances the current
TRTLLM_USE_PRECOMPILEDbehavior, that only searches for RC wheels.Summary
TRTLLM_PRECOMPILED_FROM_MAINenv var to install precompiled wheels from theGenPostMergeBuildspipeline on internal Artifactory, using vLLM-style git merge-base commit correlation=1(auto-detect via merge-base) or=<commit SHA>(short or full) to pin to a specific main commitTRTLLM_USE_PRECOMPILED=1NVIDIA_ARTIFACTORY_URLconstant intoscripts/get_wheel_from_package.pyto avoid duplicating the URL across scriptsUsage
Test plan
All checks worked as expected in a local setup.
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.