[None][fix] Fix Upload Build Info branch and run in post always#12025
Open
mzweilz wants to merge 1 commit intoNVIDIA:mainfrom
Open
[None][fix] Fix Upload Build Info branch and run in post always#12025mzweilz wants to merge 1 commit intoNVIDIA:mainfrom
mzweilz wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
mzweilz
force-pushed
the
user/abbyw/dev-abbyw-fix-post-merge-build-job-1
branch
from
9b34a0c to
f392692
Compare
Collaborator
Author
|
/bot run |
Contributor
📝 WalkthroughWalkthroughThe Upload Build Info stage is relocated from the main build stages to the post-build finalization block. The branch computation logic is updated to prioritize Changes
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)
Comment |
Collaborator
Author
|
@coderabbitai help |
Contributor
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
jenkins/L0_MergeRequest.groovy (1)
1368-1380: Good move to post-always, consider adding defensive error handling.Moving "Upload Build Info" to the
post { always { ... } }block ensures build info is captured even on pipeline failure—this aligns with the PR objective. The branch derivation logic is consistent with lines 1283-1286 (Build-Docker-Images).However, if
trtllm_utils.uploadArtifactsfails (e.g., network issues), the exception could alter the final build status or disrupt subsequent post actions. Consider wrapping in try-catch for resilience, similar to how "Test Coverage" (lines 928-970) handles failures.🛡️ Suggested defensive wrapper
stage("Upload Build Info") { + try { def branch = env.gitlabBranch ? env.gitlabBranch : "main" if (globalVars[GITHUB_PR_API_URL]) { branch = "github-pr-" + globalVars[GITHUB_PR_API_URL].split('/').last() } def buildInfo = "commit=${env.gitlabCommit}\n" + "branch=${branch}\n" + "date=${new Date().format('yyyy-MM-dd HH:mm:ss z', TimeZone.getTimeZone('UTC'))}\n" + "jenkins_url=${env.BUILD_URL}" writeFile file: 'build_info.txt', text: buildInfo trtllm_utils.uploadArtifacts("build_info.txt", "${UPLOAD_PATH}/") echo "Build info: https://urm.nvidia.com/artifactory/${UPLOAD_PATH}/build_info.txt" + } catch (Exception e) { + echo "Upload Build Info failed: ${e.toString()}" + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/L0_MergeRequest.groovy` around lines 1368 - 1380, The "Upload Build Info" post-action should be made resilient to upload failures: wrap the call to trtllm_utils.uploadArtifacts (inside the stage "Upload Build Info") in a try-catch so that network or artifact errors do not fail the post block or change the final build status; keep writeFile for 'build_info.txt' and the echo of the artifact URL, and in the catch log a clear error (using echo or existing logger) including the caught exception message so failures are recorded but do not throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1368-1380: The "Upload Build Info" post-action should be made
resilient to upload failures: wrap the call to trtllm_utils.uploadArtifacts
(inside the stage "Upload Build Info") in a try-catch so that network or
artifact errors do not fail the post block or change the final build status;
keep writeFile for 'build_info.txt' and the echo of the artifact URL, and in the
catch log a clear error (using echo or existing logger) including the caught
exception message so failures are recorded but do not throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9abc5a4b-24c8-4ab4-92bc-626c2b6cac76
📒 Files selected for processing (1)
jenkins/L0_MergeRequest.groovy
Collaborator
|
PR_Github #38236 [ run ] triggered by Bot. Commit: |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
jenkins/L0_MergeRequest.groovy (1)
1368-1380: LGTM! The stage correctly runs in post-always and resolves branch for GitHub PRs.The implementation properly:
- Runs in
post { always }ensuring execution even on failure- Resolves branch using
globalVars[GITHUB_PR_API_URL]for GitHub PRsOptional refactor: The branch computation logic (lines 1369-1372) is duplicated from the Build-Docker-Images stage (lines 1283-1286). Consider extracting to a helper function if this pattern grows.
♻️ Optional: Extract branch computation to helper
Add helper function near other utility functions:
def getSourceBranch(globalVars) { def branch = env.gitlabBranch ? env.gitlabBranch : "main" if (globalVars[GITHUB_PR_API_URL]) { branch = "github-pr-" + globalVars[GITHUB_PR_API_URL].split('/').last() } return branch }Then simplify both usages:
-def branch = env.gitlabBranch ? env.gitlabBranch : "main" -if (globalVars[GITHUB_PR_API_URL]) { - branch = "github-pr-" + globalVars[GITHUB_PR_API_URL].split('/').last() -} +def branch = getSourceBranch(globalVars),
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jenkins/L0_MergeRequest.groovy` around lines 1368 - 1380, Extract the duplicated branch resolution logic into a helper function (e.g., def getSourceBranch(globalVars)) that returns env.gitlabBranch ?: "main" but when globalVars[GITHUB_PR_API_URL] exists returns "github-pr-" + globalVars[GITHUB_PR_API_URL].split('/').last(); then replace the inline logic in the Upload Build Info stage and the Build-Docker-Images stage with a call to getSourceBranch(globalVars) to avoid duplication and keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1368-1380: Extract the duplicated branch resolution logic into a
helper function (e.g., def getSourceBranch(globalVars)) that returns
env.gitlabBranch ?: "main" but when globalVars[GITHUB_PR_API_URL] exists returns
"github-pr-" + globalVars[GITHUB_PR_API_URL].split('/').last(); then replace the
inline logic in the Upload Build Info stage and the Build-Docker-Images stage
with a call to getSourceBranch(globalVars) to avoid duplication and keep
behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a930fa26-7c79-4195-ab7e-50c9590c6828
📒 Files selected for processing (1)
jenkins/L0_MergeRequest.groovy
mzweilz
force-pushed
the
user/abbyw/dev-abbyw-fix-post-merge-build-job-1
branch
from
f392692 to
ddd4a6b
Compare
Collaborator
|
PR_Github #38236 [ run ] completed with state
|
Collaborator
Author
|
/bot run |
mzweilz
force-pushed
the
user/abbyw/dev-abbyw-fix-post-merge-build-job-1
branch
from
ddd4a6b to
ef94fa8
Compare
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #38240 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #38241 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #38241 [ run ] completed with state
|
- Resolve branch using globalVars[GITHUB_PR_API_URL] for GithubPR jobs - Move Upload Build Info to post always so it runs even on failure Signed-off-by: Abby Wei <mengzew@nvidia.com>
mzweilz
force-pushed
the
user/abbyw/dev-abbyw-fix-post-merge-build-job-1
branch
from
ef94fa8 to
28ddc9a
Compare
Collaborator
Author
|
/bot run |
Collaborator
|
PR_Github #38415 [ run ] triggered by Bot. Commit: |
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
3 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
Description
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
To see a list of available CI bot commands, please comment
/bot help.