Skip to content

ci: cap NVHPC lane concurrency with max-parallel - #1826

Open
sbryngelson wants to merge 4 commits into
masterfrom
ci/nvhpc-max-parallel
Open

ci: cap NVHPC lane concurrency with max-parallel#1826
sbryngelson wants to merge 4 commits into
masterfrom
ci/nvhpc-max-parallel

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Sep 5, 2026

Copy link
Copy Markdown
Member

The 30 NVHPC lanes dispatch all at once and occupy 30 standard-runner slots for roughly two hours, leaving every other workflow queued behind them. Minutes are free on this repo; concurrency is the scarce resource.

The change

strategy.max-parallel is the direct control — a matrix entry queued behind it holds no runner. The catch is that it applies to an entire job matrix, and the NVHPC entries currently share the github job with the ubuntu/macOS lanes, so capping in place would have throttled those too.

So the NVHPC entries move into their own nvhpc job that carries the cap:

  nvhpc:
    name: NVHPC ${{ matrix.nvhpc }} (${{ matrix.target }})
    strategy:
      fail-fast: false
      max-parallel: 6
      matrix:
        nvhpc: ['23.11', '24.1', ... '26.3']
        target: [cpu, gpu]

Peak slot usage for these lanes goes 30 → 6. The 30 hand-written include: entries also collapse to a two-axis product.

What is deliberately unchanged

No compiler coverage is dropped. All 15 releases from 23.11 through 26.3 still build; cpu still runs --test-all; gpu still builds acc then omp plus the case-optimized ICE guard. This trades wall-clock for slots and nothing else.

  • Job names are byte-identical (NVHPC 25.5 (cpu)), so no check-name churn and no branch-protection impact.
  • No job needs: the github job, so the split adds no ordering constraints.
  • I verified all 15 step bodies are byte-identical to master apart from the intended edits: dropping the now-vacuous matrix.nvhpc / !matrix.nvhpc guards, and moving each # ── NVHPC ... ── section comment along with the steps it describes.

Side benefit: this staggers the nvcr.io image pulls that the retry loop in Pull NVHPC container exists to survive — that step's own comment notes "~30 matrix jobs hit it at once".

Tuning

max-parallel: 6 is a starting point, not a derived optimum. Raise it for faster feedback, lower it to free more slots. Timings from a recent master run, if useful for tuning:

Lane Fixed overhead Build Test Total
cpu ~7 min 11.7 min 102.4 min ~120 min
gpu ~5 min 90 min (acc+omp) + 13.8 (case-opt) ~110 min

Not included

Two larger reductions I looked at but left out, since this PR is deliberately scoped to the concurrency cap:

  • The cpu lane spends 102 of its 120 minutes running --test-all. Now included — see "cpu test step" below.
  • The cuda_multi image tag is what forces the Free disk space step; a single-CUDA tag would be far smaller. That depends on MFC's actual CUDA requirements, which I haven't verified.

Also included: --test-all off the cpu test step

--test-all does two unrelated things (toolchain/mfc/test/test.py):

Where What it does Keep?
Build (line 389) adds post_process to the compiled binaries yes — this is the compile coverage
Test (line 735) re-runs pre_process+simulation+post_process a second time per case, plus silo processing no — ~half the lane's wall clock

Only the first is what the matrix exists for. The second duplicates runtime checking the GNU, Intel and self-hosted lanes already do.

So the flag moves out of the shared MFC_NVHPC_TEST_FLAGS and onto the build step alone:

-e "MFC_NVHPC_TEST_FLAGS=--no-mpi"        # was: --test-all --no-mpi
# Build (NVHPC):  mfc.sh test --dry-run ... --test-all $MFC_NVHPC_TEST_FLAGS
# Test  (NVHPC):  mfc.sh test --max-attempts 3 ... $MFC_NVHPC_TEST_FLAGS

post_process still compiles on all 15 releases — no compiler coverage is lost — it just is not re-run here. --no-mpi stays shared, because that one genuinely must match between build and test, which is what the original comment was guarding.

The gpu lanes are untouched: they already pass --test-all explicitly and never referenced the env var.

Expected effect on the cpu lane is roughly halving the ~102 min test step rather than eliminating it, since the flag adds one extra pipeline run per case. I have not measured it — the first run on this branch will show the real number.

https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd

The 30 NVHPC lanes dispatched all at once and occupied 30 standard-runner
slots for roughly two hours, leaving every other workflow queued behind
them. Minutes are free on this repo; concurrency is the scarce resource.

`max-parallel` is the direct control, but it applies to an entire job
matrix, and the NVHPC entries shared the `github` job with the ubuntu and
macOS lanes -- throttling in place would have throttled those too. So the
NVHPC entries move to their own `nvhpc` job that carries the cap, and the
30 hand-written include: entries collapse to a two-axis product.

A matrix entry queued behind max-parallel holds no runner, so this trades
wall-clock for slots without dropping any compiler coverage: all 15
releases from 23.11 through 26.3 still build, cpu still tests, gpu still
builds acc then omp plus the case-optimized guard. It also staggers the
nvcr.io pulls that the retry loop in the pull step exists to survive.

Job names are unchanged ("NVHPC 25.5 (cpu)"), so no check-name churn. No
job depends on `github`, so the split adds no ordering constraints.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
Copilot AI lite review requested due to automatic review settings September 5, 2026 18:13
Comment thread .github/workflows/test.yml Fixed

Copilot AI 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.

🟢 Approval recommended

The workflow refactor appears YAML-correct and preserves intended CI behavior while adding the desired NVHPC concurrency cap (only a minor comment wording tweak suggested).

Pull request overview

This PR restructures the GitHub Actions CI workflow to reduce runner-slot contention by moving the NVHPC matrix into a dedicated job and capping its concurrency via strategy.max-parallel, while keeping compiler coverage intact.

Changes:

  • Split NVHPC lanes out of the existing github job into a new nvhpc job.
  • Apply strategy.max-parallel: 6 to the NVHPC job matrix to cap concurrent NVHPC runners.
  • Simplify NVHPC matrix definition from a long include: list to a 2-axis product (nvhpc × target).
File summaries
File Description
.github/workflows/test.yml Splits NVHPC into a separate job and caps its matrix concurrency to reduce runner-slot usage.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +152 to +156
# Coverage-based test selection ENFORCED on PRs: runs only the tests whose
# recorded coverage overlaps the PR's changed files (conservative ladder in
# coverage.py — non-.fpp changes and .fpp files no test covers fall back to
# run-all). Pushes to master run the full suite (SELECT empty) as a backstop,
# and the nvhpc job still runs --test-all as a pre-merge full check.
CodeQL flagged the `nvhpc` job this PR adds for missing permissions. The
job itself is fine -- it checks out and pulls a container, nothing more --
but this branch is based on master, which has no `permissions:` block, so
every job in test.yml still inherits the repository default.

Add the workflow-level block here rather than relying on #1825 landing
first, so this PR clears the alert on its own in either merge order. The
block is byte-identical to the one #1825 adds to this file, so the two
merge without conflict (verified with git merge-tree).

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
@sbryngelson

Copy link
Copy Markdown
Member Author

Fixed in 4acb93c.

The alert is correct, and it's a merge-order artifact rather than anything wrong with the new job. This branch is based on master, which has no permissions: block in test.yml, so every job there still inherits the repository-default GITHUB_TOKEN — and adding a job makes CodeQL flag the file again. #1825 adds that block, but relying on it landing first would leave a window where merging this PR alone reintroduces the alert.

So this PR now declares it itself:

permissions:
  contents: read
  pull-requests: read

The nvhpc job needs only contents: read (checkout, then a docker pull from nvcr.io that uses no GitHub token). pull-requests: read is there for the file-changes job's dorny/paths-filter, exactly as in #1825.

The block is byte-identical to the one #1825 adds to this same file, so the two branches merge cleanly in either order — I confirmed with git merge-tree against their merge base: zero conflict markers.

One thing I did not do, which is worth a look separately: this grants pull-requests: read workflow-wide, so it reaches the multi-hour NVHPC and self-hosted Frontier/Phoenix jobs that have no use for it. Tighter would be workflow-level contents: read with a job-level override on file-changes alone, since that's the only job running paths-filter. I left it consistent with #1825 rather than diverging the two PRs mid-review — happy to tighten both if you'd prefer.

Mirrors the same change on #1825 so the two branches keep an identical
permissions region in test.yml and continue to merge in either order.

The nvhpc job this PR adds needs only contents: read -- it checks out and
pulls a container image, using no GitHub token -- so it now inherits
exactly that rather than a workflow-wide pull-requests grant it has no use
for. Same for the self-hosted Frontier and Phoenix lanes.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
The cpu lane spent about 102 of its ~120 minutes in `mfc.sh test`, running
the full suite once per NVHPC release, 15 times over.

--test-all does two unrelated things (toolchain/mfc/test/test.py). At build
time it adds post_process to the compiled binaries (line 389). At test time
it re-runs pre_process+simulation+post_process a second time for every case
and post-processes the silo output (line 735). Only the first is the compile
coverage these lanes exist for; the second roughly doubles the runtime and
duplicates checking the GNU, Intel and self-hosted lanes already do.

So the flag moves out of the shared MFC_NVHPC_TEST_FLAGS and onto the build
step alone. post_process is still compiled on all 15 releases -- no loss of
compiler coverage, which is the point of the matrix -- it just is not re-run
here. --no-mpi stays shared, since that one genuinely must match between
build and test.

The gpu lanes are untouched: they already pass --test-all explicitly and
never referenced the env var.

Claude-Session: https://claude.ai/code/session_01XPqfEaUBG7ZaZVzeMWnKHd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants