Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Build and test path updates jenkins/Build.groovy, jenkins/L0_Test.groovy |
Build, test, and package-sanity paths remove alternative TensorRT setup and replacement logic. Helpers and callers drop cpver; dependency cleanup removes TensorRT runtime files unconditionally. |
Job selection conditions jenkins/L0_MergeRequest.groovy, jenkins/L0_Test.groovy, jenkins/scripts/cbts/README.md |
Job selection and test-stage conditions no longer use alternativeTRT. The CBTS fallback rule now applies to PostMerge jobs without that condition. |
Downstream job API and architecture wiring jenkins/L0_MergeRequest.groovy |
Launch helpers drop the platform argument. Architecture build, test, dry-run, BOLT profile-generation, and Docker-image calls use the revised signatures. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Other
Suggested reviewers: juney-nvidia
Merge Risk: 
2ebda
-
No actionable merge-blocking risk is established. The rebased revision has not yet been validated in Jenkins.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: removing legacy alternativeTRT CI paths. It also follows the required [None][chore] format. Description check The description explains the problem and solution, documents validation and test limitations, and includes the required checklist sections. It is sufficiently complete for this maintenance change. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
|
/bot run --disable-fail-fast |
|
PR_Github #79186 [ run ] triggered by Bot. Commit: |
|
PR_Github #79186 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #79283 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
I grepped every caller of the changed signatures (buildWheelInContainer, launchJob, launchInfraDryRunTestJob, and the runLLMTestlist* / executeLLMTestOnSlurm chain). All of them now pass arguments in the new positional order, and cpver is still in runLLMBuild and the kitmaker dry-run helper, where it still selects the wheel ABI. Two out-of-repo follow-ups:
- Jenkins jobs that still set
alternativeTRT. If any TRT dependency-testing job (or a saved parameterized build) still injectsalternativeTRT, that run will now be treated as an ordinary pre-merge job: CBTS/changed-file filtering, no forced multi-GPU or post-merge stages, andgetMergeRequestChangedFileListcalling the MR API with no MR behind it. Please confirm those jobs are retired, or remove the parameter from their job configs, so they don't start failing for unclear reasons. trtllm_utils.replaceWithAlternativeTRTin the shared library has no callers after this PR. A follow-up to delete it there would finish the cleanup.
The description also says Jenkins pipelines weren't run locally. Since the PR has the ci: full pre-merge approved label, a full /bot run (including the SBSA and Slurm/multi-node stages that use the reordered runLLMTestlistOnSlurm arguments) before merge would confirm the positional changes at runtime.
|
PR_Github #79283 [ run ] completed with state
|
80f8ea5 to
639aabe
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #81838 [ run ] triggered by Bot. Commit: |
|
PR_Github #81838 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #81877 [ run ] triggered by Bot. Commit: |
|
PR_Github #81877 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
639aabe to
2ebda8e
Compare
Description
Problem
Jenkins still carries
alternativeTRTandalternativeTrtSBSApaths after removal of the TensorRT backend. These paths replace TensorRT packages, rewrite requirements, and override normal test selection.Solution
Remove the legacy overrides from the build, test, and downstream job-launch scripts, along with their CBTS documentation. Remove unused platform and Python ABI arguments and update their callers. Preserve the existing default PR, PostMerge, and nightly-release conditions.
Test Coverage
Validation
skipInstallWheel; the 12 existing infrastructure dry-run pipeline tests passed.git diff --checkpassed; a repository-wide search found no remainingalternativeTRT,alternativeTrtSBSA, orreplaceWithAlternativeTRTreferences.80f8ea5and639aabefailed; the current head has not yet been validated by Jenkins CI.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)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.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.Dev Engineer Review
alternativeTRTandalternativeTrtSBSAhandling from Jenkins build, test, and downstream job-launch paths.cpverand platform parameters, with caller updates.PR_Githubjobs succeeded. Their associatedL0_MergeRequest_PRpipelines failed. The supplied CI comments do not identify the failing tests.QA Engineer Review
No test changes.
Per-File QA Perspective
jenkins/Build.groovy: Verify wheel builds no longer replace TensorRT packages or edit requirements. Check callers against the revisedbuildWheelInContainersignature.jenkins/L0_MergeRequest.groovy: Verify downstream jobs receive the expected parameters after removal of platform and alternative-TRT arguments. Check PR, PostMerge, nightly, BOLT, Docker-image, and container-scan paths.jenkins/L0_Test.groovy: Verify test, Slurm, platform, package-sanity, and post-merge paths after removal ofcpverand alternative-TRT logic. Check retry and reporting behavior.jenkins/scripts/cbts/README.md: Verify CBTS documentation describes PostMerge fallback without analternativeTRTcondition.