Skip to content

ci(nightly): preserve artifact handoffs while fixing skipped validation - #250

Merged
jasonqinzhou merged 8 commits into
mainfrom
pvijayakrish/fix-nightly-gate-skip
Sep 17, 2026
Merged

jasonqinzhou merged 8 commits into
mainfrom
pvijayakrish/fix-nightly-gate-skip

Conversation

@pvijayakrish

@pvijayakrish pvijayakrish commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Scheduled nightlies skip the manual-approval ancestor, which can also suppress downstream validation through Actions' implicit success() condition. Give license-evidence and fpe-support-matrix explicit !cancelled() predicates with successful dependency requirements so they run after an unattended successful build and still reject cancellation or failed inputs.

The verified nightly-dist wheel, checksum, and provenance artifacts remain available to FPE, accuracy, and installation consumers. FPE continues to qualify the same wheel downloaded from Artifactory.

Validation at 9bf86e2743a22f1c6ddcfa0a8ad5873f7f0a5f0d, based on main 6c07147a after #241: 222 workflow/qualification/release tests pass under root pytest.ini warnings-as-errors. The focused accuracy consumer suite has 8 passing Node tests; Ruff, actionlint, and whitespace checks pass. Regression coverage includes skipped approval ancestors, cancellation, failed/skipped/cancelled dependencies, and unchanged-source suppression. Both review threads are fixed and resolved. CodeRabbit, Fast CI, and Full CI passed at this exact head. Squash-merged as dfa8a1fdac8374058b91d797e8a8bc433b05f406; the merged tree was verified identical to the tested head.

jasonqinzhou and others added 3 commits September 16, 2026 17:58
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Remove the nightly-dist uploads — Artifactory is the only channel.
FPE builds its own qualification wheel at the same SHA, and e2e
accuracy always builds from source. Also override the implicit
success() on license-evidence and fpe-support-matrix so the approval
job skipped on scheduled runs cannot cascade and skip the GitLab
trigger.
@pvijayakrish
pvijayakrish requested a review from a team as a code owner September 17, 2026 14:11
@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 590c0170-8c3b-44cc-9d31-5a0c0d97b73a

📥 Commits

Reviewing files that changed from the base of the PR and between b44fda2 and 9bf86e2.

📒 Files selected for processing (2)
  • .github/workflows/nightly-ci.yml
  • tests/test_ci_workflow_contracts.py
🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

  • ai-dynamo/dynamo (manual)
  • ai-dynamo/aiconfigurator (manual)

Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review.

📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (4)
Only root workflows are active.

⚙️ CodeRabbit configuration file

Files:

  • .github/workflows/nightly-ci.yml
Require coverage of the changed behavior and its negative or boundary cases.

⚙️ CodeRabbit configuration file

Files:

  • tests/test_ci_workflow_contracts.py
Read REVIEW.md before commenting.

⚙️ CodeRabbit configuration file

Files:

  • tests/test_ci_workflow_contracts.py
Before making any change under: `python/aisimulate/src/aiconfigurator/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/ais...

📄 CodeRabbit inference engine (AGENTS.md)

Files:

  • tests/test_ci_workflow_contracts.py
🪛 ast-grep (0.45.3)
tests/test_ci_workflow_contracts.py

[info] 1797-1797: use of eval can be insecure
Context: eval(expression, {"builtins": {}})
Note: [CWE-94] Improper Control of Generation of Code ('Code Injection').

(no-eval-python)

🔀 Multi-repo context ai-dynamo/aiconfigurator, ai-dynamo/dynamo

Linked repositories findings

ai-dynamo/aiconfigurator

  • Artifact consumers must verify _MANIFEST.json and _COMPLETE.json; completion includes the manifest checksum. [::ai-dynamo/aiconfigurator::] (README.md:56-58, tools/finalize_wheels_in_artifactory.py:91-100)
  • No direct references to the changed nightly jobs or nightly-dist were found. [::ai-dynamo/aiconfigurator::]

ai-dynamo/dynamo

  • No matching workflow references to the changed nightly jobs, dependency names, or nightly-dist were found. [::ai-dynamo/dynamo::]

📝 Summary

Risk: medium. Human attention should focus on cancellation predicates, dependency-result handling, and final Full CI status.

Changed behavior

  • license-evidence requires a non-cancelled workflow and successful build-artifacts and python-compliance.
  • fpe-support-matrix requires a non-cancelled workflow and successful build-artifacts.
  • Failed, skipped, or cancelled dependencies remain blocked.
  • The nightly-dist-<arch> wheel, checksum, and provenance handoff remains preserved.
  • FPE and accuracy continue to consume the verified staged wheel with installation provenance.

Technical quality

  • Supplied validation includes 19 workflow-contract tests, 8 accuracy-workflow tests, Ruff, and whitespace checks.
  • Regression coverage includes skipped approval, cancellation, failed dependencies, and unchanged-main suppression.
  • Review severity counts are unavailable.
  • The latest shell result reports success but provides no diff or status details. Independent repository verification remains unavailable.

Merge readiness

  • Final Full CI status is not supplied.
  • PR #241 must be integrated before final Full CI closeout because of pre-existing strict-warning license-evidence failures.
  • The /ok to test comment confirms testing authorization, not review approval.

Walkthrough

Nightly CI conditions now block selected jobs after cancellation or unsuccessful required dependencies. Contract tests simulate cancellation and validate dependency and build-selection outcomes.

Changes

Nightly CI guards

Layer / File(s) Summary
Nightly job conditions
.github/workflows/nightly-ci.yml
license-evidence now requires a non-cancelled workflow and successful build-artifacts and python-compliance. fpe-support-matrix now requires a non-cancelled workflow, enabled build selection, and successful build-artifacts.
Condition evaluation tests
tests/test_ci_workflow_contracts.py
The condition evaluator accepts a cancellation override. Tests cover skipped approval, cancellation, failed or skipped dependencies, cancelled dependencies, and disabled build selection.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 9bf86

The nightly jobs now remain blocked after cancelled, skipped, or failed prerequisites while successful scheduled builds can proceed. No current merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 8
✅ Passed checks (8 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cross-Layer Contract ✅ Passed PASS. The authoritative diff changes only two workflow-contract files. It adds no Rust, Python, CLI, serialization, documentation, public input, schema, default, or workflow output interface. The work…
Modeling And Data Evidence ✅ Passed The PR changes workflow selection predicates, not model outputs or performance data. The YAML comments explain the before/after behavior, and the added contract tests reproduce the predicates from the…
Compatibility Boundaries ✅ Passed PASS. The authoritative diff contains only .github/workflows/nightly-ci.yml and workflow-contract tests. It changes job predicates, not Python or Rust surfaces, schemas, versions, bindings, or defau…
Review Evidence ✅ Passed The description names the validation evidence and results: 19 workflow-contract tests, 8 accuracy-workflow tests, Ruff, and whitespace checks. It lists boundary cases for skipped approval, cancellatio…
Title check ✅ Passed The title identifies the nightly CI behavior change: preserve artifact handoffs and prevent validation jobs from being skipped incorrectly.
Description check ✅ Passed The description clearly states the problem, behavior change, affected consumers, preserved artifact handoff, validation commit, test results, regression cases, and remaining merge requirements. It omi…

Comment @coderabbitai help to get the list of available commands.

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current head against main 920453e. The explicit status predicates address the scheduled-run skip cascade. Two artifact-contract regressions need correction before this can merge. I am preparing the fixes on this PR as requested, retaining the nightly consumer handoff and limiting this change to the skip fix. Existing validation also reproduces one Python artifact-contract failure and three accuracy-workflow failures.

Comment thread .github/workflows/nightly-ci.yml Outdated
Comment thread .github/workflows/nightly-ci.yml
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
@jasonqinzhou
jasonqinzhou requested a review from a team as a code owner September 17, 2026 16:42
@copy-pr-bot

copy-pr-bot Bot commented Sep 17, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@jasonqinzhou jasonqinzhou changed the title ci(nightly): fix nightly gate skip ci(nightly): preserve artifact handoffs while fixing skipped validation Sep 17, 2026
@jasonqinzhou jasonqinzhou added the review-ready Ready for automated and human review label Sep 17, 2026 — with ChatGPT Codex Connector

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh Codex review of 07a6b70 against main 920453e: no remaining actionable finding in the final two-file diff. Both artifact-contract regressions are fixed and their threads resolved. The verified wheel/provenance handoff is unchanged from main; explicit !cancelled() predicates preserve required dependency failures while allowing skipped approval ancestors. Local validation: 19 focused Python workflow contracts, 8 Node consumer tests, Ruff, and whitespace checks pass. Current Fast CI is green and CodeRabbit substantively reviewed both files at this exact head with no actionable comments. The broader strict-warning suite has three pre-existing license-evidence resource-warning failures; PR #241 fixes them. Full CI is still required before merge; this review does not waive that gate or the independent approval required on #241.

Copy link
Copy Markdown
Contributor

/ok to test 07a6b70

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refreshed Codex review at b44fda2 after merging main c3cc310. The final two-file diff is unchanged from reviewed 07a6b70; no new actionable finding. The !cancelled() predicates require successful build/compliance inputs and retain the exact verified wheel/checksum/provenance handoffs. Integration with current #241 remains conflict-free. The old-head Full CI run was cancelled after the base advanced; it is not current evidence. Merge order remains #241 then #250 then #251: #241 fixes known strict application-contract failures inherited by the other branches. Current automated review, Full CI, and approval gates still apply.

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technical approval of the final two-file change at b44fda2. The validated review findings are fixed, both threads are resolved, verified artifact handoffs are preserved, and the skip/cancellation/dependency regression tests pass. Current Fast CI is green. This approval does not authorize bypassing the remaining merge gates: land #241's existing contract-failure fixes, refresh this branch, and require completed current-head CodeRabbit and Full CI before merging.

Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>
Signed-off-by: Jason Zhou (Engrg-Hardware 1) <jasonzho@nvidia.com>

@jasonqinzhou jasonqinzhou left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fresh review and approval at 9bf86e2 against main 6c07147 after #241 landed. The final diff remains the two explicit nightly status predicates and their regression tests; no actionable finding remains. The verified wheel/checksum/provenance handoffs are preserved. All 222 workflow/qualification/release tests now pass under warnings-as-errors, including the three previously blocked by #241. The final main-merge commit is tree-identical to that tested local integration. Current-head CodeRabbit, Fast CI, and Full CI must pass before the squash merge.

Copy link
Copy Markdown
Contributor

/ok to test 9bf86e2

@jasonqinzhou
jasonqinzhou merged commit dfa8a1f into main Sep 17, 2026
67 checks passed
@jasonqinzhou
jasonqinzhou deleted the pvijayakrish/fix-nightly-gate-skip branch September 17, 2026 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-ready Ready for automated and human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants