[TRTLLMINF-218][infra] Gate multi-GPU CI stages behind 'ci: full pre-merge approved' label - #16578
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds approval-label validation, gates pre-merge x86_64 and SBSA multi-GPU Jenkins stages, supports configurable single-GPU routing for selected jobs, and updates bot help and CI documentation. ChangesMulti-GPU approval label enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant ApprovalWorkflow
participant TeamMembershipAPI
participant Jenkins
participant MultiGPUStages
GitHub->>ApprovalWorkflow: Apply ci: full pre-merge approved
ApprovalWorkflow->>GitHub: Fetch latest label event
ApprovalWorkflow->>TeamMembershipAPI: Verify labeling actor
TeamMembershipAPI-->>ApprovalWorkflow: Return membership status
ApprovalWorkflow->>GitHub: Remove label and comment if unverified
Jenkins->>GitHub: Validate pull-request approval label
GitHub-->>Jenkins: Return label authorization status
Jenkins->>MultiGPUStages: Continue or fail pre-merge multi-GPU stage
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
jenkins/L0_MergeRequest.groovy (1)
1574-1589: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated multi-GPU label gate into a helper. Both stages carry identical gate logic (label check,
currentBuild.descriptionmarker, anderrormessage) differing only by thex86_64/SBSAarch string. Fold it into one helper (e.g.requireMultiGpuApprovalLabel(pipeline, globalVars, arch)) so the exemption rules, marker HTML, and error text stay in sync.
jenkins/L0_MergeRequest.groovy#L1574-L1589: replace the inline x86_64 gate with a call to the shared helper, passing"x86_64".jenkins/L0_MergeRequest.groovy#L1701-L1716: replace the inline SBSA gate with the same helper, passing"SBSA".🤖 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_MergeRequest.groovy` around lines 1574 - 1589, Extract the duplicated multi-GPU approval gate into a shared requireMultiGpuApprovalLabel helper that accepts pipeline, globalVars, and arch, preserving the GitHub PR and PostMerge exemptions, label check, description marker, and arch-specific error text. Replace the inline gate at jenkins/L0_MergeRequest.groovy lines 1574-1589 with a helper call using "x86_64", and the gate at lines 1701-1716 with a call using "SBSA".
🤖 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 `@docs/source/developer-guide/ci-overview.md`:
- Line 108: Add the repository-required NVIDIA copyright header at the beginning
of ci-overview.md and set its year to 2026, leaving the existing “Multi-GPU
Tests” documentation unchanged.
- Line 108: Add the Multi-GPU Tests heading to the table of contents using the
`#multi-gpu-tests` anchor, then renumber all subsequent entries to preserve the
document’s section order.
- Around line 119-123: Update the rerun instructions near the multi-GPU command
to preserve the originally selected mode: tell readers to rerun the same command
they used, and include distinct examples for --only-multi-gpu-test and
--add-multi-gpu-test so the broader pipeline is not selected unintentionally.
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 660-668: Update the curl invocation in the pipeline.sh call to
include the --fail option alongside --silent, ensuring HTTP error responses
produce a non-zero exit status and reach the existing fail-open catch handler.
Leave the JSON parsing and normal successful-response behavior unchanged.
---
Nitpick comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1574-1589: Extract the duplicated multi-GPU approval gate into a
shared requireMultiGpuApprovalLabel helper that accepts pipeline, globalVars,
and arch, preserving the GitHub PR and PostMerge exemptions, label check,
description marker, and arch-specific error text. Replace the inline gate at
jenkins/L0_MergeRequest.groovy lines 1574-1589 with a helper call using
"x86_64", and the gate at lines 1701-1716 with a call using "SBSA".
🪄 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: 1987f20b-6a09-48aa-a110-cdf448988613
📒 Files selected for processing (4)
.github/workflows/bot-command.yml.github/workflows/full-premerge-approval.ymldocs/source/developer-guide/ci-overview.mdjenkins/L0_MergeRequest.groovy
9f82671 to
eb669b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/full-premerge-approval.yml:
- Around line 157-222: The cleanup flow around the timeline re-check and
issues.removeLabel must reconcile the latest approval event after deletion to
detect an intervening authorized label addition, restore that verified newer
approval label, and stop without posting a denial comment. Track whether this
run actually removed the unauthorized state, and only create the denial comment
when removal succeeded without a valid newer approval; handle concurrent 404
removals accordingly.
🪄 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: 50965d17-a352-4c2b-ab3d-fc2ceeadfadd
📒 Files selected for processing (4)
.github/workflows/bot-command.yml.github/workflows/full-premerge-approval.ymldocs/source/developer-guide/ci-overview.mdjenkins/L0_MergeRequest.groovy
🚧 Files skipped from review as they are similar to previous changes (3)
- jenkins/L0_MergeRequest.groovy
- .github/workflows/bot-command.yml
- docs/source/developer-guide/ci-overview.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/L0_Test.groovy`:
- Around line 2082-2085: Populate MULTI_GPU_RUN_WITH_SINGLE with the intended
multi-GPU stage name patterns so matching stages can be exempted from
approval-label gating. Preserve the existing relocation logic at
jenkins/L0_Test.groovy lines 5893-5901; no direct change is required there.
🪄 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: e8189e40-30e3-41f8-8ab4-6092445a46ba
📒 Files selected for processing (2)
jenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovy
8878d27 to
408f423
Compare
|
/bot run --add-multi-gpu-test |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/L0_MergeRequest.groovy`:
- Line 1620: Move the requireMultiGpuApprovalLabel(...) call outside and before
the surrounding try blocks at both jenkins/L0_MergeRequest.groovy lines
1620-1620 and 1737-1737, leaving each approval failure uncaught by the ignore
handlers so denial remains blocking.
In `@jenkins/L0_Test.groovy`:
- Around line 5940-5950: Remove the unconditional test-only bypass block
surrounding the if (true) statement, including its forced multi-GPU marker,
logging, and early return. Preserve the normal singleGpuJobs execution flow in
the surrounding test logic.
🪄 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: 350e4bd0-4cef-42c1-9276-1f4d6bac30e2
📒 Files selected for processing (5)
.github/workflows/bot-command.yml.github/workflows/full-premerge-approval.ymldocs/source/developer-guide/ci-overview.mdjenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovy
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/bot-command.yml
- docs/source/developer-guide/ci-overview.md
|
PR_Github #61546 [ run ] triggered by Bot. Commit: |
|
PR_Github #61546 [ run ] completed with state
|
408f423 to
e14a237
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #61559 [ run ] triggered by Bot. Commit: |
|
PR_Github #61559 [ run ] completed with state
|
e14a237 to
e157bcf
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #61565 [ run ] triggered by Bot. Commit: |
|
PR_Github #61565 [ run ] completed with state
|
e157bcf to
94fbb1c
Compare
|
/bot run --add-multi-gpu-test |
|
PR_Github #61801 [ run ] triggered by Bot. Commit: |
|
/bot run --add-multi-gpu-test |
|
PR_Github #62358 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-multi-gpu-test |
|
PR_Github #62400 [ run ] triggered by Bot. Commit: |
|
PR_Github #62358 [ run ] completed with state |
|
PR_Github #62400 [ run ] completed with state
|
|
/bot run --disable-multi-gpu-test |
|
PR_Github #62732 [ run ] triggered by Bot. Commit: |
ZhanruiSunCh
added
the
ci: post-merge approved
github-actions
Bot
removed
the
ci: post-merge approved
|
Removed the "ci: post-merge approved" label because @ZhanruiSunCh could not be verified as an active member of NVIDIA/trt-llm-ci-approvers. Ask a member of that team to apply it. |
|
PR_Github #62732 [ run ] completed with state |
|
@CodeRabbit code review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
jenkins/L0_MergeRequest.groovy (1)
1617-1629: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated "Blocked" stage scaffolding into a shared helper.
Both the x86_64 and SBSA multi-GPU gates repeat the same
requireMultiGpuApprovalLabel()→ check →stage("[...] Blocked") { catchError(...) { error ... } }→returnpattern. Consolidating into one helper reduces duplication and keeps blocked-stage behavior consistent if it needs to change later.
jenkins/L0_MergeRequest.groovy#L1617-L1629: replace with a call to a sharedblockMultiGpuStageIfNeeded(pipeline, globalVars, "x86_64")helper that encapsulates the gate check,Blockedstage creation, and early return.jenkins/L0_MergeRequest.groovy#L1746-L1755: replace with the same shared helper call for"SBSA".♻️ Proposed helper
def blockMultiGpuStageIfNeeded(pipeline, globalVars, String arch) { def blockMsg = requireMultiGpuApprovalLabel(pipeline, globalVars, arch) if (blockMsg) { stage("[Test-${arch}-Multi-GPU] Blocked") { catchError(buildResult: 'UNSTABLE', stageResult: 'FAILURE') { error blockMsg } } return true } return false }Then at each call site:
- def x86LabelBlock = requireMultiGpuApprovalLabel(pipeline, globalVars, "x86_64") - if (x86LabelBlock) { - stage("[Test-x86_64-Multi-GPU] Blocked") { - catchError(buildResult: 'UNSTABLE', stageResult: 'FAILURE') { - error x86LabelBlock - } - } - return - } + if (blockMultiGpuStageIfNeeded(pipeline, globalVars, "x86_64")) { + return + }🤖 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_MergeRequest.groovy` around lines 1617 - 1629, Extract the duplicated multi-GPU label-gate handling into a shared blockMultiGpuStageIfNeeded(pipeline, globalVars, arch) helper that performs requireMultiGpuApprovalLabel, creates the appropriate Blocked stage with catchError/error, and returns whether execution was blocked. Replace the x86_64 gate at jenkins/L0_MergeRequest.groovy lines 1617-1629 and the SBSA gate at lines 1746-1755 with calls to this helper, returning early when it reports a block.
🤖 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/L0_MergeRequest.groovy`:
- Around line 647-693: The currentBuild.description update in
requireMultiGpuApprovalLabel() is not synchronized across parallel x86_64 and
SBSA branches. Protect the existing description-read and append assignment with
a Jenkins lock or equivalent shared mutex, while preserving the marker content
and block-reason behavior.
---
Nitpick comments:
In `@jenkins/L0_MergeRequest.groovy`:
- Around line 1617-1629: Extract the duplicated multi-GPU label-gate handling
into a shared blockMultiGpuStageIfNeeded(pipeline, globalVars, arch) helper that
performs requireMultiGpuApprovalLabel, creates the appropriate Blocked stage
with catchError/error, and returns whether execution was blocked. Replace the
x86_64 gate at jenkins/L0_MergeRequest.groovy lines 1617-1629 and the SBSA gate
at lines 1746-1755 with calls to this helper, returning early when it reports a
block.
🪄 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: 020ec368-67ad-4fe6-9b76-fd5e5f36a676
📒 Files selected for processing (5)
.github/workflows/bot-command.yml.github/workflows/full-premerge-approval.ymldocs/source/developer-guide/ci-overview.mdjenkins/L0_MergeRequest.groovyjenkins/L0_Test.groovy
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/developer-guide/ci-overview.md
…merge approved' label Gate x86_64 and SBSA multi-GPU pre-merge CI stages behind the GitHub label 'ci: full pre-merge approved' so resource-intensive multi-GPU tests only run when explicitly approved by a CI approver. Changes: - L0_MergeRequest.groovy: add hasGithubPRLabel() (fail-open, reuses github-cred-trtllm-ci); insert label check at the top of [Test-x86_64-Multi-GPU] and [Test-SBSA-Multi-GPU] Remote Run stages; write description marker so wrapper can surface call-to-action - full-premerge-approval.yml: new GitHub Actions workflow (modeled after post-merge-approval.yml) to remove the label and post a deny comment when an unauthorized user applies the label - bot-command.yml: document label requirement for --add-multi-gpu-test and --only-multi-gpu-test - ci-overview.md: add Multi-GPU Tests section explaining label gating PostMerge pipelines and GitLab MR builds are exempt from the check. Wrapper (trtllm-github-ci) change tracked in GitLab branch trtllminf-218-full-premerge-label (based on MR NVIDIA#86). Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
- hasGithubPRLabel: add --fail --show-error to curl so HTTP errors (401/403/5xx) exit non-zero and reach the catch/fail-open handler instead of returning empty HTML that blocks CI - Extract duplicated multi-GPU label gate logic into shared requireMultiGpuApprovalLabel() helper - ci-overview.md: add NVIDIA copyright header, add Multi-GPU Tests entry to TOC, clarify rerun instructions for both --add-multi-gpu-test and --only-multi-gpu-test Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
… add exempt stage list - L0_MergeRequest.groovy: replace hasGithubPRLabel() with trtllm_utils.validatePRLabelApproval() from the shared lib, which verifies both label existence and labeler team membership; error message now distinguishes "label not present" from "labeler not authorized"; fail-open on API errors; echo reason on early return - L0_Test.groovy: add MULTI_GPU_RUN_WITH_SINGLE exempt list; matching multi-GPU stages run inside the single-GPU job instead of waiting for the separate multi-GPU dispatch (which requires the label) Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
…ror message - requireMultiGpuApprovalLabel() now returns the block reason string (instead of boolean true) so callers can display the exact cause - Blocked stage error message now shows whether the label is missing or was applied by an unauthorized user, plus guidance to contact NVIDIA/trt-llm-ci-approvers Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
…mal stages - Switch team slug to trt-llm-infra-devs for label validation testing - Disable all x86 Kubernetes test stages (only doc build runs) - Keep one multi-GPU SLURM stage to trigger the label gate marker - Revert all [TEST] markers before merge Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
…and minimal stages" This reverts commit d30a4d9. Signed-off-by: ZhanruiSunCh <184402041+ZhanruiSunCh@users.noreply.github.com>
2dff6e8 to
7703c47
Compare
|
/bot reuse-pipeline |
|
PR_Github #63412 [ reuse-pipeline ] triggered by Bot. Commit: |
|
PR_Github #63412 [ reuse-pipeline ] completed with state |
3 participants
Summary
Gate x86_64 and SBSA multi-GPU pre-merge CI stages behind the GitHub label
ci: full pre-merge approved. Only active members ofNVIDIA/trt-llm-ci-approverscan authorize this label.Key Changes
jenkins/L0_MergeRequest.groovyrequireMultiGpuApprovalLabel()— calls shared lib to verify label existence + labeler team membership; shows[Blocked]stage on failure (UNSTABLE, no fail-fast)jenkins/L0_Test.groovyMULTI_GPU_RUN_WITH_SINGLE— configurable exempt list; matching multi-GPU stages run in single-GPU job without label.github/workflows/full-premerge-approval.yml.github/workflows/bot-command.yml--add-multi-gpu-test/--only-multi-gpu-testdocs/source/developer-guide/ci-overview.mdtrtllm-jenkins-shared-lib(MR #84)validatePRLabelApproval()— unified label + author validationtrtllm-github-ci(MR #86)Design Decisions
catchError(buildResult: 'UNSTABLE'), does not kill parallel branches (e.g. SBSA build)validatePRLabelApproval()checks both label existence and labeler is active team memberci: post-merge approved)Test Coverage
A. GitHub Actions Label Guard
B. Jenkins Multi-GPU Gate
Triggered by: file change auto-detection /
--add-multi-gpu-test/--only-multi-gpu-test/--post-merge/--stage-listwith multi-GPU stage[Test-x86_64-Multi-GPU] Blocked(red)[Test-x86_64-Multi-GPU] Blocked(red)[Test-x86_64-Multi-GPU] Remote RunnormalBlocked stage error messages:
x86_64 Multi-GPU tests blocked: label 'ci: full pre-merge approved' is not present on this PR. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI.x86_64 Multi-GPU tests blocked: label 'ci: full pre-merge approved' was applied by 'xxx' who is not an active member of NVIDIA/trt-llm-ci-approvers. Ask a member of NVIDIA/trt-llm-ci-approvers to add the label, then re-trigger CI.SBSA behavior is identical (stage name uses
SBSAinstead ofx86_64).C. Exemptions (label check skipped)
Skipping label check: PostMerge pipeline is exemptSkipping label check: not a GitHub PR (no GITHUB_PR_API_URL)/bot run)D. MULTI_GPU_RUN_WITH_SINGLE
PR Checklist
Dev Engineer Review
ci: full pre-merge approvedlabel, including unauthorized-label removal and safe handling of newer approval events./bot runhelp text and CI documentation, includingPerfSanitypost-merge selectors.QA Engineer Review
No test changes.