Follow-ups from #284 (plain MPI instead of Boost.MPI)

Open
  • Is something wrong?
#292 0 comments 0 reactions 1 assignee View on GitHub

Maintainers usually reply within 3 days

@skilledwolf is already working on this.

Since Nov 5, 2026.

  • #324 by @skilledwolf — open

Assessment

This issue has not been assessed yet.

Description

bug discussion documentation

Follow-up items noticed while reviewing #284 (Use plain MPI instead of Boost.MPI). None of these block the PR; they are suggestions for later.

1. Generic MPI helpers live in a parapack header

src/alps/parapack/include/alps/parapack/mpi.h adds alps::mpi::send, recv, probe, iprobe, split, broadcast_object and gather, which replace the Boost.MPI calls ALPS used. Nothing in it is specific to parapack: it is plain MPI plus Boost.Serialization for non-arithmetic values.

Code outside parapack now includes it:

  • src/alps/alea/tests/observableset_mpi.C
  • src/alps/legacy_parameters/tests/parameters_mpi.C
  • tests/cli/citation_runtime.cpp
  • tutorials/examples/parapack/multiple/ising.h (a parapack tutorial, so this one is fine)

This builds only because these targets link the monolithic ALPS::alps. It makes lower-level components (alea, legacy_parameters) depend on a header in the top-level parapack component.

Suggestion: move the header to src/alps/utilities/include/alps/utilities/ (e.g. mpi_serialization.hpp), next to <alps/utilities/mpi.hpp>, and update the includes above. Keep it separate from mpi.hpp, which does not depend on Boost.

2. ULFM support removed

The PR removes ALPS_NGS_OPENMPI_ULFM and <alps/ngs/ulfm.hpp>. That is fine: no ALPS code can recover from a failed rank, so nothing could use them. The helpers called OMPI_Comm_shrink and OMPI_Comm_revoke from a pre-release Open MPI. Released MPIs with ULFM (Open MPI 5, MPICH) name them MPIX_Comm_shrink and MPIX_Comm_revoke.

Suggestion: nothing to do now. If fault tolerance is ever wanted, start from the released MPIX_ interface and a scheduler that can recover from a failed rank. That is a research project, not a port of the old helpers.

3. tests/cmake/preserved_public_headers.json is no longer read

The PR adds alps/ngs/boost_mpi.hpp and alps/ngs/ulfm.hpp to its retired list. Nothing reads this file anymore. test_sdk_contracts.py used it as a frozen header baseline until a65015e20 ("Check installed headers against the source tree") replaced that with a check against the source include directories. No test, CMake file or document on master refers to it now.

While the old check was active, the list also kept dead code in the tree. alps/ngs/ulfm.hpp and alps/ngs/boost_mpi.hpp are in its frozen headers list (lines 196 and 252), so deleting either one failed CI unless the JSON was edited too. Leaving an unused header cost nothing; deleting it cost a test failure. ULFM had been unused since 2013 and was still here for #284 to remove. The file also shows nobody checks it now: #284 adds both headers under retired but leaves them in headers, so the file lists them as both kept and removed.

Suggestion: delete the file in a separate PR. If a list of removed headers is still wanted, for example for people installing over an old prefix, keep it in the CHANGELOG instead.

4. alps::mpi::status has an unused default constructor

src/alps/parapack/include/alps/parapack/mpi.h:56 declares status() : status_() {}. Nothing default-constructs an alps::mpi::status: every status comes from iprobe, probe or recv, and the std::optional<status> returned by iprobe doesn't need it.

Suggestion: remove the constructor.

5. The new header is named like MPI's own <mpi.h>

alps/parapack/process.h:27 includes the new header as # include "mpi.h". A quoted include looks in the including file's directory first, so this finds alps/parapack/mpi.h. MPI's header has the same name. It works today: every other mpi.h include in the tree uses <mpi.h> and none is in that directory. Nothing adds include/alps/parapack to an include path either. But a quoted "mpi.h" in a future parapack header, or a downstream project that puts that directory on its include path, would silently get the parapack header instead of MPI's.

Suggestion: give the header a name that can't clash. The move in item 1 (to alps/utilities/mpi_serialization.hpp) does that. If the header stays in parapack, rename it, e.g. alps/parapack/parapack_mpi.h, and include it as <alps/parapack/parapack_mpi.h>.

Dominant language
C++
Stars
32
Forks
10
Avg merge
4d 5h
Merged PRs (30d)
52

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from ALPSim/ALPS

All issues in ALPSim/ALPS

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.