Skip to content

fix(ci): 🐛 install MPI for the bare-metal benchmark - #340

Merged
Panadestein merged 1 commit into
mainfrom
fix/bench-mpi-toolchain
Sep 8, 2026
Merged

Panadestein merged 1 commit into
mainfrom
fix/bench-mpi-toolchain

Conversation

@diagonal-hamiltonian

@diagonal-hamiltonian diagonal-hamiltonian commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

🤖 AI text below 🤖

Summary

The bare-metal benchmark has been red since #323 merged. That PR turned the job into the
three-rung ladder, which builds with monoprop_ENABLE_MPI=ON and launches L2b under
mpiexec -n 4 — but bench.yml sets up its environment with only a cache-suffix, and
.github/actions/setup installs tools/packages/apt-mpi.txt only when it is handed
mpi: on. So the runner had no MPI::MPI_CXX while the build was told to require it, and
the run stopped in Install package:

--   Package 'mpi-cxx' not found
CMake Error at .../FindPackageHandleStandardArgs.cmake:290 (message):
*** CMake configuration failed

Every bare-metal run before #323 was green because the old workflow built without MPI. The
fix is the one input, plus a cache suffix that moves with the configuration: benchmark was
filled by those non-MPI builds, and a monoprop restored from it is the binary every rung
shares — resolve_cores.py would then stop the run one step later with "monoprop was built
without MPI"
.

Nothing downstream of the build had ever run in CI, so this was validated end to end rather
than by making the configure step pass: bench_bare_metal.yml dispatched on this branch,
all three rungs measured, shape-checked and uploaded. Run linked in a comment below.

Changes

  • .github/workflows/bench.yml: pass mpi: "on" to the setup action, and move the
    setup-uv cache suffix to benchmark-mpi.

Checklist

  • Tests added or updated to cover the changes
  • Documentation updated (docstrings, docs/, CONTRIBUTING.md) if needed
  • CHANGELOG / release notes updated if applicable

AI/LLM disclosure

  • I did not use LLM tooling, or used it only privately for ideation
  • I used the following tool to help write this PR description: Claude Code (Opus 5)
  • I used the following tool to generate or modify code: Claude Code (Opus 5)

Summary by CodeRabbit

  • Chores
    • Updated benchmark build setup to ensure MPI support is enabled.
    • Improved benchmark environment caching for MPI-enabled builds.

The ladder introduced in #323 builds with monoprop_ENABLE_MPI=ON and launches L2b under
mpiexec, but the job set up its environment without the `mpi` input, so no MPI runtime
reached the runner and CMake stopped at `Package 'mpi-cxx' not found`.

The cache suffix moves with the configuration: the `benchmark` cache was filled by the
non-MPI builds that preceded the ladder, and a monoprop restored from it would be the one
every rung shares.

Assisted-by: claude-code:claude-opus-5
@coderabbitai

coderabbitai Bot commented Sep 8, 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: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 16ada33a-d058-4364-975f-fccbe942544e

📥 Commits

Reviewing files that changed from the base of the PR and between 4c0252f and 305743c.

📒 Files selected for processing (1)
  • .github/workflows/bench.yml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The benchmark workflow now uses a dedicated benchmark-mpi cache suffix and enables MPI in the setup action. This ensures benchmark rungs restore an MPI-enabled build.

Changes

Benchmark cache setup

Layer / File(s) Summary
Enable MPI in benchmark setup
.github/workflows/bench.yml
The setup action uses the benchmark-mpi cache suffix and sets mpi: "on" for the benchmark build.

Priority: ⬇️ Low — Defer this narrow benchmark CI setup change because it only enables MPI dependencies and isolates the benchmark cache for bare-metal runs.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 30574

The benchmark workflow now installs MPI and uses a separate MPI-specific cache, preventing non-MPI binaries from being reused by MPI benchmark runs. The change is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: enabling MPI installation for the bare-metal benchmark CI workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/bench-mpi-toolchain

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.

❤️ Share

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Docs preview: https://pr-340.monoprop-docs.pages.dev

@diagonal-hamiltonian

Copy link
Copy Markdown
Collaborator Author

🤖 AI text below 🤖

End-to-end validation: run 34228895756, dispatched on this branch, green in 9m21s.

  • Found MPI_CXX: /usr/lib/x86_64-linux-gnu/openmpi/lib/libmpi.so (found version "3.1") — the build configures, which is what was failing.
  • variant default, cores 8 visible physical of 8, BENCH_CORES=8.
  • All three rungs measured and shape-checked: ranks=1 partitions_env=1, ranks=1 partitions_env=8 pinned>=8, ranks=4 partitions_env=2 pinned>=2. No ::error:: emitted.
  • cpu slug: intel-xeon-platinum-8488c, so the testbeds are intel-xeon-platinum-8488c-8c-L1 / -8c-L2a / -8c-L2b, exactly as docs/content/docs/benchmarks.mdx documents them.
  • Three reports uploaded, each carrying all three measures (latency, peak-memory, terms).
  • terms matches the documented figures: 19,902,244 at L1, and 167,515,463 at both L2a and L2b — the geometry-independence check the terms threshold exists to hold.

Peaks landed just under the documented sizing figures (L1 2.45 vs ~2.60 GiB, L2a 18.36 vs ~19.11 GiB, L2b 20.73 vs ~23.71 GiB/node), so the --obs-terms=2500000 ceiling still has headroom on this node.

This run also auto-created a Bencher branch named fix/bench-mpi-toolchain (BENCHER_BRANCH is github.ref_name); worth archiving after merge. The three testbeds are the ones main will reuse, so they should stay.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.70%. Comparing base (4c0252f) to head (305743c).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #340   +/-   ##
=======================================
  Coverage   97.70%   97.70%           
=======================================
  Files          14       14           
  Lines         742      742           
  Branches       98       98           
=======================================
  Hits          725      725           
  Misses         12       12           
  Partials        5        5           
Flag Coverage Δ
cpp 97.70% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@sonarqubecloud

sonarqubecloud Bot commented Sep 8, 2026

Copy link
Copy Markdown

@Panadestein
Panadestein merged commit 79d870d into main Sep 8, 2026
34 checks passed
@Panadestein
Panadestein deleted the fix/bench-mpi-toolchain branch September 8, 2026 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants