build(python): 🎨 dynamically add mpi4py to build-system.requires - #314
Merged
Conversation
robertodr
requested review from
diagonal-hamiltonian,
fpietra and
ludmilaasb
as code owners
August 31, 2026 10:43
github-actions
Bot
added
documentation
|
Docs preview: https://pr-314.monoprop-docs.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
=======================================
Coverage 97.70% 97.70%
=======================================
Files 14 14
Lines 742 742
Branches 98 98
=======================================
Hits 725 725
Misses 12 12
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
robertodr
requested review from
Panadestein
and removed request for
diagonal-hamiltonian and
fpietra
August 31, 2026 11:15
robertodr
added a commit
that referenced
this pull request
Sep 1, 2026
🤖 _AI text below_ 🤖 Adds a Nix flake so Nix/NixOS users can build and hack on monoprop without installing the C++ toolchain, hwloc, Boost or MPI by hand. Stacked on #314 — this branch is rebased onto `chore-mpi4py-build-req-dynamically` and should merge after it. ## Outputs | Command | What it gives you | | --- | --- | | `nix develop` | dev shell: GCC 15, CMake, Ninja, Boost, hwloc, Open MPI, `uv`, `just`, Node.js, clang-tools, gdb, lcov, doxygen | | `nix build .#monoprop` | the Python package, without MPI | | `nix build .#monoprop-mpi` | the same package with `monoprop_ENABLE_MPI=ON` | | `nix run` | a Python interpreter with `monoprop` importable | Inside `nix develop`, the existing `uv sync` and `just` workflows apply unchanged. The shell sets `UV_PYTHON_PREFERENCE=only-system` (uv's managed interpreters expect a loader NixOS does not provide, and `[tool.uv]` pins `only-managed`) and `LD_LIBRARY_PATH` so manylinux wheels resolve `libstdc++`. ## Blocker: the packages do not currently build `nix build .#monoprop` and `.#monoprop-mpi` both fail in `pypaBuildPhase` with `Unmet dependencies`: #243 pinned `nanobind==3.0.0` and added `nanobind-backend>=1.0.0` to `[build-system] requires`, but nixpkgs ships nanobind 2.13.0 and has no `nanobind-backend` at all. This predates the rebase — it arrived in this branch through its last `Merge branch 'main'` and the validation below was never re-run against it. The flake needs the same kind of override it already applies to scikit-build-core before this can merge. The dev shell is unaffected. ## Deviations from the `uv` build All in `nix/monoprop.nix` and documented in the building guide: - C++ unit tests are off — they resolve msgpack-cxx through a CPM git fetch that the build sandbox denies. Build them from the dev shell instead. - `monoprop_ENABLE_ARCH_FLAGS` is off, since a store path may be substituted onto a machine other than the one that built it. `.override { enableArchFlags = true; }` restores `-march=native`. - The version is pinned, because setuptools-scm cannot read git metadata inside the sandbox. **It needs bumping alongside the release tag.** - MPI is enabled by setting the CMake option directly rather than through #314's `monoprop_ENABLE_MPI` environment switch, because that switch also appends mpi4py to `build.requires` — a PyPI resolution the sandbox denies. The store copy of mpi4py is supplied as a build input of `monoprop-mpi` only. ## Notes for reviewers - nixpkgs ships scikit-build-core 1.0.2 while `[build-system] requires` asks for `>=1.0.3`, so the flake overrides it with the 1.0.3 PyPI sdist. That override should be dropped once nixpkgs catches up. - `tools/generate-dispatch.py` reads `.github/license-header.txt` during `cmake --install`, but `.github` is in `sdist.exclude`. The flake works around it by including that one file in the source set; an sdist-based install would presumably hit the same failure. ## Validation On `x86_64-linux`, **before #243 landed**: `nix build .#monoprop` and `.#monoprop-mpi` both compiled and passed the import check, `nix flake check` passed, `nix run` evaluated a `MajoranaOperator`, and `uv sync` inside `nix develop` built the bindings against the Nix toolchain. All of that needs re-running once the nanobind override is in. Darwin and `aarch64-linux` are exposed via `flake-utils.lib.eachDefaultSystem` but untested. --------- Signed-off-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com> Co-authored-by: Roberto Di Remigio Eikås <robertodr@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 AI text below 🤖
Summary
mpi4pysat inbuild-system.requires, so every build installed it — including the non-MPI builds that are the default everywhere, and the cibuildwheel jobs that explicitly pass-Dmonoprop_ENABLE_MPI=OFF. It is only needed for MPI builds, wheresrc/monoprop/bindings/CMakeLists.txtreads its headers to hand a communicator across the nanobind boundary. PEP 508 markers cannot express "only when MPI is on", so the dependency has to become dynamic.It is now injected per-build through PEP 517's
get_requires_for_build_wheel, via scikit-build-core'sbuild.requiressetting.monoprop_ENABLE_MPI=ONin the environment is the single switch: a[[tool.scikit-build.overrides]]block matches it and both addsmpi4pyand sets the CMake option, so the two cannot drift apart. Enabling MPI therefore moves from a config-settings flag to an environment variable in the justfile, CI, README and docs.The config-settings route still works but cannot chain: an override's
if.clause matches environment variables only, so--config-settings=cmake.define.monoprop_ENABLE_MPI=ONmust carry--config-settings=build.requires=mpi4py>=4.1.0alongside it. Getting that wrong used to surface as an opaqueCould NOT find mpi4py (missing: PY_mpi4py); the bindings CMakeLists now fails with a message naming both routes.Changes
pyproject.toml: dropmpi4pyfrombuild-system.requires; add a[[tool.scikit-build.overrides]]block keyed onif.env.monoprop_ENABLE_MPIsettingbuild.requiresandcmake.define.monoprop_ENABLE_MPI. It must stay last in the[tool.scikit-build]section — an array-of-tables swallows any bare key that follows it.src/monoprop/bindings/CMakeLists.txt:find_python_module(mpi4py QUIET)plus an explicitFATAL_ERRORspelling out both ways to enable MPI.justfile(test-mpi,bench-build-mpi) and.github/workflows/qa-analysis.yml(sonarqube-analysis,clang-tidy): switch to the environment switch. MPI moves out ofSKBUILD_CMAKE_DEFINE, which keeps its other defines.docs/content/docs/building.mdx: documents the environment route, the uv build-cache caveat, and a "Driving it with config-settings instead" subsection covering the--config-settings-packageform and its repeated-pin cost.docs/content/docs/testing.mdx,README.md,cpp/tests/README.md,AGENTS.md: updated to the environment switch.Checklist
docs/,CONTRIBUTING.md) if neededCHANGELOG/ release notes updated if applicableAI/LLM disclosure
Important
By opening this PR I confirm that I have read CONTRIBUTING.md and I agree to the terms of the Contributor License Agreement.
Warning
If you're contributing on behalf of your employer, contact cla@algorithmiq.fi to arrange a Corporate CLA.