Skip to content

fix (docker): stop the docker gate from skipping the whole test suite - #1798

Merged
Zhichenzzz merged 2 commits into
mainfrom
zhichen/fix-ci-docker-skip-cascade
Jul 26, 2026
Merged

fix (docker): stop the docker gate from skipping the whole test suite#1798
Zhichenzzz merged 2 commits into
mainfrom
zhichen/fix-ci-docker-skip-cascade

Conversation

@Zhichenzzz

@Zhichenzzz Zhichenzzz commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

The bug

#1791 made resolve-ci-image depend on docker-build, so a docker PR runs its suites inside the freshly built image. docker-build only runs when docker files change, so on every other PR it is skipped — and GitHub propagates a skip down the whole needs closure, including through resolve-ci-image's always() guard. stage-a-cpu has no if: at all, so it was skipped too, and stage-b-* / stage-c-* gate on stage-a-cpu succeeding, so the entire matrix went with it.

The run still reports success, because a skipped job is not a failed one:

run status: completed   conclusion: success
  docker-paths       success
  resolve-ci-policy  success
  resolve-ci-image   success
  docker-build       skipped
  stage-a-cpu        skipped     <- CPU suite
  stage-b-cpu        skipped
  stage-c-2/4/8-gpu  skipped     <- entire GPU matrix

Sub-jobs that actually expanded, per PR's PR Test run:

PR Test run when (UTC) docker-build expanded sub-jobs
#1790 07-24 21:17 absent (pre-#1791) 30
#1794 07-25 18:45 skipped 0
#1793 07-26 06:14 skipped 0
#1792 07-26 06:17 skipped 0

#1791 merged 07-25 01:42 UTC; #1790 was the last run that executed anything.

Two things made this hard to notice. #1791's own CI was green and really ran, because it changed docker/Dockerfile — the regression could not appear in the PR that introduced it. And labels don't reveal it either: resolve-ci-policy parses them fine (labels=[run-ci-megatron run-ci-lora]), but they are consumed by run_suite.py inside the stage jobs, which never start.

The fix

Keep #1791's behavior, restore the old graph. The rule is that no job feeding the stage chain may end up "skipped".

  • docker-paths — always runs. It also loses if: github.event_name == 'pull_request', which had the same effect on the nightly cron: no PR context meant a skipped job and a silently empty scheduled run. It now reports changed=false for non-PR events.
  • docker-build — always reaches a conclusion, under if: always() && !cancelled(). With nothing to build every step is a no-op on ubuntu-latest, off the GPU fleet, and it reports built=false. It deliberately does not require docker-paths to have succeeded: if that job ever fails, the chain must still carry on rather than silently disable the suite again.
  • resolve-ci-image — reads needs.docker-build.outputs.built instead of the job result, and needs only docker-build (it never used docker-paths' output). It keeps no if: guard on purpose: a real build failure should stop the suites rather than run them against a stale image.

Fork PRs keep the pre-existing behavior from #1791 — they cannot push to the registry, so they take the no-op path and test against the released image.

Nothing else changed. Verified by hashing the file in three parts against 775c87378^:

region vs pre-#1791
start → end of resolve-ci-policy identical (e65866e1…)
stage-a-cpu → end of file (all 7 stage jobs) identical (cd1fa924…)
docker block in between #1791's feature + this fix

The diff against pre-#1791 also removes zero lines — every line of the old pipeline is still there.

What a non-docker PR gets

Exactly the pre-#1791 path: same image (radixark/miles:dev), same tag-resolution order, same stage definitions, same fast-fail gating, same label-driven suite selection. The only addition is two short ubuntu-latest jobs (docker-paths + a no-op docker-build) ahead of the chain.

Test plan

This PR is its own test: it touches no docker path, so docker-build takes the no-op branch — the exact case that was broken.

Latest run on this branch: 30 sub-jobs expanded, all green (matching #1790's pre-regression count of 30), with real tests executing —

job executed
stage-a-cpu (4 partitions) 536 + 1384 + 933 + 1117 = 3970 passed
stage-b-cpu 125 passed
stage-b-2-gpu-h200 17 passed across its suites
stage-c-* No tests found … cadence=regular — label-gated, and this PR carries no run-ci-* label

docker-build behaved as intended on the no-op path: ran on ubuntu-latest, every build step skipped, Nothing to build succeeded.

Reviewers: please check expanded sub-jobs, not the run conclusion — the failure mode being fixed is a green run with nothing in it.

gh run view <run-id> --json jobs \
  --jq '[.jobs[]|select(.name|contains("/ run"))]|length'

Not covered here: the changed=true branch never executes on this PR, so the real build path — GPU runner, built=true, suites pulling radixark/miles:pr-<N> — will first be exercised by the next PR that touches a docker file.

#1791 made resolve-ci-image depend on docker-build so that a docker PR runs its
suites inside the freshly built image. docker-build only runs when docker files
change, so on every other PR it is skipped -- and GitHub propagates a skip down
the entire needs closure, including through resolve-ci-image's always() guard.
stage-a-cpu carries no if: at all, so it was skipped, and stage-b/stage-c gate on
stage-a-cpu succeeding, so the whole matrix went with it.

The run still reports success, because a skipped job is not a failed one. Since
#1791 merged, PRs 1792/1793/1794 each completed "green" with zero test jobs
expanded; the last run that actually executed anything was #1790 at 07-24T21:17Z,
before the merge. #1791's own CI was green and did run, because it changed
docker/Dockerfile -- the regression could not appear in the PR that introduced it.

Fix keeps the new behavior and restores the old graph: docker-paths and
docker-build now always reach a conclusion instead of skipping, so nothing
downstream is poisoned. With nothing to build, docker-build no-ops on
ubuntu-latest rather than occupying a GPU runner, and reports built=false;
resolve-ci-image reads that output instead of the job result. All seven stage-*
jobs are byte-identical to their pre-#1791 definitions.

docker-paths also loses its `if: github.event_name == 'pull_request'`, which had
the same effect on the nightly cron: no PR context meant a skipped job and a
silently empty scheduled run. It now reports changed=false for non-PR events.
@Zhichenzzz
Zhichenzzz force-pushed the zhichen/fix-ci-docker-skip-cascade branch from cee1b6f to b20b4b7 Compare July 26, 2026 06:34
@Zhichenzzz Zhichenzzz changed the title ci: stop the docker gate from skipping the whole test suite fix (docker): stop the docker gate from skipping the whole test suite Jul 26, 2026
@Zhichenzzz
Zhichenzzz force-pushed the zhichen/fix-ci-docker-skip-cascade branch from 9a5f286 to 666fd4c Compare July 26, 2026 06:54
The ternary mixed a JSON array with a bare string, and only the string branch is
exercised by a PR that touches no docker file. Make both branches arrays so the
untested branch cannot fail on a type mismatch.
@Zhichenzzz
Zhichenzzz force-pushed the zhichen/fix-ci-docker-skip-cascade branch from 666fd4c to edf9b71 Compare July 26, 2026 07:23
@radixark radixark deleted a comment from gemini-code-assist Bot Jul 26, 2026
@Zhichenzzz
Zhichenzzz merged commit 6d476e1 into main Jul 26, 2026
39 checks passed
@Zhichenzzz
Zhichenzzz deleted the zhichen/fix-ci-docker-skip-cascade branch July 26, 2026 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants