Add submodule mpi by wdeconinck · Pull Request #40 · GridTools/atlas4py

wdeconinck · 2026-08-31T08:00:13Z

This pull request introduces a new atlas4py.mpi Python submodule, providing comprehensive MPI communicator management from eckit and interoperability with mpi4py and other MPI libraries. It includes the C++ implementation, Python bindings, CMake integration, version parsing utilities, and extensive test coverage. The changes are grouped below by theme.

New MPI Python Submodule and Bindings

Added a new mpi submodule to atlas4py, exposing MPI communicator management, registration, and context management in Python, with support for interoperability with mpi4py and handling communicators as integer handles or objects.

CMake Integration and Version Utilities

Introduced a reusable CMake function (lifted from ecbuild) and helper file for parsing version strings and exporting version components as variables, including an integer encoding for version comparison (src/atlas4py/cmake/atlas4py_parse_version.cmake). This is needed for ensuring forward/backward compatibility with different atlas versions for an API change.
Updated the main CMake configuration to build and link the new MPI submodule, include source directories, and propagate version information for conditional compilation (src/atlas4py/CMakeLists.txt).

Testing

Added comprehensive tests for all new MPI submodule features, including communicator creation, registration, conversion to/from integers and mpi4py, context management with Scope, and communicator splitting (tests/test_bindings.py).

These changes provide robust MPI support in atlas4py, enable seamless integration with existing Python MPI workflows, and ensure correctness and maintainability through improved build tooling and thorough testing.

Copilot

Pull request overview

Adds MPI communicator management and interoperability to the atlas4py Python bindings.

Changes:

Adds communicator bindings, registration, scoping, splitting, and mpi4py conversion.
Adds Atlas version parsing and conditional compilation support.
Adds MPI binding tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`tests/test_bindings.py`	Tests MPI APIs and interoperability.
`src/atlas4py/mpi/_mpi.hpp`	Declares MPI wrappers.
`src/atlas4py/mpi/_mpi.cpp`	Implements and binds MPI functionality.
`src/atlas4py/CMakeLists.txt`	Builds the MPI binding sources.
`src/atlas4py/cmake/atlas4py_parse_version.cmake`	Parses version components.
`src/atlas4py/cmake/atlas4py_add_atlas.cmake`	Integrates Atlas version detection.
`src/atlas4py/_atlas4py.cpp`	Registers the MPI submodule.

Suppressed comments (1)

tests/test_bindings.py:497

The repository runs this suite with plain pytest and has no mpiexec/mpirun test invocation, so world.size is one and color is always zero here. The test only covers the degenerate serial split and cannot detect multi-rank grouping/collective failures. Add an MPI-launched multi-process case with ranks assigned to both colors.

    color = world.rank % 2
    split_comm = world.split(color, "split_comm")

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

tehrengruber

First round. Fable review otw.

tehrengruber · 2026-09-21T07:18:59Z

        atlas4py.Config.from_file(test_config_yaml, format="toml")

    assert "does not select a parser or validate the file contents" in atlas4py.Config.from_file.__doc__
+


New test file maybe?

Yes that makes sense. We're modularising library code, so then also test code.

tehrengruber · 2026-09-21T07:21:08Z

@@ -0,0 +1,99 @@
+# This file defines a function to parse version strings and export version components as CMake variables.


No strong objection, but until there is a user that needs this I would KISS and rather only support the latest atlas api in the bindings so that no compatibility logic is required.

So you mean that we should bump the required atlas version instead? I'm OK with that of course!
Let's make that a new policy from now on then.

tehrengruber · 2026-09-21T07:23:56Z

@@ -0,0 +1,274 @@
+


Should we get rid of the underscores in the filename? For _atlas4py.cpp I believe the reason is that this is also the name of the python module which (at least somewhat) makes sense.

Sounds sensible indeed.

tehrengruber-ai

Nice work — the interop design is sound: to_int() maps to eckit::mpi::Comm::communicator(), which is the Fortran handle, exactly what mpi4py's toint()/fromint() use, so the round-trip is correct. The duck-typed toint/to_int dispatch is a good touch.

Four correctness issues below. I have some further notes on the CMake version plumbing, exception types, GIL handling around blocking MPI calls, and test isolation, but those are minor — happy to follow up separately.

tehrengruber-ai · 2026-09-21T08:34:34Z

+        .def( "__repr__", []( const mpi::Scope& scope ) {
+            return "atlas4py.mpi.Scope(name=" + atlas::mpi::comm().name() + ")";
+        }, "Return a string representation of the MPI scope." );


Scope.__repr__ reports the wrong name.

The scope parameter is unused — this prints the current default comm rather than the scope's own name. Scope never stores its name, so nested scopes report the innermost one:

with mpi.Scope("scope 1") as s1: with mpi.Scope("scope 2"): repr(s1) # -> "atlas4py.mpi.Scope(name=scope 2)"

test_scope_repr_and_str passes only because it exercises a single active scope.

Suggest storing name_ in Scope (see the related comment on pop(), which needs it anyway) and printing that.

tehrengruber-ai · 2026-09-21T08:34:35Z

+        static void pop() {
+            #if ATLAS4PY_ATLAS_VERSION_AT_LEAST(0,46,0)
+                atlas::mpi::scope::pop();
+            #else
+                atlas::mpi::pop();
+            #endif
+        }


pop() uses the unchecked overload, which can silently pop the wrong scope.

atlas exposes both pop() and pop(name) — and atlas::mpi::Scope itself deliberately uses the checked one:

// atlas/parallel/mpi/mpi.h struct Scope { Scope(std::string_view name) : name_(name) { push(name_); } ~Scope() { pop(name_); } // verifies the name matches };

Python object destruction is not guaranteed to be LIFO, so an out-of-order collection pops someone else's scope instead of raising:

s1 = mpi.Scope("a") # push a s2 = mpi.Scope("b") # push b del s1 # pops "b"

Related, in the constructor at lines 172-174: the push happens at construction rather than in __enter__, so mpi.Scope("world") mutates global state the moment it is constructed and unwinds only at GC. test_scope_repr_and_str does exactly this, leaving "world" pushed for as long as the object survives.

Two fixes worth considering: store the name and pass it to pop, and/or move the push into __enter__.

tehrengruber-ai · 2026-09-21T08:34:35Z

+    static const Comm& SELF();
+
+private:
+    const eckit::mpi::Comm& comm_;


This reference dangles after delete_comm.

mpi::Comm holds const eckit::mpi::Comm&, but eckit documents deleteComm as "Unregister and delete specific comm", and Comm::free() as "After calling this method, the communicator should not be used again". Any live Python Comm pointing at that object is then dangling. Same applies to unregister_comm on eckit >= 2.0.

The new test does this:

# tests/test_bindings.py, test_mpi_comm_split split_comm_by_name = mpi.Comm("split_comm") ... mpi.delete_comm("split_comm") # split_comm and split_comm_by_name now dangle

It survives today only because name_ / size_ / rank_ were already cached before the delete — to_int() on either object afterwards is a use-after-free.

Storing the name and resolving through atlas::mpi::comm(name_) on each access would make this safe (at the cost of a lookup), or the lifetime constraint needs documenting loudly.

tehrengruber-ai · 2026-09-21T08:34:35Z

+    if ( register_if_not_found == RegisterIfNotFound::yes ) {
+        std::string name = comm_name_from_int(comm_as_int);
+        eckit::mpi::addComm( name.c_str(), comm_as_int );
+        return atlas::mpi::comm( name );
+    }


Unknown integer handles are registered without validation.

Comm(int) always passes RegisterIfNotFound::yes, so Comm(9999) registers handle 9999 as "int.9999" with no check that it is a real communicator. Under a real MPI build eckit will convert and use that handle (MPI_Comm_f2c, then MPI_Comm_size), which aborts.

test_mpi_scope's Mock_mpi4py_MPI_Comm(9999) and test_mpi_comm_registration's handle 6 pass only against the serial stub — they would fail under mpirun.

Worth either validating the handle before registering, or restricting those tests to serial builds so the suite stays runnable under a real MPI.

Minor, same block: the RegisterIfNotFound::no branch is never reached — no caller passes it. And at line 59, for( std::string name : ... ) copies each string; const auto& would do.

wdeconinck requested a balanced review from Copilot August 31, 2026 08:00

Copilot started reviewing on behalf of wdeconinck August 31, 2026 08:00

View session

wdeconinck force-pushed the mpi branch from 9bea162 to 05336bb Compare August 31, 2026 08:03

Copilot AI reviewed Aug 31, 2026

View reviewed changes

wdeconinck force-pushed the mpi branch 2 times, most recently from ca9589a to cb9e259 Compare September 1, 2026 20:28

wdeconinck added 5 commits September 1, 2026 23:05

Add submodule mpi

dd4e6cf

Fix atlas4py_parse_version duplication and possible undefined variables

d0cb7e5

Fix commented example

2ed68d3

Be more explicit that the mpi comm is converted to int

fd337a6

Cleanup

d0badb5

wdeconinck force-pushed the mpi branch from cb9e259 to d0badb5 Compare September 1, 2026 21:05

wdeconinck requested a review from tehrengruber September 1, 2026 21:06

wdeconinck marked this pull request as ready for review September 1, 2026 21:06

tehrengruber reviewed Sep 21, 2026

View reviewed changes

tehrengruber-ai reviewed Sep 21, 2026

View reviewed changes

		atlas4py.Config.from_file(test_config_yaml, format="toml")

		assert "does not select a parser or validate the file contents" in atlas4py.Config.from_file.__doc__

		@@ -0,0 +1,99 @@
		# This file defines a function to parse version strings and export version components as CMake variables.

Conversation

wdeconinck commented Aug 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tehrengruber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tehrengruber-ai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants