From 313a23cd59779abf257d3a7d5d3e1259d8267cb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Mon, 26 Oct 2026 16:44:17 +0100 Subject: [PATCH 1/5] =?UTF-8?q?fix(mpi):=20=F0=9F=90=9B=20run=20each=20pro?= =?UTF-8?q?pagator=20on=20a=20private=20duplicate=20of=20its=20communicato?= =?UTF-8?q?r?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit monoprop exchanged point-to-point messages on fixed tags directly on the caller's communicator, so the caller's own messages on those tags could match ours. A propagator built over an MPI communicator now duplicates it once (collectively, during construction), runs all internal traffic on the duplicate, and frees it with the last copy of the propagator; copies and functionals share the duplicate, and the free is skipped after MPI_Finalize. The test suite's communicator fixtures hand out session-scoped duplicates of COMM_SELF and COMM_WORLD instead of the predefined handles. Assisted-by: Pi:claude-opus-5-5 --- cpp/include/monoprop/MonomialPropagator.h | 19 ++- .../MonomialPropagator.inl | 17 ++- cpp/monoprop/detail/mpi/CMakeLists.txt | 1 + cpp/monoprop/detail/mpi/DuplicatedComm.h | 77 ++++++++++ cpp/tests/comm_duplication_tests.cpp | 142 ++++++++++++++++++ docs/content/docs/features/parallelism.mdx | 12 ++ src/monoprop/majorana_propagator.py | 4 +- src/monoprop/monomial_propagator.py | 4 +- src/monoprop/pauli_propagator.py | 4 +- tests/conftest.py | 49 +++++- tests/mpi_support.py | 5 - tests/test_evolved_operator_coefficients.py | 5 +- tests/test_monoprop_mpi.py | 46 ++++++ 13 files changed, 358 insertions(+), 27 deletions(-) create mode 100644 cpp/monoprop/detail/mpi/DuplicatedComm.h create mode 100644 cpp/tests/comm_duplication_tests.cpp diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index ccd13c566..fbf0a7fbb 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -42,6 +43,7 @@ #include "monoprop/algebra/PauliAlgebra.h" #include "monoprop/core/Monomial.h" #include "monoprop/detail/evolution/CosineRecompute.h" +#include "monoprop/detail/mpi/DuplicatedComm.h" #include "monoprop/detail/mpi/MPICompat.h" #include "monoprop/detail/mpi/MPIUtils.h" #include "monoprop/detail/operator/MPOperator.h" @@ -74,6 +76,13 @@ class MonomialPropagator { public: using PartitionChildFactory = std::function>(mpi::Comm)>; + /// Builds the propagator over `comm`. + /// + /// When `comm` is an MPI communicator, construction is collective over it: every rank must call with + /// the same arguments, in the same order relative to its other collectives on `comm`. It duplicates + /// `comm` once and runs all internal traffic on the duplicate, so monoprop's messages cannot match the + /// caller's own on `comm`. The duplicate is freed when the propagator is destroyed, or once the last + /// copy of it is. MonomialPropagator(const OperatorDict &initial_operator, unsigned int cutoff, const VecZ &initial_state, @@ -93,6 +102,8 @@ class MonomialPropagator { /// Deep copy: clones the operator store, shares the immutable graph cores, and clones the whole /// partition group on a facade. The virtual destructor suppresses implicit moves, so a "move" deep-copies. + /// The copy shares this propagator's communicator duplicate instead of making another, so copying stays + /// non-collective on a single-partition propagator. MonomialPropagator(const MonomialPropagator &other); auto operator=(const MonomialPropagator &) -> MonomialPropagator & = delete; @@ -235,7 +246,8 @@ class MonomialPropagator { auto basis_change() const -> std::optional> { return basis_change_; } - /// MPI_COMM_SELF on a partition, which trades over an in-process comm instead. + /// The private duplicate of the communicator passed at construction: MPI_CONGRUENT with it, never + /// MPI_IDENT. MPI_COMM_SELF on a partition, which trades over an in-process comm instead. auto comm() const -> MPI_Comm { return comm_.mpi; } /// Build the propagation graph, one layer per generator, recording each layer's gate info @@ -324,7 +336,10 @@ class MonomialPropagator { auto apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD>; bool schrodinger_; - mpi::Comm comm_; // real MPI across nodes, or an in-process comm across partitions + // Keeps the duplicate behind comm_ alive; declared first so it is freed last, after partition_group_ and + // every other user of comm_. Null for a partition (in-process comm) and in builds without MPI. + std::shared_ptr comm_owner_; + mpi::Comm comm_; // the duplicate across nodes, or an in-process comm across partitions CutoffFn cutoff_fn_; detail::MPOperator mp_op_; MPGraph graph_; diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index f9a09fe15..1b955ba4d 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -89,7 +89,8 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope size_t partitions, PartitionChildFactory child_factory) : schrodinger_{schrodinger_cutoff.has_value()}, - comm_{comm}, + comm_owner_{mpi::duplicate_user_comm(comm)}, + comm_{comm_owner_ ? mpi::Comm{comm_owner_->handle()} : comm}, mp_op_{}, graph_(schrodinger_cutoff.has_value()), cutoff_{cutoff}, @@ -114,11 +115,11 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope lower_atol.value())); } - const size_t n_partitions = resolve_partition_count_(partitions, comm); + const size_t n_partitions = resolve_partition_count_(partitions, comm_); // The R ranks x S partitions form one flat P = R*S SPMD world, so a mismatch across ranks would // deadlock at the first hybrid collective. - if (comm.kind == mpi::Comm::Kind::Mpi && mpi::size(comm) > 1 - && mpi::allreduce_sum(n_partitions, comm) != n_partitions * static_cast(mpi::size(comm))) { + if (comm_.kind == mpi::Comm::Kind::Mpi && mpi::size(comm_) > 1 + && mpi::allreduce_sum(n_partitions, comm_) != n_partitions * static_cast(mpi::size(comm_))) { throw PartitionCountMismatch( "Partition count differs across MPI ranks — every rank must resolve the same " "partitions= / monoprop_PARTITIONS / monoprop_NUM_THREADS so R*S is a consistent world."); @@ -134,7 +135,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope const size_t global_terms = paired_op_size(max_pairs, logical_num_modes_); // paired_op_size saturates rather than wrapping, so this is "too large to count", not a size. const bool uncountable = global_terms == std::numeric_limits::max(); - const size_t world_slots = n_partitions * static_cast(mpi::size(comm)); + const size_t world_slots = n_partitions * static_cast(mpi::size(comm_)); const size_t share = global_terms / std::max(1, world_slots); if (uncountable || share >= detail::OperatorIndex::kIndexCeiling) { const auto how_many = uncountable ? std::string("more than 2^64") : std::format("{}", global_terms); @@ -170,7 +171,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope }}; partition_group_ = std::make_unique>(static_cast(n_partitions), factory, - comm); + comm_); return; } @@ -240,6 +241,7 @@ MonomialPropagator::~MonomialPropagator() = default; template MonomialPropagator::MonomialPropagator(const MonomialPropagator &other) : schrodinger_(other.schrodinger_), + comm_owner_(other.comm_owner_), comm_(other.comm_), cutoff_fn_(other.cutoff_fn_), mp_op_(other.mp_op_), @@ -1082,7 +1084,8 @@ auto MonomialPropagator::make_functional_(Fn &&func, std::optional R { + comm, + comm_owner = comm_owner_](const VecD ¶ms) -> R { // owner: `comm` must stay valid whenever called validate_expected_initial_operator(*epoch, expected_epoch); validate_functional_call(params, num_params); validate_expected_graph_layers(graph->layers(), expected_layers); diff --git a/cpp/monoprop/detail/mpi/CMakeLists.txt b/cpp/monoprop/detail/mpi/CMakeLists.txt index 43193c85c..a2625b5d0 100644 --- a/cpp/monoprop/detail/mpi/CMakeLists.txt +++ b/cpp/monoprop/detail/mpi/CMakeLists.txt @@ -7,6 +7,7 @@ target_sources( "CheckedCount.h" "Comm.h" "CpuRelax.h" + "DuplicatedComm.h" "Exchange.h" "HybridComm.h" "MPICompat.h" diff --git a/cpp/monoprop/detail/mpi/DuplicatedComm.h b/cpp/monoprop/detail/mpi/DuplicatedComm.h new file mode 100644 index 000000000..b53730bf8 --- /dev/null +++ b/cpp/monoprop/detail/mpi/DuplicatedComm.h @@ -0,0 +1,77 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#pragma once + +#include +#include + +#include "monoprop/detail/mpi/Comm.h" + +namespace monoprop::mpi { + +/// Owns a private duplicate of a caller's communicator and frees it on destruction. +/// +/// monoprop talks point-to-point on fixed tags (see Pairwise.h), so running on the caller's own +/// communicator would let its messages and ours match each other. Immovable: the handle is freed exactly +/// once, by whoever holds the last `std::shared_ptr` (see duplicate_user_comm()). +class DuplicatedComm { +public: + /// Collective over `parent`: every rank of the communicator must construct one, in the same order. + explicit DuplicatedComm(MPI_Comm parent) : handle_(parent) { +#ifdef monoprop_ENABLE_MPI + if (MPI_Comm_dup(parent, &handle_) != MPI_SUCCESS) { + throw std::runtime_error("MPI_Comm_dup failed"); + } +#endif + } + + DuplicatedComm(const DuplicatedComm &) = delete; + auto operator=(const DuplicatedComm &) -> DuplicatedComm & = delete; + + // Python objects can outlive MPI_Finalize (mpi4py finalizes at interpreter exit), and freeing a + // communicator afterwards is an error. + ~DuplicatedComm() { +#ifdef monoprop_ENABLE_MPI + int finalized = 0; + MPI_Finalized(&finalized); + if (finalized == 0) { + MPI_Comm_free(&handle_); + } +#endif + } + + /// The duplicate; valid until the last owner is destroyed. + [[nodiscard]] auto handle() const -> MPI_Comm { return handle_; } + +private: + MPI_Comm handle_; +}; + +/// Duplicates `comm` when it is a caller-supplied MPI communicator, collectively over it. +/// +/// Returns null when there is nothing to isolate: an in-process (Shm / Hybrid) handle, which is already +/// owned by its PartitionGroup, MPI_COMM_NULL, and every build without MPI. +[[nodiscard]] inline auto duplicate_user_comm(const Comm &comm) -> std::shared_ptr { +#ifdef monoprop_ENABLE_MPI + if (comm.kind == Comm::Kind::Mpi && comm.mpi != MPI_COMM_NULL) { + return std::make_shared(comm.mpi); + } +#else + (void)comm; +#endif + return nullptr; +} + +} // namespace monoprop::mpi diff --git a/cpp/tests/comm_duplication_tests.cpp b/cpp/tests/comm_duplication_tests.cpp new file mode 100644 index 000000000..14bc98e5e --- /dev/null +++ b/cpp/tests/comm_duplication_tests.cpp @@ -0,0 +1,142 @@ +// Copyright 2026 Algorithmiq +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// A propagator talks on a private duplicate of the communicator it is given, so its fixed-tag +// point-to-point traffic cannot match the caller's own messages on that communicator. + +#include + +#ifdef monoprop_ENABLE_MPI + +#include +#include +#include +#include + +#include "TestUtilities.h" +#include "monoprop/MonomialPropagator.h" +#include "monoprop/detail/mpi/MPICompat.h" + +namespace { + +using namespace monoprop; +using namespace test_utils; + +constexpr size_t kNumModes = 8; + +auto compare_comms(MPI_Comm lhs, MPI_Comm rhs) -> int { + int result = MPI_UNEQUAL; + MPI_Comm_compare(lhs, rhs, &result); + return result; +} + +BOOST_AUTO_TEST_CASE(propagator_runs_on_a_private_duplicate_of_its_communicator) { + const auto data = load_case_data("random_exact.msgpack"); + for (const MPI_Comm user_comm : {MPI_COMM_SELF, MPI_COMM_WORLD}) { + auto sim = build_simulator(data, SimulatorConfig{.comm = user_comm}); + BOOST_TEST(sim.comm() != user_comm); + BOOST_TEST(compare_comms(sim.comm(), user_comm) == MPI_CONGRUENT); + } +} + +// Counts the communicators an attribute was deleted from: MPI runs the delete callback when a +// communicator carrying it is freed, which is the only way to observe a free from outside. +int g_deleted_attributes = 0; + +auto copy_attribute(MPI_Comm, int, void *, void *value_in, void *value_out, int *flag) -> int { + *static_cast(value_out) = value_in; + *flag = 1; + return MPI_SUCCESS; +} + +auto count_deleted_attribute(MPI_Comm, int, void *, void *) -> int { + ++g_deleted_attributes; + return MPI_SUCCESS; +} + +BOOST_AUTO_TEST_CASE(duplicate_is_shared_by_copies_and_freed_with_the_last_owner) { + const auto data = load_case_data("random_exact.msgpack"); + MPI_Comm user_comm = MPI_COMM_NULL; + MPI_Comm_dup(MPI_COMM_WORLD, &user_comm); + int keyval = MPI_KEYVAL_INVALID; + MPI_Comm_create_keyval(copy_attribute, count_deleted_attribute, &keyval, nullptr); + MPI_Comm_set_attr(user_comm, keyval, nullptr); + g_deleted_attributes = 0; + + auto original = std::make_unique>( + build_simulator(data, SimulatorConfig{.comm = user_comm})); + auto copy = std::make_unique>(*original); + BOOST_TEST(copy->comm() == original->comm()); + + const MPI_Comm duplicate = copy->comm(); + original.reset(); + BOOST_TEST(g_deleted_attributes == 0); // the copy still owns it + BOOST_TEST(MPI_Barrier(duplicate) == MPI_SUCCESS); + + copy.reset(); + BOOST_TEST(g_deleted_attributes == 1); // the duplicate, and only it, was freed + + MPI_Comm_free(&user_comm); + BOOST_TEST(g_deleted_attributes == 2); // the caller's own communicator is still theirs to free + MPI_Comm_free_keyval(&keyval); +} + +// The caller's message sits unmatched on the very communicator monoprop is handed, one per internal tag. +// Without the duplicate, monoprop's own receives (same source, same tag) would consume it. +BOOST_AUTO_TEST_CASE(user_messages_on_internal_tags_survive_propagation) { + const int rank = mpi::rank(MPI_COMM_WORLD); + const int size = mpi::size(MPI_COMM_WORLD); + if (size < 2) { + BOOST_TEST_MESSAGE("Skipping user_messages_on_internal_tags_survive_propagation: world size = 1."); + return; + } + const int partner = rank ^ 1; + const bool has_partner = partner < size; + constexpr std::array tags{mpi::kHybridCountTag, + mpi::kHybridPayloadTag, + mpi::kFlatPayloadTag, + mpi::kFlatCountTag, + mpi::kFlatReplayTag}; + + std::array outgoing{}; + std::vector sends; + for (size_t i = 0; i < tags.size() && has_partner; ++i) { + outgoing[i] = (1000 * rank) + tags[i]; + sends.emplace_back(); + MPI_Isend(&outgoing[i], 1, MPI_INT, partner, tags[i], MPI_COMM_WORLD, &sends.back()); + } + + const auto data = load_case_data("random_exact.msgpack"); + auto sim = build_simulator(data, SimulatorConfig{.comm = MPI_COMM_WORLD}); + BOOST_CHECK_SMALL(evaluate_expval(sim, data, false) - data.actual_expval, 1e-9); + + // Polled against a deadline so a consumed message fails the test instead of hanging it. + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); + for (size_t i = 0; i < tags.size() && has_partner; ++i) { + int incoming = -1; + MPI_Request receive = MPI_REQUEST_NULL; + MPI_Irecv(&incoming, 1, MPI_INT, partner, tags[i], MPI_COMM_WORLD, &receive); + int done = 0; + while (done == 0 && std::chrono::steady_clock::now() < deadline) { + MPI_Test(&receive, &done, MPI_STATUS_IGNORE); + } + BOOST_REQUIRE_MESSAGE(done != 0, "the user's message with tag " << tags[i] << " was consumed"); + BOOST_TEST(incoming == (1000 * partner) + tags[i]); + } + MPI_Waitall(static_cast(sends.size()), sends.data(), MPI_STATUSES_IGNORE); +} + +} // namespace + +#endif // monoprop_ENABLE_MPI diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 836a066ff..3f1af0f3a 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -122,6 +122,18 @@ built for raises [MPIAbiMismatchError][monoprop.exceptions.MPIAbiMismatchError], library older than the supported versions (see [Enabling MPI](#enabling-mpi)) raises [MPIVersionError][monoprop.exceptions.MPIVersionError]. +### Communicator ownership + +monoprop never trades messages on the communicator you pass. Building a propagator is collective over +`comm`: every rank must construct it, in the same order relative to its other collectives on `comm`. It +duplicates `comm` once, runs all of its traffic on the duplicate, and frees the duplicate when the +propagator is destroyed. monoprop's point-to-point messages therefore cannot match yours, even on a +shared tag, and you keep ownership of `comm` itself. + +A deep copy of a propagator shares the original's duplicate instead of making another, so +copying stays non-collective. The duplicate is released when the last copy goes. A propagator that +outlives `MPI_Finalize`, such as one still referenced at interpreter exit, skips the free. + ### Single-node (`MPI.COMM_SELF`) ```python notest diff --git a/src/monoprop/majorana_propagator.py b/src/monoprop/majorana_propagator.py index 4ad371e53..fa432dfa6 100644 --- a/src/monoprop/majorana_propagator.py +++ b/src/monoprop/majorana_propagator.py @@ -111,7 +111,9 @@ def __init__( exception applies on top of either. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of complexity. - comm: Optional MPI communicator (must outlive the simulator). + comm: Optional MPI communicator. Construction is collective over it. The simulator + runs on a private duplicate, so its messages never mix with the caller's on + ``comm``; the duplicate is freed with the simulator. """ majorana_operator: MajoranaOperator = ( initial_operator diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index ca3a711cc..131d96dc2 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -104,7 +104,9 @@ def _init_simulator( lower_atol: Coefficient-truncation threshold, or ``None``. upper_atol: Coefficient-retention threshold, or ``None``. basis_change: Internal per-Majorana basis change (``2 * num_modes`` entries). - comm: Optional MPI communicator (must outlive the propagator). + comm: Optional MPI communicator. Construction is collective over it. The propagator + runs on a private duplicate, so its messages never mix with the caller's on + ``comm``; the duplicate is freed with the propagator. basis: Engine basis -- ``"majorana"`` (default) or ``"pauli"``. """ num_modes = majorana_operator.num_modes diff --git a/src/monoprop/pauli_propagator.py b/src/monoprop/pauli_propagator.py index ec8719262..8f798df69 100644 --- a/src/monoprop/pauli_propagator.py +++ b/src/monoprop/pauli_propagator.py @@ -70,7 +70,9 @@ def __init__( so a value approaching ``num_qubits`` is rejected with a ``RuntimeError`` rather than started. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of weight. - comm: Optional MPI communicator (must outlive the propagator). + comm: Optional MPI communicator. Construction is collective over it. The propagator + runs on a private duplicate, so its messages never mix with the caller's on + ``comm``; the duplicate is freed with the propagator. """ # The engine takes the Schrodinger cutoff in slots (two per qubit); this API in qubits. schrodinger_cutoff = ( diff --git a/tests/conftest.py b/tests/conftest.py index c6750a97e..20bc0be0b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,6 +20,12 @@ allreduce internally. ``serial_comm`` is always COMM_SELF: use it for rank-local state such as ``evolved_operator``, ``contract_partially``, ``size()``, or ``graph_size()``. +Both hand out session-wide duplicates of those communicators, never the predefined handles +themselves, so the suite's traffic stays off the predefined communicators that conftest and +mpi4py use. +Each duplicate is created on first use and freed at session teardown: ``Dup`` is collective, and +doing it per test would add a collective to every one of them for no extra isolation. + Under more than one rank each rank gets its own ``--basetemp``; see ``_rank_basetemp``. """ @@ -33,10 +39,12 @@ import pytest -from tests.mpi_support import HAS_MPI_BACKEND, MPI, SERIAL_COMM +from tests.mpi_support import HAS_MPI_BACKEND, MPI from tests.reference_data import load_problem if TYPE_CHECKING: + from collections.abc import Iterator + from tests.reference_data import FermionicProblem @@ -124,26 +132,53 @@ def pytest_runtest_setup(item: pytest.Item) -> None: pytest.skip(f"Test requires {min_size} MPI processes") +@pytest.fixture(scope="session") +def comm_self_duplicate() -> Iterator[Any]: + """Session-wide duplicate of COMM_SELF; ``None`` when monoprop has no MPI backend.""" + if not HAS_MPI_BACKEND: + yield None + return + duplicate = MPI.COMM_SELF.Dup() + yield duplicate + duplicate.Free() + + +@pytest.fixture(scope="session") +def comm_world_duplicate() -> Iterator[Any]: + """Session-wide duplicate of COMM_WORLD; ``None`` when monoprop has no MPI backend.""" + if not HAS_MPI_BACKEND: + yield None + return + duplicate = MPI.COMM_WORLD.Dup() + yield duplicate + duplicate.Free() + + _COMM_PARAMS = ( [pytest.param(None, id="comm_none")] if not HAS_MPI_BACKEND else [ - pytest.param(MPI.COMM_SELF, id="comm_self"), - pytest.param(MPI.COMM_WORLD, id="comm_world"), + pytest.param("comm_self_duplicate", id="comm_self"), + pytest.param("comm_world_duplicate", id="comm_world"), ] ) @pytest.fixture(params=_COMM_PARAMS) def comm(request: pytest.FixtureRequest) -> Any: # noqa: ANN401 - """Parametrized communicator; ``None`` when mpi4py is unavailable or monoprop has no MPI backend.""" - return request.param + """Parametrized communicator; ``None`` when mpi4py is unavailable or monoprop has no MPI backend. + + Resolved through the session-scoped duplicates, so parametrizing does not reorder the run. + """ + if request.param is None: + return None + return request.getfixturevalue(request.param) @pytest.fixture -def serial_comm() -> Any: # noqa: ANN401 +def serial_comm(comm_self_duplicate: Any) -> Any: # noqa: ANN401 """Single-rank communicator for tests that inspect rank-local operator state.""" - return SERIAL_COMM + return comm_self_duplicate @pytest.fixture( diff --git a/tests/mpi_support.py b/tests/mpi_support.py index be9e3217a..79a9d3222 100644 --- a/tests/mpi_support.py +++ b/tests/mpi_support.py @@ -16,8 +16,6 @@ from __future__ import annotations -from typing import Any - import monoprop try: @@ -27,6 +25,3 @@ HAS_MPI_BACKEND: bool = MPI is not None and bool(monoprop.MPI_ABIS) """True when mpi4py is importable and this monoprop installation ships an MPI backend.""" - -SERIAL_COMM: Any = MPI.COMM_SELF if HAS_MPI_BACKEND else None -"""Single-rank communicator for rank-local checks; ``None`` selects the serial backend.""" diff --git a/tests/test_evolved_operator_coefficients.py b/tests/test_evolved_operator_coefficients.py index a2419a9cb..0152de41c 100644 --- a/tests/test_evolved_operator_coefficients.py +++ b/tests/test_evolved_operator_coefficients.py @@ -33,7 +33,6 @@ ) from monoprop.majorana import Majorana, MajoranaOperator from monoprop.pauli import Pauli, PauliOperator -from tests.mpi_support import SERIAL_COMM from tests.reference_data import load_problem DATA = Path(__file__).parent / "data" @@ -48,7 +47,7 @@ def problem(): @pytest.fixture(scope="module") -def prop_and_params(problem): +def prop_and_params(problem, comm_self_duplicate): """A built propagator over ``problem`` and its parameters, shared across the module. ``evolved_operator`` and ``evolved_operator_coefficients`` leave simulator state alone, so one @@ -58,7 +57,7 @@ def prop_and_params(problem): problem.operator, problem.monomial_circuit.initial_state, cutoff=2 * problem.n_modes, - comm=SERIAL_COMM, + comm=comm_self_duplicate, ) prop.build_graph(problem.monomial_circuit.to_circuit()) return prop, problem.monomial_circuit.parameters diff --git a/tests/test_monoprop_mpi.py b/tests/test_monoprop_mpi.py index ad02d6d0a..54f584915 100644 --- a/tests/test_monoprop_mpi.py +++ b/tests/test_monoprop_mpi.py @@ -19,6 +19,8 @@ from __future__ import annotations +import time + import numpy as np import pytest @@ -47,6 +49,11 @@ def _make_mp(problem, comm, *, schrodinger=False): return mp, circuit +# The point-to-point tags monoprop's exchanges use (cpp/monoprop/detail/mpi/Pairwise.h). +_INTERNAL_TAGS = tuple(range(0x6D70, 0x6D75)) +_RECEIVE_TIMEOUT_SECONDS = 30 + + def _assert_expval(expval, expected, atol=1e-9): assert np.isclose(expval, expected, atol=atol), ( f"Expectation value mismatch: {expval} vs {expected}" @@ -191,3 +198,42 @@ def test_mpi_comm_self(self, lih_fermionic_spin_exact): lih_fermionic_spin_exact.monomial_circuit.parameters ) _assert_expval(expval, lih_fermionic_spin_exact.exact_expval) + + @pytest.mark.mpi(min_size=2) + def test_user_messages_on_internal_tags_survive_propagation( + self, lih_fermionic_spin_exact + ): + world = MPI.COMM_WORLD + partner = world.Get_rank() ^ 1 + has_partner = partner < world.Get_size() + outgoing = { + tag: np.array([1000 * world.Get_rank() + tag], dtype=np.int64) + for tag in _INTERNAL_TAGS + } + sends = ( + [ + world.Isend([payload, MPI.INT64_T], dest=partner, tag=tag) + for tag, payload in outgoing.items() + ] + if has_partner + else [] + ) + + mp, circuit = _make_mp(lih_fermionic_spin_exact, world) + mp.build_graph(circuit) + expval = mp.expval_functional(pare_threshold=1e-10)( + lih_fermionic_spin_exact.monomial_circuit.parameters + ) + _assert_expval(expval, lih_fermionic_spin_exact.exact_expval) + + # Polled against a deadline so a consumed message fails the test instead of hanging it. + deadline = time.monotonic() + _RECEIVE_TIMEOUT_SECONDS + for tag in _INTERNAL_TAGS if has_partner else (): + incoming = np.zeros(1, dtype=np.int64) + receive = world.Irecv([incoming, MPI.INT64_T], source=partner, tag=tag) + arrived = receive.Test() + while not arrived and time.monotonic() < deadline: + arrived = receive.Test() + assert arrived, f"the user's message with tag {tag} was consumed" + assert incoming[0] == 1000 * partner + tag + MPI.Request.Waitall(sends) From f765ee2a80d8172b0a5ac49150f5645ea268cd83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Tue, 27 Oct 2026 21:14:04 +0100 Subject: [PATCH 2/5] =?UTF-8?q?refactor(mpi):=20=E2=99=BB=EF=B8=8F=20drop?= =?UTF-8?q?=20library-side=20communicator=20duplication;=20document=20pass?= =?UTF-8?q?ing=20a=20duplicate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Duplicating the caller's communicator inside the library needed an owner shared by copies and functionals, and a guard against freeing after MPI_Finalize. That is too much machinery for a narrow hazard: monoprop's fixed-tag point-to-point exchanges complete within each call, so they can only match a caller's messages that are in flight on the same communicator. Document instead that callers should pass a duplicate, as the test fixtures do. Rename the session-scoped fixtures to comm_self/comm_world and type them as Intracomm | None. Assisted-by: Pi:claude-opus-5-5 --- cpp/include/monoprop/MonomialPropagator.h | 19 +-- .../MonomialPropagator.inl | 17 +-- cpp/monoprop/detail/mpi/CMakeLists.txt | 1 - cpp/monoprop/detail/mpi/DuplicatedComm.h | 77 ---------- cpp/tests/comm_duplication_tests.cpp | 142 ------------------ docs/content/docs/features/parallelism.mdx | 23 ++- src/monoprop/majorana_propagator.py | 6 +- src/monoprop/monomial_propagator.py | 6 +- src/monoprop/pauli_propagator.py | 6 +- tests/conftest.py | 22 +-- tests/test_evolved_operator_coefficients.py | 4 +- tests/test_monoprop_mpi.py | 46 ------ 12 files changed, 41 insertions(+), 328 deletions(-) delete mode 100644 cpp/monoprop/detail/mpi/DuplicatedComm.h delete mode 100644 cpp/tests/comm_duplication_tests.cpp diff --git a/cpp/include/monoprop/MonomialPropagator.h b/cpp/include/monoprop/MonomialPropagator.h index fbf0a7fbb..ccd13c566 100644 --- a/cpp/include/monoprop/MonomialPropagator.h +++ b/cpp/include/monoprop/MonomialPropagator.h @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include @@ -43,7 +42,6 @@ #include "monoprop/algebra/PauliAlgebra.h" #include "monoprop/core/Monomial.h" #include "monoprop/detail/evolution/CosineRecompute.h" -#include "monoprop/detail/mpi/DuplicatedComm.h" #include "monoprop/detail/mpi/MPICompat.h" #include "monoprop/detail/mpi/MPIUtils.h" #include "monoprop/detail/operator/MPOperator.h" @@ -76,13 +74,6 @@ class MonomialPropagator { public: using PartitionChildFactory = std::function>(mpi::Comm)>; - /// Builds the propagator over `comm`. - /// - /// When `comm` is an MPI communicator, construction is collective over it: every rank must call with - /// the same arguments, in the same order relative to its other collectives on `comm`. It duplicates - /// `comm` once and runs all internal traffic on the duplicate, so monoprop's messages cannot match the - /// caller's own on `comm`. The duplicate is freed when the propagator is destroyed, or once the last - /// copy of it is. MonomialPropagator(const OperatorDict &initial_operator, unsigned int cutoff, const VecZ &initial_state, @@ -102,8 +93,6 @@ class MonomialPropagator { /// Deep copy: clones the operator store, shares the immutable graph cores, and clones the whole /// partition group on a facade. The virtual destructor suppresses implicit moves, so a "move" deep-copies. - /// The copy shares this propagator's communicator duplicate instead of making another, so copying stays - /// non-collective on a single-partition propagator. MonomialPropagator(const MonomialPropagator &other); auto operator=(const MonomialPropagator &) -> MonomialPropagator & = delete; @@ -246,8 +235,7 @@ class MonomialPropagator { auto basis_change() const -> std::optional> { return basis_change_; } - /// The private duplicate of the communicator passed at construction: MPI_CONGRUENT with it, never - /// MPI_IDENT. MPI_COMM_SELF on a partition, which trades over an in-process comm instead. + /// MPI_COMM_SELF on a partition, which trades over an in-process comm instead. auto comm() const -> MPI_Comm { return comm_.mpi; } /// Build the propagation graph, one layer per generator, recording each layer's gate info @@ -336,10 +324,7 @@ class MonomialPropagator { auto apply_initial_operator_(const OperatorDict &op_dict) -> std::pair, VecD>; bool schrodinger_; - // Keeps the duplicate behind comm_ alive; declared first so it is freed last, after partition_group_ and - // every other user of comm_. Null for a partition (in-process comm) and in builds without MPI. - std::shared_ptr comm_owner_; - mpi::Comm comm_; // the duplicate across nodes, or an in-process comm across partitions + mpi::Comm comm_; // real MPI across nodes, or an in-process comm across partitions CutoffFn cutoff_fn_; detail::MPOperator mp_op_; MPGraph graph_; diff --git a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl index 1b955ba4d..f9a09fe15 100644 --- a/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl +++ b/cpp/monoprop/detail/monomial_propagator/MonomialPropagator.inl @@ -89,8 +89,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope size_t partitions, PartitionChildFactory child_factory) : schrodinger_{schrodinger_cutoff.has_value()}, - comm_owner_{mpi::duplicate_user_comm(comm)}, - comm_{comm_owner_ ? mpi::Comm{comm_owner_->handle()} : comm}, + comm_{comm}, mp_op_{}, graph_(schrodinger_cutoff.has_value()), cutoff_{cutoff}, @@ -115,11 +114,11 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope lower_atol.value())); } - const size_t n_partitions = resolve_partition_count_(partitions, comm_); + const size_t n_partitions = resolve_partition_count_(partitions, comm); // The R ranks x S partitions form one flat P = R*S SPMD world, so a mismatch across ranks would // deadlock at the first hybrid collective. - if (comm_.kind == mpi::Comm::Kind::Mpi && mpi::size(comm_) > 1 - && mpi::allreduce_sum(n_partitions, comm_) != n_partitions * static_cast(mpi::size(comm_))) { + if (comm.kind == mpi::Comm::Kind::Mpi && mpi::size(comm) > 1 + && mpi::allreduce_sum(n_partitions, comm) != n_partitions * static_cast(mpi::size(comm))) { throw PartitionCountMismatch( "Partition count differs across MPI ranks — every rank must resolve the same " "partitions= / monoprop_PARTITIONS / monoprop_NUM_THREADS so R*S is a consistent world."); @@ -135,7 +134,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope const size_t global_terms = paired_op_size(max_pairs, logical_num_modes_); // paired_op_size saturates rather than wrapping, so this is "too large to count", not a size. const bool uncountable = global_terms == std::numeric_limits::max(); - const size_t world_slots = n_partitions * static_cast(mpi::size(comm_)); + const size_t world_slots = n_partitions * static_cast(mpi::size(comm)); const size_t share = global_terms / std::max(1, world_slots); if (uncountable || share >= detail::OperatorIndex::kIndexCeiling) { const auto how_many = uncountable ? std::string("more than 2^64") : std::format("{}", global_terms); @@ -171,7 +170,7 @@ MonomialPropagator::MonomialPropagator(const OperatorDict &initial_ope }}; partition_group_ = std::make_unique>(static_cast(n_partitions), factory, - comm_); + comm); return; } @@ -241,7 +240,6 @@ MonomialPropagator::~MonomialPropagator() = default; template MonomialPropagator::MonomialPropagator(const MonomialPropagator &other) : schrodinger_(other.schrodinger_), - comm_owner_(other.comm_owner_), comm_(other.comm_), cutoff_fn_(other.cutoff_fn_), mp_op_(other.mp_op_), @@ -1084,8 +1082,7 @@ auto MonomialPropagator::make_functional_(Fn &&func, std::optional R { // owner: `comm` must stay valid whenever called + comm](const VecD ¶ms) -> R { validate_expected_initial_operator(*epoch, expected_epoch); validate_functional_call(params, num_params); validate_expected_graph_layers(graph->layers(), expected_layers); diff --git a/cpp/monoprop/detail/mpi/CMakeLists.txt b/cpp/monoprop/detail/mpi/CMakeLists.txt index a2625b5d0..43193c85c 100644 --- a/cpp/monoprop/detail/mpi/CMakeLists.txt +++ b/cpp/monoprop/detail/mpi/CMakeLists.txt @@ -7,7 +7,6 @@ target_sources( "CheckedCount.h" "Comm.h" "CpuRelax.h" - "DuplicatedComm.h" "Exchange.h" "HybridComm.h" "MPICompat.h" diff --git a/cpp/monoprop/detail/mpi/DuplicatedComm.h b/cpp/monoprop/detail/mpi/DuplicatedComm.h deleted file mode 100644 index b53730bf8..000000000 --- a/cpp/monoprop/detail/mpi/DuplicatedComm.h +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2026 Algorithmiq -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#pragma once - -#include -#include - -#include "monoprop/detail/mpi/Comm.h" - -namespace monoprop::mpi { - -/// Owns a private duplicate of a caller's communicator and frees it on destruction. -/// -/// monoprop talks point-to-point on fixed tags (see Pairwise.h), so running on the caller's own -/// communicator would let its messages and ours match each other. Immovable: the handle is freed exactly -/// once, by whoever holds the last `std::shared_ptr` (see duplicate_user_comm()). -class DuplicatedComm { -public: - /// Collective over `parent`: every rank of the communicator must construct one, in the same order. - explicit DuplicatedComm(MPI_Comm parent) : handle_(parent) { -#ifdef monoprop_ENABLE_MPI - if (MPI_Comm_dup(parent, &handle_) != MPI_SUCCESS) { - throw std::runtime_error("MPI_Comm_dup failed"); - } -#endif - } - - DuplicatedComm(const DuplicatedComm &) = delete; - auto operator=(const DuplicatedComm &) -> DuplicatedComm & = delete; - - // Python objects can outlive MPI_Finalize (mpi4py finalizes at interpreter exit), and freeing a - // communicator afterwards is an error. - ~DuplicatedComm() { -#ifdef monoprop_ENABLE_MPI - int finalized = 0; - MPI_Finalized(&finalized); - if (finalized == 0) { - MPI_Comm_free(&handle_); - } -#endif - } - - /// The duplicate; valid until the last owner is destroyed. - [[nodiscard]] auto handle() const -> MPI_Comm { return handle_; } - -private: - MPI_Comm handle_; -}; - -/// Duplicates `comm` when it is a caller-supplied MPI communicator, collectively over it. -/// -/// Returns null when there is nothing to isolate: an in-process (Shm / Hybrid) handle, which is already -/// owned by its PartitionGroup, MPI_COMM_NULL, and every build without MPI. -[[nodiscard]] inline auto duplicate_user_comm(const Comm &comm) -> std::shared_ptr { -#ifdef monoprop_ENABLE_MPI - if (comm.kind == Comm::Kind::Mpi && comm.mpi != MPI_COMM_NULL) { - return std::make_shared(comm.mpi); - } -#else - (void)comm; -#endif - return nullptr; -} - -} // namespace monoprop::mpi diff --git a/cpp/tests/comm_duplication_tests.cpp b/cpp/tests/comm_duplication_tests.cpp deleted file mode 100644 index 14bc98e5e..000000000 --- a/cpp/tests/comm_duplication_tests.cpp +++ /dev/null @@ -1,142 +0,0 @@ -// Copyright 2026 Algorithmiq -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// A propagator talks on a private duplicate of the communicator it is given, so its fixed-tag -// point-to-point traffic cannot match the caller's own messages on that communicator. - -#include - -#ifdef monoprop_ENABLE_MPI - -#include -#include -#include -#include - -#include "TestUtilities.h" -#include "monoprop/MonomialPropagator.h" -#include "monoprop/detail/mpi/MPICompat.h" - -namespace { - -using namespace monoprop; -using namespace test_utils; - -constexpr size_t kNumModes = 8; - -auto compare_comms(MPI_Comm lhs, MPI_Comm rhs) -> int { - int result = MPI_UNEQUAL; - MPI_Comm_compare(lhs, rhs, &result); - return result; -} - -BOOST_AUTO_TEST_CASE(propagator_runs_on_a_private_duplicate_of_its_communicator) { - const auto data = load_case_data("random_exact.msgpack"); - for (const MPI_Comm user_comm : {MPI_COMM_SELF, MPI_COMM_WORLD}) { - auto sim = build_simulator(data, SimulatorConfig{.comm = user_comm}); - BOOST_TEST(sim.comm() != user_comm); - BOOST_TEST(compare_comms(sim.comm(), user_comm) == MPI_CONGRUENT); - } -} - -// Counts the communicators an attribute was deleted from: MPI runs the delete callback when a -// communicator carrying it is freed, which is the only way to observe a free from outside. -int g_deleted_attributes = 0; - -auto copy_attribute(MPI_Comm, int, void *, void *value_in, void *value_out, int *flag) -> int { - *static_cast(value_out) = value_in; - *flag = 1; - return MPI_SUCCESS; -} - -auto count_deleted_attribute(MPI_Comm, int, void *, void *) -> int { - ++g_deleted_attributes; - return MPI_SUCCESS; -} - -BOOST_AUTO_TEST_CASE(duplicate_is_shared_by_copies_and_freed_with_the_last_owner) { - const auto data = load_case_data("random_exact.msgpack"); - MPI_Comm user_comm = MPI_COMM_NULL; - MPI_Comm_dup(MPI_COMM_WORLD, &user_comm); - int keyval = MPI_KEYVAL_INVALID; - MPI_Comm_create_keyval(copy_attribute, count_deleted_attribute, &keyval, nullptr); - MPI_Comm_set_attr(user_comm, keyval, nullptr); - g_deleted_attributes = 0; - - auto original = std::make_unique>( - build_simulator(data, SimulatorConfig{.comm = user_comm})); - auto copy = std::make_unique>(*original); - BOOST_TEST(copy->comm() == original->comm()); - - const MPI_Comm duplicate = copy->comm(); - original.reset(); - BOOST_TEST(g_deleted_attributes == 0); // the copy still owns it - BOOST_TEST(MPI_Barrier(duplicate) == MPI_SUCCESS); - - copy.reset(); - BOOST_TEST(g_deleted_attributes == 1); // the duplicate, and only it, was freed - - MPI_Comm_free(&user_comm); - BOOST_TEST(g_deleted_attributes == 2); // the caller's own communicator is still theirs to free - MPI_Comm_free_keyval(&keyval); -} - -// The caller's message sits unmatched on the very communicator monoprop is handed, one per internal tag. -// Without the duplicate, monoprop's own receives (same source, same tag) would consume it. -BOOST_AUTO_TEST_CASE(user_messages_on_internal_tags_survive_propagation) { - const int rank = mpi::rank(MPI_COMM_WORLD); - const int size = mpi::size(MPI_COMM_WORLD); - if (size < 2) { - BOOST_TEST_MESSAGE("Skipping user_messages_on_internal_tags_survive_propagation: world size = 1."); - return; - } - const int partner = rank ^ 1; - const bool has_partner = partner < size; - constexpr std::array tags{mpi::kHybridCountTag, - mpi::kHybridPayloadTag, - mpi::kFlatPayloadTag, - mpi::kFlatCountTag, - mpi::kFlatReplayTag}; - - std::array outgoing{}; - std::vector sends; - for (size_t i = 0; i < tags.size() && has_partner; ++i) { - outgoing[i] = (1000 * rank) + tags[i]; - sends.emplace_back(); - MPI_Isend(&outgoing[i], 1, MPI_INT, partner, tags[i], MPI_COMM_WORLD, &sends.back()); - } - - const auto data = load_case_data("random_exact.msgpack"); - auto sim = build_simulator(data, SimulatorConfig{.comm = MPI_COMM_WORLD}); - BOOST_CHECK_SMALL(evaluate_expval(sim, data, false) - data.actual_expval, 1e-9); - - // Polled against a deadline so a consumed message fails the test instead of hanging it. - const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(30); - for (size_t i = 0; i < tags.size() && has_partner; ++i) { - int incoming = -1; - MPI_Request receive = MPI_REQUEST_NULL; - MPI_Irecv(&incoming, 1, MPI_INT, partner, tags[i], MPI_COMM_WORLD, &receive); - int done = 0; - while (done == 0 && std::chrono::steady_clock::now() < deadline) { - MPI_Test(&receive, &done, MPI_STATUS_IGNORE); - } - BOOST_REQUIRE_MESSAGE(done != 0, "the user's message with tag " << tags[i] << " was consumed"); - BOOST_TEST(incoming == (1000 * partner) + tags[i]); - } - MPI_Waitall(static_cast(sends.size()), sends.data(), MPI_STATUSES_IGNORE); -} - -} // namespace - -#endif // monoprop_ENABLE_MPI diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 3f1af0f3a..5e1559993 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -122,18 +122,6 @@ built for raises [MPIAbiMismatchError][monoprop.exceptions.MPIAbiMismatchError], library older than the supported versions (see [Enabling MPI](#enabling-mpi)) raises [MPIVersionError][monoprop.exceptions.MPIVersionError]. -### Communicator ownership - -monoprop never trades messages on the communicator you pass. Building a propagator is collective over -`comm`: every rank must construct it, in the same order relative to its other collectives on `comm`. It -duplicates `comm` once, runs all of its traffic on the duplicate, and frees the duplicate when the -propagator is destroyed. monoprop's point-to-point messages therefore cannot match yours, even on a -shared tag, and you keep ownership of `comm` itself. - -A deep copy of a propagator shares the original's duplicate instead of making another, so -copying stays non-collective. The duplicate is released when the last copy goes. A propagator that -outlives `MPI_Finalize`, such as one still referenced at interpreter exit, skips the free. - ### Single-node (`MPI.COMM_SELF`) ```python notest @@ -143,13 +131,20 @@ sim = MajoranaPropagator(..., comm=MPI.COMM_SELF) ### Multi-node (`MPI.COMM_WORLD`) -Replace the communicator and launch with `mpiexec`: +Pass a duplicate of `MPI.COMM_WORLD` and launch with `mpiexec`: ```python notest from mpi4py import MPI -sim = MajoranaPropagator(..., comm=MPI.COMM_WORLD) +comm = MPI.COMM_WORLD.Dup() +sim = MajoranaPropagator(..., comm=comm) +... +comm.Free() # once sim is no longer used ``` +monoprop sends point-to-point messages on fixed tags over `comm`, so they could match messages your own +code has in flight on the same communicator. A duplicate keeps the two apart. It must outlive the +propagator. + ```bash mpiexec -n 8 uv run python your_script.py ``` diff --git a/src/monoprop/majorana_propagator.py b/src/monoprop/majorana_propagator.py index fa432dfa6..d9a6a968e 100644 --- a/src/monoprop/majorana_propagator.py +++ b/src/monoprop/majorana_propagator.py @@ -111,9 +111,9 @@ def __init__( exception applies on top of either. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of complexity. - comm: Optional MPI communicator. Construction is collective over it. The simulator - runs on a private duplicate, so its messages never mix with the caller's on - ``comm``; the duplicate is freed with the simulator. + comm: Optional MPI communicator (must outlive the simulator). Pass a duplicate, + such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends + point-to-point messages on: monoprop uses fixed tags that could match them. """ majorana_operator: MajoranaOperator = ( initial_operator diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index 131d96dc2..cda6e6dce 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -104,9 +104,9 @@ def _init_simulator( lower_atol: Coefficient-truncation threshold, or ``None``. upper_atol: Coefficient-retention threshold, or ``None``. basis_change: Internal per-Majorana basis change (``2 * num_modes`` entries). - comm: Optional MPI communicator. Construction is collective over it. The propagator - runs on a private duplicate, so its messages never mix with the caller's on - ``comm``; the duplicate is freed with the propagator. + comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate, + such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends + point-to-point messages on: monoprop uses fixed tags that could match them. basis: Engine basis -- ``"majorana"`` (default) or ``"pauli"``. """ num_modes = majorana_operator.num_modes diff --git a/src/monoprop/pauli_propagator.py b/src/monoprop/pauli_propagator.py index 8f798df69..1343d4bc5 100644 --- a/src/monoprop/pauli_propagator.py +++ b/src/monoprop/pauli_propagator.py @@ -70,9 +70,9 @@ def __init__( so a value approaching ``num_qubits`` is rejected with a ``RuntimeError`` rather than started. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of weight. - comm: Optional MPI communicator. Construction is collective over it. The propagator - runs on a private duplicate, so its messages never mix with the caller's on - ``comm``; the duplicate is freed with the propagator. + comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate, + such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends + point-to-point messages on: monoprop uses fixed tags that could match them. """ # The engine takes the Schrodinger cutoff in slots (two per qubit); this API in qubits. schrodinger_cutoff = ( diff --git a/tests/conftest.py b/tests/conftest.py index 20bc0be0b..6bb4ec206 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,8 +21,8 @@ ``evolved_operator``, ``contract_partially``, ``size()``, or ``graph_size()``. Both hand out session-wide duplicates of those communicators, never the predefined handles -themselves, so the suite's traffic stays off the predefined communicators that conftest and -mpi4py use. +themselves, as the parallelism docs tell users to do: monoprop's point-to-point traffic then +cannot match messages that conftest or mpi4py exchange on the predefined communicators. Each duplicate is created on first use and freed at session teardown: ``Dup`` is collective, and doing it per test would add a collective to every one of them for no extra isolation. @@ -35,7 +35,7 @@ import os import tempfile from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING import pytest @@ -45,6 +45,8 @@ if TYPE_CHECKING: from collections.abc import Iterator + from mpi4py.MPI import Intracomm + from tests.reference_data import FermionicProblem @@ -133,7 +135,7 @@ def pytest_runtest_setup(item: pytest.Item) -> None: @pytest.fixture(scope="session") -def comm_self_duplicate() -> Iterator[Any]: +def comm_self() -> Iterator[Intracomm | None]: """Session-wide duplicate of COMM_SELF; ``None`` when monoprop has no MPI backend.""" if not HAS_MPI_BACKEND: yield None @@ -144,7 +146,7 @@ def comm_self_duplicate() -> Iterator[Any]: @pytest.fixture(scope="session") -def comm_world_duplicate() -> Iterator[Any]: +def comm_world() -> Iterator[Intracomm | None]: """Session-wide duplicate of COMM_WORLD; ``None`` when monoprop has no MPI backend.""" if not HAS_MPI_BACKEND: yield None @@ -158,14 +160,14 @@ def comm_world_duplicate() -> Iterator[Any]: [pytest.param(None, id="comm_none")] if not HAS_MPI_BACKEND else [ - pytest.param("comm_self_duplicate", id="comm_self"), - pytest.param("comm_world_duplicate", id="comm_world"), + pytest.param("comm_self", id="comm_self"), + pytest.param("comm_world", id="comm_world"), ] ) @pytest.fixture(params=_COMM_PARAMS) -def comm(request: pytest.FixtureRequest) -> Any: # noqa: ANN401 +def comm(request: pytest.FixtureRequest) -> Intracomm | None: """Parametrized communicator; ``None`` when mpi4py is unavailable or monoprop has no MPI backend. Resolved through the session-scoped duplicates, so parametrizing does not reorder the run. @@ -176,9 +178,9 @@ def comm(request: pytest.FixtureRequest) -> Any: # noqa: ANN401 @pytest.fixture -def serial_comm(comm_self_duplicate: Any) -> Any: # noqa: ANN401 +def serial_comm(comm_self: Intracomm | None) -> Intracomm | None: """Single-rank communicator for tests that inspect rank-local operator state.""" - return comm_self_duplicate + return comm_self @pytest.fixture( diff --git a/tests/test_evolved_operator_coefficients.py b/tests/test_evolved_operator_coefficients.py index 0152de41c..a28bf7a05 100644 --- a/tests/test_evolved_operator_coefficients.py +++ b/tests/test_evolved_operator_coefficients.py @@ -47,7 +47,7 @@ def problem(): @pytest.fixture(scope="module") -def prop_and_params(problem, comm_self_duplicate): +def prop_and_params(problem, comm_self): """A built propagator over ``problem`` and its parameters, shared across the module. ``evolved_operator`` and ``evolved_operator_coefficients`` leave simulator state alone, so one @@ -57,7 +57,7 @@ def prop_and_params(problem, comm_self_duplicate): problem.operator, problem.monomial_circuit.initial_state, cutoff=2 * problem.n_modes, - comm=comm_self_duplicate, + comm=comm_self, ) prop.build_graph(problem.monomial_circuit.to_circuit()) return prop, problem.monomial_circuit.parameters diff --git a/tests/test_monoprop_mpi.py b/tests/test_monoprop_mpi.py index 54f584915..ad02d6d0a 100644 --- a/tests/test_monoprop_mpi.py +++ b/tests/test_monoprop_mpi.py @@ -19,8 +19,6 @@ from __future__ import annotations -import time - import numpy as np import pytest @@ -49,11 +47,6 @@ def _make_mp(problem, comm, *, schrodinger=False): return mp, circuit -# The point-to-point tags monoprop's exchanges use (cpp/monoprop/detail/mpi/Pairwise.h). -_INTERNAL_TAGS = tuple(range(0x6D70, 0x6D75)) -_RECEIVE_TIMEOUT_SECONDS = 30 - - def _assert_expval(expval, expected, atol=1e-9): assert np.isclose(expval, expected, atol=atol), ( f"Expectation value mismatch: {expval} vs {expected}" @@ -198,42 +191,3 @@ def test_mpi_comm_self(self, lih_fermionic_spin_exact): lih_fermionic_spin_exact.monomial_circuit.parameters ) _assert_expval(expval, lih_fermionic_spin_exact.exact_expval) - - @pytest.mark.mpi(min_size=2) - def test_user_messages_on_internal_tags_survive_propagation( - self, lih_fermionic_spin_exact - ): - world = MPI.COMM_WORLD - partner = world.Get_rank() ^ 1 - has_partner = partner < world.Get_size() - outgoing = { - tag: np.array([1000 * world.Get_rank() + tag], dtype=np.int64) - for tag in _INTERNAL_TAGS - } - sends = ( - [ - world.Isend([payload, MPI.INT64_T], dest=partner, tag=tag) - for tag, payload in outgoing.items() - ] - if has_partner - else [] - ) - - mp, circuit = _make_mp(lih_fermionic_spin_exact, world) - mp.build_graph(circuit) - expval = mp.expval_functional(pare_threshold=1e-10)( - lih_fermionic_spin_exact.monomial_circuit.parameters - ) - _assert_expval(expval, lih_fermionic_spin_exact.exact_expval) - - # Polled against a deadline so a consumed message fails the test instead of hanging it. - deadline = time.monotonic() + _RECEIVE_TIMEOUT_SECONDS - for tag in _INTERNAL_TAGS if has_partner else (): - incoming = np.zeros(1, dtype=np.int64) - receive = world.Irecv([incoming, MPI.INT64_T], source=partner, tag=tag) - arrived = receive.Test() - while not arrived and time.monotonic() < deadline: - arrived = receive.Test() - assert arrived, f"the user's message with tag {tag} was consumed" - assert incoming[0] == 1000 * partner + tag - MPI.Request.Waitall(sends) From 8772ef7247c2621a1a62e424fadd9d0087662e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Wed, 28 Oct 2026 08:36:43 +0100 Subject: [PATCH 3/5] =?UTF-8?q?docs(mpi):=20=F0=9F=93=9D=20keep=20the=20du?= =?UTF-8?q?plicate=20alive=20for=20functionals=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A functional holds the propagator's communicator handle, so the duplicate must outlive every functional, not just the propagator. Assisted-by: Pi:claude-opus-5-5 --- docs/content/docs/features/parallelism.mdx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 5e1559993..274e2b7a9 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -138,12 +138,12 @@ from mpi4py import MPI comm = MPI.COMM_WORLD.Dup() sim = MajoranaPropagator(..., comm=comm) ... -comm.Free() # once sim is no longer used +comm.Free() # once sim and its functionals are no longer used ``` monoprop sends point-to-point messages on fixed tags over `comm`, so they could match messages your own code has in flight on the same communicator. A duplicate keeps the two apart. It must outlive the -propagator. +propagator and every functional obtained from it. ```bash mpiexec -n 8 uv run python your_script.py From 23501ec55fbefa475912d819152e1d406b021b19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Fri, 30 Oct 2026 10:47:19 +0100 Subject: [PATCH 4/5] =?UTF-8?q?fix(tests):=20=F0=9F=94=92=20refuse=20a=20p?= =?UTF-8?q?er-rank=20basetemp=20parent=20that=20another=20user=20controls?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-rank --basetemp skips the checks pytest applies to its own pytest-of- directory. On a shared temporary root another user could create that predictable name first and tamper with the tests' files. The parent is now refused when it is a symbolic link or not owned by the current user, and made private (0700) otherwise, as pytest does. Follows up #426, found by CodeRabbit after it merged. Assisted-by: Pi:claude-opus-5-5 --- tests/conftest.py | 25 ++++++++++++++++++++++- tests/test_mpi_configuration.py | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/tests/conftest.py b/tests/conftest.py index 6bb4ec206..e5c700a37 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -33,6 +33,7 @@ import getpass import os +import stat import tempfile from pathlib import Path from typing import TYPE_CHECKING @@ -68,6 +69,28 @@ def _rank_basetemp() -> Path | None: return temproot / f"pytest-of-{user}" / f"mpi-rank-{MPI.COMM_WORLD.rank}" +def _make_private_dir(path: Path) -> None: + """Create ``path`` if needed and make sure only the current user can use it. + + The checks pytest applies to its own ``pytest-of-``, which an explicit ``--basetemp`` + skips: on a shared temporary root another user could create the predictable name first. + """ + path.mkdir(mode=0o700, parents=True, exist_ok=True) + # Not following symlinks: a swapped-in link would otherwise pass every check below. + info = path.lstat() + if stat.S_ISLNK(info.st_mode): + raise OSError( + f"The temporary directory {path} is a symbolic link. Fix this and try again." + ) + if hasattr(os, "getuid") and info.st_uid != os.getuid(): + raise OSError( + f"The temporary directory {path} is not owned by the current user. " + "Fix this and try again." + ) + if info.st_mode & 0o077: + path.chmod(0o700) + + @pytest.hookimpl(tryfirst=True) def pytest_cmdline_main(config: pytest.Config) -> None: """Give each MPI rank its own ``--basetemp`` unless one was passed explicitly. @@ -77,7 +100,7 @@ def pytest_cmdline_main(config: pytest.Config) -> None: """ if config.option.basetemp is None and (basetemp := _rank_basetemp()) is not None: # pytest creates only the basetemp itself when one is given, not its parent. - basetemp.parent.mkdir(mode=0o700, parents=True, exist_ok=True) + _make_private_dir(basetemp.parent) config.option.basetemp = basetemp diff --git a/tests/test_mpi_configuration.py b/tests/test_mpi_configuration.py index 8668b5066..34a6cd886 100644 --- a/tests/test_mpi_configuration.py +++ b/tests/test_mpi_configuration.py @@ -159,3 +159,38 @@ def test_basetemp_parent_is_created(monkeypatch, tmp_path): config.option.basetemp = None conftest.pytest_cmdline_main(config) assert Path(config.option.basetemp).parent.is_dir() + + +def _basetemp_for_rank(monkeypatch, temproot: Path) -> Path: + monkeypatch.setattr(conftest, "MPI", Mock(COMM_WORLD=Mock(size=3, rank=2))) + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(temproot)) + config = Mock() + config.option.basetemp = None + conftest.pytest_cmdline_main(config) + return Path(config.option.basetemp) + + +def test_basetemp_parent_owned_by_another_user_is_refused(monkeypatch, tmp_path): + monkeypatch.setattr(conftest.os, "getuid", lambda: tmp_path.stat().st_uid + 1) + with pytest.raises(OSError, match="not owned by the current user"): + _basetemp_for_rank(monkeypatch, tmp_path) + + +def test_basetemp_parent_symlink_is_refused(monkeypatch, tmp_path): + temproot = tmp_path / "temproot" + temproot.mkdir() + elsewhere = tmp_path / "elsewhere" + elsewhere.mkdir(mode=0o700) + monkeypatch.setattr(conftest.getpass, "getuser", lambda: "someone") + (temproot / "pytest-of-someone").symlink_to(elsewhere) + with pytest.raises(OSError, match="symbolic link"): + _basetemp_for_rank(monkeypatch, temproot) + + +def test_basetemp_parent_is_made_private(monkeypatch, tmp_path): + monkeypatch.setattr(conftest.getpass, "getuser", lambda: "someone") + parent = tmp_path / "pytest-of-someone" + parent.mkdir(mode=0o755) + parent.chmod(0o755) + assert _basetemp_for_rank(monkeypatch, tmp_path).parent == parent + assert parent.stat().st_mode & 0o777 == 0o700 From 02e640d15f8b94b38e9852915b354aa7d98041ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Roberto=20Di=20Remigio=20Eik=C3=A5s?= Date: Fri, 30 Oct 2026 10:47:20 +0100 Subject: [PATCH 5/5] =?UTF-8?q?docs(mpi):=20=F0=9F=93=9D=20reserve=20the?= =?UTF-8?q?=20duplicate=20communicator=20against=20the=20caller's=20receiv?= =?UTF-8?q?es=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A receive with MPI.ANY_TAG on the same communicator can take one of monoprop's point-to-point messages, not only the caller's sends can collide. The comm docstrings and the parallelism guide now ask for a duplicate reserved for monoprop. Found by CodeRabbit. Assisted-by: Pi:claude-opus-5-5 --- docs/content/docs/features/parallelism.mdx | 7 ++++--- src/monoprop/majorana_propagator.py | 7 ++++--- src/monoprop/monomial_propagator.py | 7 ++++--- src/monoprop/pauli_propagator.py | 7 ++++--- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/docs/content/docs/features/parallelism.mdx b/docs/content/docs/features/parallelism.mdx index 274e2b7a9..0a4d493d4 100644 --- a/docs/content/docs/features/parallelism.mdx +++ b/docs/content/docs/features/parallelism.mdx @@ -141,9 +141,10 @@ sim = MajoranaPropagator(..., comm=comm) comm.Free() # once sim and its functionals are no longer used ``` -monoprop sends point-to-point messages on fixed tags over `comm`, so they could match messages your own -code has in flight on the same communicator. A duplicate keeps the two apart. It must outlive the -propagator and every functional obtained from it. +monoprop exchanges point-to-point messages on fixed tags over `comm`, so your own sends and receives +on the same communicator could match them; a receive with `MPI.ANY_TAG` could even take one of +monoprop's messages and leave it waiting. Reserve the duplicate for monoprop, with no point-to-point +traffic of your own on it. It must outlive the propagator and every functional obtained from it. ```bash mpiexec -n 8 uv run python your_script.py diff --git a/src/monoprop/majorana_propagator.py b/src/monoprop/majorana_propagator.py index d9a6a968e..0072ea386 100644 --- a/src/monoprop/majorana_propagator.py +++ b/src/monoprop/majorana_propagator.py @@ -111,9 +111,10 @@ def __init__( exception applies on top of either. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of complexity. - comm: Optional MPI communicator (must outlive the simulator). Pass a duplicate, - such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends - point-to-point messages on: monoprop uses fixed tags that could match them. + comm: Optional MPI communicator (must outlive the simulator). Pass a duplicate + reserved for monoprop, such as ``MPI.COMM_WORLD.Dup()``: monoprop exchanges + point-to-point messages on fixed tags, which your own sends and receives on the + same communicator, including ``MPI.ANY_TAG`` receives, could match. """ majorana_operator: MajoranaOperator = ( initial_operator diff --git a/src/monoprop/monomial_propagator.py b/src/monoprop/monomial_propagator.py index cda6e6dce..5b840e492 100644 --- a/src/monoprop/monomial_propagator.py +++ b/src/monoprop/monomial_propagator.py @@ -104,9 +104,10 @@ def _init_simulator( lower_atol: Coefficient-truncation threshold, or ``None``. upper_atol: Coefficient-retention threshold, or ``None``. basis_change: Internal per-Majorana basis change (``2 * num_modes`` entries). - comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate, - such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends - point-to-point messages on: monoprop uses fixed tags that could match them. + comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate + reserved for monoprop, such as ``MPI.COMM_WORLD.Dup()``: monoprop exchanges + point-to-point messages on fixed tags, which your own sends and receives on the + same communicator, including ``MPI.ANY_TAG`` receives, could match. basis: Engine basis -- ``"majorana"`` (default) or ``"pauli"``. """ num_modes = majorana_operator.num_modes diff --git a/src/monoprop/pauli_propagator.py b/src/monoprop/pauli_propagator.py index 1343d4bc5..009bf4b33 100644 --- a/src/monoprop/pauli_propagator.py +++ b/src/monoprop/pauli_propagator.py @@ -70,9 +70,10 @@ def __init__( so a value approaching ``num_qubits`` is rejected with a ``RuntimeError`` rather than started. lower_atol: Monomials with ``|coeff| < lower_atol`` are discarded during evolution. upper_atol: Monomials with ``|coeff| > upper_atol`` are kept regardless of weight. - comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate, - such as ``MPI.COMM_WORLD.Dup()``, not a communicator your own code sends - point-to-point messages on: monoprop uses fixed tags that could match them. + comm: Optional MPI communicator (must outlive the propagator). Pass a duplicate + reserved for monoprop, such as ``MPI.COMM_WORLD.Dup()``: monoprop exchanges + point-to-point messages on fixed tags, which your own sends and receives on the + same communicator, including ``MPI.ANY_TAG`` receives, could match. """ # The engine takes the Schrodinger cutoff in slots (two per qubit); this API in qubits. schrodinger_cutoff = (