Add last aggregator job in unit/gpu CI test to use as required check for PRs #292
Conversation
|
Warning Rate limit exceeded@kevalmorabia97 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCompute PR merge-base for diff checks, set checkout fetch-depth to 0, make changed-tests tolerate failures and use the computed base, enable fail_on_initial_diff_error for changed-files, add a PR-scoped GPU-required guard that fails when test-relevant changes exist and GPU PR tests didn't succeed, and add a PR-scoped unit aggregator job that echoes completion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PR as PR Event
participant CM as calculate-merge-base
participant CO as checkout (fetch-depth:0)
participant CF as check-file-changes
participant CT as changed-tests
participant CFS as changed-files
participant GPU as gpu-tests-pr
participant GUARD as gpu-pr-required-check
PR->>CO: checkout PR (fetch-depth: 0)
PR->>CM: compute merge-base (head.sha, base.sha)
CM-->>CF: provide merge-base
CF->>CT: run changed-tests (continue-on-error: true, base_sha=merge-base)
CF->>CFS: run changed-files (fail_on_initial_diff_error: true)
PR->>GPU: run gpu-tests-pr
CF-->>GUARD: outputs.any_changed
GPU-->>GUARD: result status
alt any_changed == 'true' AND gpu-tests-pr != success
GUARD->>GUARD: exit 1 (fail workflow)
else
GUARD->>PR: allow/continue
end
sequenceDiagram
autonumber
participant PR as PR Event
participant L as linux
participant W as windows
participant MP as multi-py
participant MT as multi-torch
participant MTr as multi-transformers
participant PI as partial-install
participant AGG as unit-pr-required-check
PR->>L: run
PR->>W: run
PR->>MP: run
PR->>MT: run
PR->>MTr: run
PR->>PI: run
par All unit jobs complete
L-->>AGG: done
W-->>AGG: done
MP-->>AGG: done
MT-->>AGG: done
MTr-->>AGG: done
PI-->>AGG: done
end
AGG->>PR: echo "All PR unit test jobs completed"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
kevalmorabia97
changed the title
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
a888a83 to
fc6b2b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
setup.py (1)
16-16: Nit: capitalize brand and clarify docstring.Suggest aligning with project naming style elsewhere in the file (uses "NVIDIA").
-"""The package setup script for nvidia-modelopt.""" +"""Package setup script for NVIDIA Model Optimizer (nvidia-modelopt).""".github/workflows/unit_tests.yml (1)
129-134: Make the PR “required check” job explicit and resilient (run always, fail on unmet deps).Today, if a needed job fails, this job is skipped (which still blocks merges but looks ambiguous). Mirror the GPU pattern: always run on PRs and explicitly fail when any dependency isn’t success. Also keeps the check green when everything passes.
- unit-pr-required-check: - if: github.event_name == 'pull_request' + unit-pr-required-check: + if: ${{ github.event_name == 'pull_request' && always() }} needs: [linux, windows, multi-py, multi-torch, multi-transformers, partial-install] runs-on: ubuntu-latest steps: - - run: echo "All PR unit test jobs completed" + - run: echo "All PR unit test jobs completed" + - name: Fail if any required unit-test job did not succeed + if: ${{ needs.linux.result != 'success' + || needs.windows.result != 'success' + || needs['multi-py'].result != 'success' + || needs['multi-torch'].result != 'success' + || needs['multi-transformers'].result != 'success' + || needs['partial-install'].result != 'success' }} + run: exit 1.github/workflows/gpu_tests.yml (1)
73-81: Good pattern for a PR-gating “required check”.Runs regardless of upstream outcome and fails only when GPU tests were required but not successful. Looks correct.
Optionally add a final informational step so the job shows a positive message when passing:
steps: - name: Required GPU tests did not succeed if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} run: exit 1 + - name: GPU required check passed + if: ${{ needs.check-file-changes.outputs.any_changed != 'true' || needs.gpu-tests-pr.result == 'success' }} + run: echo "GPU required check conditions satisfied"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/gpu_tests.yml(1 hunks).github/workflows/unit_tests.yml(1 hunks)setup.py(1 hunks)
⏰ 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). (3)
- GitHub Check: linux
- GitHub Check: build-docs
- GitHub Check: code-quality
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
fc6b2b2 to
b94ad1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/gpu_tests.yml (1)
74-82: Guard job pattern LGTM; minor hardening suggestions.The aggregator logic properly fails when relevant changes exist and GPU tests didn’t succeed. Consider:
- Add least-privilege permissions.
- Add a small timeout to avoid rare hangs.
- Optional: add a success echo step for clearer logs.
Example:
gpu-pr-required-check: # Run even if gpu-tests-pr is skipped if: ${{ startsWith(github.ref, 'refs/heads/pull-request/') && always() }} needs: [check-file-changes, gpu-tests-pr] runs-on: ubuntu-latest + permissions: {} + timeout-minutes: 5 steps: + - name: All good (no relevant changes or GPU tests succeeded) + if: ${{ needs.check-file-changes.outputs.any_changed != 'true' || needs.gpu-tests-pr.result == 'success' }} + run: echo "GPU checks passed or not applicable" - name: Required GPU tests did not succeed if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} run: exit 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/gpu_tests.yml(2 hunks).github/workflows/unit_tests.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
⏰ 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). (3)
- GitHub Check: linux
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (1)
.github/workflows/gpu_tests.yml (1)
39-39: Ignore invalid suggestion:base_shaaccepts branch names and there is nobase_refinput in step-security/changed-files.Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
=======================================
Coverage 73.95% 73.95%
=======================================
Files 172 172
Lines 17405 17405
=======================================
Hits 12871 12871
Misses 4534 4534 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
b94ad1b to
458c054
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/gpu_tests.yml (3)
30-30: OK to tolerate action failure; minor hardening.continue-on-error here works with your fallback. Consider pinning step-security/changed-files to a commit SHA for supply-chain safety.
85-93: Aggregator gate looks good; add least-privilege + timeout.Reduce blast radius and avoid hangs by tightening permissions and adding a small timeout.
gpu-pr-required-check: # Run even if gpu-tests-pr is skipped if: ${{ startsWith(github.ref, 'refs/heads/pull-request/') && always() }} needs: [check-file-changes, gpu-tests-pr] runs-on: ubuntu-latest + permissions: {} + timeout-minutes: 5 steps: - name: Required GPU tests did not succeed if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} run: exit 1
6-6: Branch pattern likely too strict.GitHub’s glob doesn’t treat + as “one or more”. If you want pull-request/1234, prefer either of:
- pull-request/** (broad)
- pull-request/[0-9][0-9]* (digits)
- branches: ["pull-request/[0-9]+"] + branches: ["pull-request/[0-9][0-9]*"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/gpu_tests.yml(3 hunks).github/workflows/unit_tests.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
⏰ 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). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (4)
.github/workflows/gpu_tests.yml (4)
22-22: Output plumbing LGTM.Switching to steps.compute-any-changed.outputs.any_changed is correct and resilient.
40-41: Conservative fallback confirmation.Setting fail_on_initial_diff_error: true ensures the step fails on force-push diffs and your fallback triggers. If you intended to avoid running GPU tests on ambiguous diffs, flip this to false.
42-50: Fallback computation is robust.always() + outcome check + GITHUB_OUTPUT is correct. No changes needed.
39-39: No changes required:base_shaaccepts branch refs
Thestep-security/[email protected]action’sbase_shainput accepts either a commit SHA or a branch ref (e.g., “main”), so usingfromJSON(...).base.refis valid. [1][2]Likely an incorrect or invalid review comment.
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
458c054 to
29705eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/gpu_tests.yml (4)
30-30: Toleratingchanged-fileserrors is fine given the fallback
continue-on-error: trueis appropriate here since you compute a safe fallback right after. Consider adding a brief log line when it fails to aid triage, but not required.
40-41: Use PR base SHA, not ref name, to reduce diff flakiness
base_shashould be the base commit SHA, not the branch ref. Using the ref can cause intermittent “initial diff” errors and needlessly force tests to run.Apply this change (outside the changed-line range) to prefer the exact base commit:
with: files: | # …unchanged… base_sha: ${{ fromJSON(steps.get-pr-info.outputs.pr-info).base.sha }} # Optional: keep the comment and fail_on_initial_diff_error as-is
42-50: Minor hardening of the fallback stepExplicit shell + strict mode improves robustness; otherwise logic is solid.
- - name: Compute any_changed (fallback to true on failure) + - name: Compute any_changed (fallback to true on failure) id: compute-any-changed if: ${{ always() }} - run: | - if [ "${{ steps.changed-tests.outcome }}" != "success" ]; then - echo "any_changed=true" >> "$GITHUB_OUTPUT" - else - echo "any_changed=${{ steps.changed-tests.outputs.any_changed }}" >> "$GITHUB_OUTPUT" - fi + shell: bash + run: | + set -euo pipefail + if [ "${{ steps.changed-tests.outcome }}" != "success" ]; then + echo "any_changed=true" >> "$GITHUB_OUTPUT" + else + val="${{ steps.changed-tests.outputs.any_changed }}" + echo "any_changed=${val:-true}" >> "$GITHUB_OUTPUT" + fi
85-93: Aggregator job is correct; add least-privilege and alignifstyleJob correctly fails when tests are required but didn’t succeed. Two small tweaks:
- Set
permissions: {}to avoid granting an unnecessary token.- Optional: match earlier jobs’
if:style (no${{ }}wrapper).gpu-pr-required-check: # Run even if gpu-tests-pr is skipped - if: ${{ startsWith(github.ref, 'refs/heads/pull-request/') && always() }} + if: startsWith(github.ref, 'refs/heads/pull-request/') && always() needs: [check-file-changes, gpu-tests-pr] runs-on: ubuntu-latest + permissions: {} steps: - name: Required GPU tests did not succeed if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} run: exit 1Also, please ensure branch protection marks this check as required using the exact name “GPU tests / gpu-pr-required-check”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/gpu_tests.yml(3 hunks).github/workflows/unit_tests.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
⏰ 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). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (1)
.github/workflows/gpu_tests.yml (1)
22-22: Output rewiring to computed value looks goodSourcing
outputs.any_changedfrom the new compute step is correct and prevents silent green whenchanged-filesflakes. No further changes needed.
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
29705eb to
69523f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/gpu_tests.yml (1)
83-91: Make required-check failure explicit and hook to synthesized output.Emit a clear message/summary so maintainers know why it failed. If you adopt the synthesized output, reference it here.
Apply:
- gpu-pr-required-check: + gpu-pr-required-check: # Run even if gpu-tests-pr is skipped if: ${{ startsWith(github.ref, 'refs/heads/pull-request/') && always() }} needs: [check-file-changes, gpu-tests-pr] runs-on: ubuntu-latest steps: - name: Required GPU tests did not succeed - if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} - run: exit 1 + if: ${{ needs.check-file-changes.outputs.any_changed == 'true' && needs.gpu-tests-pr.result != 'success' }} + run: | + echo "GPU PR tests must pass when test‑relevant files change." + echo "any_changed=${{ needs.check-file-changes.outputs.any_changed }}, gpu-tests-pr=${{ needs.gpu-tests-pr.result }}" + echo "GPU PR tests did not succeed for test‑relevant changes." >> "$GITHUB_STEP_SUMMARY" + exit 1If you adopt the synthesized output key name, no change needed here since it remains needs.check-file-changes.outputs.any_changed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/gpu_tests.yml(2 hunks).github/workflows/unit_tests.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
⏰ 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). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: code-quality
- GitHub Check: build-docs
🔇 Additional comments (1)
.github/workflows/gpu_tests.yml (1)
83-86: Confirm branch-naming assumption.The guard relies on refs/heads/pull-request/*. Please confirm all PR mirror branches follow this pattern; otherwise, the required check may never run.
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
69523f1 to
bb6a568
Compare
Signed-off-by: Keval Morabia <[email protected]>
kevalmorabia97
force-pushed
the
kmorabia/add-ci-requirement-aggregator
branch
from
bb6a568 to
13106c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/gpu_tests.yml (1)
25-26: LGTM: full git history enables reliable merge-base.Setting fetch-depth: 0 fixes shallow-clone issues with git merge-base. Matches prior feedback.
🧹 Nitpick comments (3)
.github/workflows/gpu_tests.yml (3)
29-35: Guard against empty merge-base (nit).Consider failing fast if merge-base is empty to avoid downstream null outputs.
- run: | - (echo -n "merge-base="; git merge-base "$BASE_SHA" "$PR_SHA") | tee --append "${GITHUB_OUTPUT}" + run: | + set -euo pipefail + mb="$(git merge-base "$BASE_SHA" "$PR_SHA" || true)" + if [ -z "${mb:-}" ]; then + echo "Failed to compute merge-base" >&2 + exit 1 + fi + echo "merge-base=$mb" | tee --append "${GITHUB_OUTPUT}"
40-42: Optional: synthesize any_changed on diff errors to avoid silent bypass.If the changed-files step errors, you can set any_changed=true to force gating rather than relying on job failure propagation.
- name: Check for changes in test-relevant directories id: changed-tests uses: step-security/[email protected] with: base_sha: ${{ steps.calculate-merge-base.outputs.merge-base }} sha: ${{ fromJSON(steps.get-pr-info.outputs.pr-info).head.sha }} files: | .github/workflows/gpu_tests.yml modelopt/** tests/gpu/** tox.ini pyproject.toml setup.py fail_on_initial_diff_error: true + - name: Synthesize any_changed (treat failures as changed) + id: compute-any-changed + run: | + if [ "${{ steps.changed-tests.outcome }}" != "success" ]; then + echo "any_changed=true" >> "$GITHUB_OUTPUT" + else + echo "any_changed=${{ steps.changed-tests.outputs.any_changed }}" >> "$GITHUB_OUTPUT" + fiThen expose that as the job output:
- outputs: - any_changed: ${{ steps.changed-tests.outputs.any_changed }} + outputs: + any_changed: ${{ steps.compute-any-changed.outputs.any_changed }}Also applies to: 49-49
24-28: Pin third-party actions to commit SHAs (supply-chain hardening).actions/checkout@v4 is fine, but get-pr-info@main and step-security/[email protected] are mutable tags. Pin to immutable SHAs.
- - id: get-pr-info - uses: nv-gha-runners/get-pr-info@main + - id: get-pr-info + uses: nv-gha-runners/get-pr-info@<pinned-commit-sha> - uses: step-security/[email protected] + uses: step-security/changed-files@<pinned-commit-sha> - - uses: nv-gha-runners/setup-proxy-cache@main + - uses: nv-gha-runners/setup-proxy-cache@<pinned-commit-sha>Also applies to: 38-38, 73-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/gpu_tests.yml(2 hunks).github/workflows/unit_tests.yml(1 hunks)
⏰ 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). (4)
- GitHub Check: linux
- GitHub Check: wait-checks / wait
- GitHub Check: build-docs
- GitHub Check: code-quality
Signed-off-by: Keval Morabia <[email protected]>
What does this PR do?
Type of change: CI fixes
Overview:
Testing
Verified CI run
Summary by CodeRabbit
Tests
Chores