Skip to content

[ci] fix: resolve test matrix from the PR merge commit, not the head - #4486

Merged
ko3n1g merged 1 commit into
mainfrom
ko3n1g/ci/matrix-use-merge-commit-sha
Jun 24, 2026
Merged

[ci] fix: resolve test matrix from the PR merge commit, not the head#4486
ko3n1g merged 1 commit into
mainfrom
ko3n1g/ci/matrix-use-merge-commit-sha

Conversation

@ko3n1g

@ko3n1g ko3n1g commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Background / motivation

A large batch of functional jobs on PR #4466 failed with bash: tests/functional_tests/launch_scripts/.../<NAME>.sh: No such file or directory (exit 127), unrelated to the PR's actual change.

Root cause is a commit-resolution split inside cicd-main.yml:

  • generate-test-matrix / generate-gb200-test-matrix scan a checkout that defaults to github.sha — the PR head branch tip.
  • cicd-container-build checks out and builds from the PR merge_commit_sha (head merged into main), but tags the image with github.sha, which disguised the split. The launch scripts execute inside that container via docker exec (the host checkout is not mounted).

When main renames/recategorizes launch scripts (e.g. #4470 bumped tier prefixes L0→L1→L2), a PR that is behind main scans the stale head-tree names, schedules jobs under those names, and then can't find them in the merge-commit container → exit 127. Every non-renamed script still passes, which is why the failures look scattered.

What changed

  • Resolve the merge-commit SHA once in the configure job and expose it as a merge_sha output.
  • Consume merge_sha throughout the workflow: every source checkout (ref:), the image build tag, and every container-image reference.
  • Remove the now-redundant per-job merge-sha resolution in cicd-container-build.
  • Add configure to the needs: of jobs that now consume the output.

The matrix is now scanned from the same tree the container is built from, so a PR behind a launch-script rename stays green without a rebase.

Details

  • configure: new merge_sha output + Resolve merge commit sha step (PR → get-pr-info.merge_commit_sha, non-PR → $GITHUB_SHA; identical logic to the old container-build step).
  • cicd-container-build: drops its local Get merge commit sha step; checkout ref and the :${{ github.sha }} image tag → :${{ needs.configure.outputs.merge_sha }}.
  • 12 image references (build tag + all consumers) and 17 source checkouts now use merge_sha.
  • No other workflow consumes megatron-bridge:${{ github.sha }}, so nothing downstream breaks.
flowchart TD
    subgraph before["Before — split commit"]
        H1["github.sha (PR head)"] --> M1[generate-test-matrix]
        MC1["merge_commit_sha (head + main)"] --> B1[cicd-container-build]
        M1 -->|"old script names"| T1{{docker exec in container}}
        B1 -->|"new script names"| T1
        T1 --> X1["exit 127: No such file"]
    end
    subgraph after["After — single source"]
        CFG["configure.merge_sha = merge_commit_sha"] --> M2[generate-test-matrix]
        CFG --> B2[cicd-container-build]
        M2 -->|"same names"| T2{{docker exec in container}}
        B2 -->|"same names"| T2
        T2 --> OK2["tests run"]
    end
Loading

Tested

  • python3 -c "import yaml; yaml.safe_load(open('.github/workflows/cicd-main.yml'))" → parses.
  • Verified no ${{ github.sha }} or steps.sha.outputs.main references remain in cicd-main.yml.
  • Verified every job consuming merge_sha has configure in needs (no DAG cycle; configure only needs pre-flight).
  • Full CI run on this PR exercises the end-to-end path.

generate-test-matrix scanned a checkout of github.sha (the PR head branch
tip), but the container image — and thus the launch scripts that actually
run via docker exec — is built from the PR merge_commit_sha (head merged
into main). When main renames or recategorizes launch scripts, the matrix
schedules jobs under stale head-tree names absent from the merge-commit
container, so every renamed script fails exit 127 'No such file'.

Centralize merge_commit_sha resolution in the configure job (new merge_sha
output) and consume it throughout: every source checkout, the image build
tag, and every container-image reference. The matrix is now scanned from
the same tree the container is built from, so a PR behind a launch-script
rename stays green without a rebase.

Signed-off-by: oliver könig <okoenig@nvidia.com>
@ko3n1g
ko3n1g requested a review from a team as a code owner June 24, 2026 15:56
@ko3n1g

ko3n1g commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test 3316e68

@ko3n1g ko3n1g added the ci CI, automation, test queue, or workflow infrastructure work label Jun 24, 2026
@ko3n1g
ko3n1g enabled auto-merge (squash) June 24, 2026 15:57
@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

LGTM — clean, well-scoped fix.

Verified:

  • All 12 ${{ github.sha }} image-tag references replaced with needs.configure.outputs.merge_sha
  • All 17 source checkouts now use ref: ${{ needs.configure.outputs.merge_sha }}
  • Stale steps.sha.outputs.main reference in cicd-container-build removed
  • configure correctly added to needs: for the 9 jobs that previously lacked it (lint-check, unit-tests-core, unit-tests-diffusion, generate-test-matrix, functional-tests-l0, functional-tests-flaky, generate-gb200-test-matrix, gb200-l0, gb200-flaky)
  • Jobs that already depended on configure (L1, L2, gb200-L1, gb200-L2, Nemo_CICD_Test, Coverage) are unchanged — correct
  • get-pr-info step retained in cicd-container-build (still needed for cache-key computation)
  • context.sha in Coverage_Fake is intentionally github.sha — commit statuses must target the PR head, not the merge commit
  • No DAG cycles introduced (configure only needs pre-flight)

Suggested test cases

No perf tests impacted.

@ko3n1g
ko3n1g merged commit 7583593 into main Jun 24, 2026
176 of 178 checks passed
@ko3n1g
ko3n1g deleted the ko3n1g/ci/matrix-use-merge-commit-sha branch June 24, 2026 17:24
liding-nv pushed a commit that referenced this pull request Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci CI, automation, test queue, or workflow infrastructure work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants