Document how to use parallel flow from Python - #31

The underline is shorter than the title (22 characters under a 33-character heading), so Sphinx emits parallel-in-python.rst:27: WARNING: Title underline too short. It recovers and the section still renders, so this is easy to miss — the workflow does not pass -W to Sphinx, which means the build succeeds and the page publishes with the warning only in the log.

While you are on this line, the heading itself reads a little awkwardly in English. Suggestion below fixes both at once, but feel free to use a shorter title such as "Example script" instead.

Now the name is exactly what you suggested.

"Without one, the grid cannot be distributed across ranks" is stronger than what the code does. There is a fourth partitioning method, simple, which splits the underlying Cartesian grid into rectangular blocks and needs no external library at all. From opm-grid, opm/grid/common/GridEnums.hpp:

enum PartitionMethod {
    /// \brief Use simple approach based on rectangular partitioning the underlying cartesian grid.
    simple=0,
    /// \brief Use Zoltan for partitioning
    zoltan=1,
    /// \brief Use METIS for partitioning
    metis=2,
    /// \brief use Zoltan on GraphOfGrid for partitioning
    zoltanGoG=3
};

I ran your example on 4 ranks with BlackOilSimulator(filename=CASE, args=["--partition-method=simple"]) and it completed normally.

What does seem true is that the default is zoltanwell (PartitionMethod in opm/simulators/flow/FlowGenericVanguard.hpp), so a build without Zoltan or METIS will fail unless the user asks for the simple method explicitly. Suggested rewording:

• A graph partitioner — Zoltan or METIS/ParMETIS — present at configure time. The default partitioning method is zoltanwell, so without one of these libraries you will need --partition-method=simple, which uses OPM's built-in rectangular partitioning of the Cartesian grid instead.

One caveat on my own testing: my build has Zoltan, so I verified that simple works and distributes the grid, not that it works in a build compiled entirely without Zoltan. The point about the default stands either way.

I apply exactly your suggested rewrite.

I do not think mpi4py belongs in a list of things needed "in addition" to run in parallel. Two measurements, both on 4 ranks with SPE1CASE1:

Without mpi4py at all. I took your example, removed the mpi4py import, read the rank from the launcher's environment variable instead, and used setup_mpi(init=True, finalize=True) so that OPM owns MPI:

Number of MPI processes:         4
done -- no mpi4py anywhere in this script

Without even calling setup_mpi(). The defaults are already init=True, finalize=True (mpi_init_{true}, mpi_finalize_{true} in opm/simulators/flow/python/PyBaseSimulator.hpp):

from opm.simulators import BlackOilSimulator

sim = BlackOilSimulator(filename="SPE1CASE1.DATA")
sim.step_init()
sim.step()
sim.step_cleanup()

Number of MPI processes:         4
Number of timesteps:             4

Same rank count and same number of timesteps as the mpi4py version, exit code 0 in both cases.

I want to be clear that this is not an argument against mpi4py — it is the right tool, and I would still reach for it. Without it the rank can only be read from a launcher-specific environment variable (OMPI_COMM_WORLD_RANK on Open MPI, PMI_RANK on MPICH), and per-rank results cannot be combined at all, which matters because get_porosity() only ever returns the calling rank's own cells. My point is only that it enables something rather than being required for something.

I think stating it that way actually strengthens the page, because it puts the setup_mpi() flags in their proper place: they are not incidental setup, they are the price of having a second party in the process that also wants to own MPI. Suggested rewrite of this bullet:

• mpi4py, if the script itself needs MPI — to print from one rank only, or to combine the per-rank results of get_porosity() and friends. It is not needed simply to run in parallel: with the default init=True, finalize=True OPM initializes MPI itself, and mpirun -np 4 python3 my_script.py works with no mpi4py at all. But once mpi4py is imported, the flag values described under "Initialising MPI" below become required rather than optional. See the mpi4py documentation <https://mpi4py.readthedocs.io/>_.

This may also answer a question a reader could have: python/test/test_mpi.py in opm-simulators does not use mpi4py, which looks inconsistent with this page until you notice that it never runs under mpirun — CMake launches it as a single process, and its print file says Using 1 MPI processes. It is testing the init/finalize bookkeeping inside one process, where the role mpi4py plays in your script is played by the previous test in the file.

I apply exactly your suggested rewrite.

USE_MPI defaults to ON — from opm-common, cmake/Modules/UseMPI.cmake:

option (USE_MPI "Use Message Passing Interface for parallel computing" ON)

So adding the flag changes nothing on a normal build, and a reader who follows the instruction and still ends up with a serial binary has no way to tell why. The real prerequisite is that an MPI implementation is installed and found by CMake, and that USE_MPI has not been switched off. Something like "MPI available at configure time. USE_MPI is ON by default, so this normally just means having an MPI implementation installed where CMake can find it" would be more actionable than naming a flag that is already set.

I changed this bullet by stating what the default should be. My first decision is to drop this bullet, but I think it is still necessary for user awareness.

Agreed, and I think keeping it is the right call. My objection was only to telling people to add a flag that is already set — as it now reads, the bullet tells them what to check if the build turns out to be serial, which is the useful version of the same information.

Three small things in this comment: the method is step_init(), not sim_step_init(); abit should be a bit; and the sentence needs a verb.

The fact is right, though — I checked, and a failing step_init() really does return 1, because executeInitStep() returns EXIT_FAILURE when it catches an exception. I reproduced it by running the four-argument constructor on 2 ranks and both ranks printed step_init() returned 1, while the same script on 1 rank returned 0. So it is worth keeping the fact and just fixing the wording:

I apply your wording fix.

Thanks — the wording is right now. One knock-on effect worth a second look, though, and it is partly my fault for suggesting two things in one comment.

The code now reads:

# step_init() returns 0 on success and 1 on failure.
sim.step_init()

The comment states a contract that the code immediately ignores, which is a slightly odd thing for an example to model: a reader may reasonably wonder whether they are supposed to do something with that 0 or 1, and the example does not show them. What I was suggesting in the other thread was to drop the checks from the script and move the fact into prose, so that the example stays short without leaving a loose end in it.

Two ways to close it, either is fine by me:

and then a sentence after the code block, for instance: "step_init(), step() and step_cleanup() each return a status code — 0 on success, 1 on failure. The example ignores them for brevity; check them in real scripts."

Or keep the comment and restore a check, which makes the example self-consistent in the other direction. I marginally prefer the first, because it also covers step() and step_cleanup() rather than singling out step_init().

For this, I decided to drop the code comment and add a note after the code block.

The restructuring is right — dropping the comment and putting the fact in prose is what I was asking for. But the sentence I gave you is wrong about step(), and I am sorry: you adopted it verbatim and it should not have gone out that way.

step_init() and step_cleanup() do return 0 on success and 1 on failure. step() does not. It returns 1 after a step that ran normally. I stepped SPE1CASE1 through all 120 of its report steps and step() returned 1 every time and 0 never:

total steps: 120
distinct return values: [1]
step_cleanup(): 0

The reason is that step() is not a status code at all. It returns FlowMain::executeStep(), which returns simulator_->runStep(...), and runStep is declared bool. Its own call site inside the simulator names it for what it is:

bool continue_looping = runStep(timer);
if (!continue_looping) break;

So it is a "keep going" flag widened to int, with the opposite polarity to the other two. As the page currently reads, someone writing if sim.step() != 0: raise ... would raise on every successful step — which is worse than saying nothing.

Suggested replacement for the sentence:

If you would rather not carry that much detail on this page, simply deleting the sentence is fine by me too — the example does not check the return values anyway, and the API reference is the natural place for the per-method contracts. Your call; I do not want to push you through another round on my mistake.

No problem at all. Now the information about the return values has been removed.

Minor consistency point: the script carefully checks what step_init() returned, but ignores what step() returns, and step() returns a status code too. To a reader learning the right pattern from this page, checking one and not the other reads like an oversight. Either check both or neither — I would probably drop to neither, to keep the example short, and mention the return codes in prose instead.

The test is now removed. It is replaced by sim.step_init().

This is correct, and I confirmed the failure: with finalize=True, a later allgather aborts the job with

*** The MPI_Comm_test_inter() function was called after MPI_FINALIZE was invoked.
*** This is disallowed by the MPI standard.
*** Your MPI job will now abort.

The one thing I would add is when the teardown happens, because it is not where a reader will look for it. MPI_Finalize() is called from Main::~Main(), so it fires when the simulator object is destroyed — in your example, when main() returns and its local sim goes out of scope. Immediately after step_cleanup(), MPI is still up and collectives still work; I checked, and MPI.Is_finalized() is still False at that point. Anyone debugging this will inspect step_cleanup() first and find nothing wrong there, so a half-sentence naming the destructor would save them real time.

Also worth knowing, if it affects how strongly you want to word this: mpi4py does not double-finalize. It checks first, so a script that uses finalize=True and simply never makes a collective afterwards exits cleanly. The failure mode is specifically "collective after teardown", exactly as you have written it.

I added more detail based on your comment. I am not so sure that I understand correctly. Please recheck it.

Rechecked, and the substance is correct — you have understood it. MPI_Finalize() really is called from the simulator's destructor rather than from step_cleanup(), and collectives really do still work immediately after step_cleanup(). I measured both: right after step_cleanup(), MPI.Is_finalized() returns False and an allgather succeeds; after the simulator object is destroyed, MPI.Is_finalized() returns True and the same allgather aborts the job.

One small framing point. The sentence says "in the example above it fires when main() returns and sim goes out of scope" — but the example above uses finalize=False, so in that example the teardown never fires at all. The sentence is describing what would happen with finalize=True, which is correct, but a careful reader will go back to the example, see finalize=False, and wonder which of the two is wrong.

Making the condition explicit fixes it:

I applied your suggestion

Confirmed — I ran the four-argument constructor on 2 ranks and it failed, while the same script on 1 rank succeeded, so the restriction really is specific to running on more than one rank.

Since the warning says what does not work but not what the user will see, I would add the error message so that someone who hits it can find this page by searching for it:

Using it on more than one rank aborts with Parallel simulator setup is incorrect as it does not use ParallelEclipseState.

For what it is worth, the mechanism is that the deck parsed in Python gives every rank a plain EclipseState, so readDeck.cpp skips substituting a ParallelEclipseState on the non-zero ranks (it only does so when the pointer is still null), and the grid distribution step later requires exactly that subclass. That also explains why the same constructor is fine in serial: the check returns early when there is only one rank.

I added the suggested sentence.

Added correctly, thank you — that is exactly the error a user will paste into a search box. Purely cosmetic: there is a double space after "rank." on that line.

This minor comment is now fixed.

Three consecutive blank lines here, where the rest of the file uses two.

The blank lines problem is solved

Small consistency point: this page introduces British spellings that the rest of the documentation does not use — initialises (line 33), initialised (line 46), behaviour (line 90) and initialise (line 104). I grepped the other .rst files under python/sphinx_docs/docs/ and did not find any, so initializes / initialized / behavior would match the surrounding pages.

Sorry for the inconsistence english. This problem is fixed.

I think this one is a false positive — please don't apply it. The file cited is a test fixture in this repository (python/sphinx_docs/tests/files/docstrings_simulators.json, last changed January 2025), not the binding. The binding in opm-simulators is py::arg("filename") (python/simulators/PyBlackOilSimulator.cpp), so BlackOilSimulator(filename=CASE) is correct as written. I checked against the built module: filename= is accepted, while deck_filename= fails with TypeError: __init__(): incompatible constructor arguments. The example also ran unchanged on 4 MPI ranks.

Same here — a false positive from the same test fixture, please keep setup_mpi. The binding is .def("setup_mpi", ...), and mpi_init does not exist on the class (hasattr(BlackOilSimulator, "mpi_init") is False). mpi_init was a stale name in the opm-simulators docstring file, and it was removed from the published API reference in OPM/opm-simulators#7439 yesterday. The fixture Copilot read is an older copy of that file that still has it.