Draft kokkos-comm initialization/finalization using `MPI_Session`s by dssgabriel · Pull Request #68 · kokkos/kokkos-comm

dssgabriel · 2024-05-23T14:40:18Z

This PR is a first attempt at initialization and finalization for kokkos-comm relying on MPI_Sessions.

Some noteworthy additions brought by this PR:

a Communicator class that wraps the MPI_Comm and a Kokkos execution space
a Universe class that holds the handle to the MPI sessions, as well as a session-associated communicator.

I expect lots of changes in the API, which I find kind of clunky at the moment. Reviews and comments on how to improve are welcome.

…zation

devreal · 2024-05-23T15:04:24Z

src/KokkosComm_communicator.hpp

+        comm_kind = CommunicatorKind::User;
+      } else {
+        fprintf(stderr, "[KokkosComm] error: intercommunicators are not supported (yet).\n");
+        std::terminate();


Don't just terminate. Either throw an exception or call MPI_Abort (I've seen applications hang where one process aborted without calling MPI_Abort, it's nasty)

Any specific error code you suggest?

Not really, anything non-zero would work. This ties in with #29 so anything you do here is probably temporary anyway :)

devreal · 2024-05-23T15:05:32Z

src/KokkosComm_communicator.hpp

+
+  static auto dup(const Communicator &other) -> Communicator { return Communicator::dup_raw(other.as_raw()); }
+
+  static auto split_raw(MPI_Comm raw, Color color, Key key) -> Communicator {


Is there a reason why this isn't just an overload of split?

I wanted to make it explicit that we're splitting from a raw MPI_Comm handle, not a wrapped KokkosComm::Communicator, but I guess we can also simply overload.

I guess that's a design decision that the Kokkos community should make (to be consistent with the other projects)

You can drop the Raw. We typically are happy with overloads.

devreal · 2024-05-23T15:07:33Z

src/KokkosComm_communicator.hpp

+  inline static auto self(void) -> Communicator { return Communicator::from_raw_unchecked(MPI_COMM_SELF); }
+
+  inline static auto world(void) -> Communicator { return Communicator::from_raw_unchecked(MPI_COMM_WORLD); }


That doesn't belong into the communicator. Since we use sessions, there should be no use of the world process model. It could even be that no one has called MPI_Init.

devreal · 2024-05-23T15:08:19Z

src/impl/KokkosComm_MPI_instance.hpp

+}
+
+template <KokkosExecutionSpace ExecSpace>
+auto initialize(int &argc, char *argv[]) -> Universe<ExecSpace> {


It's not obvious that once version of initialize uses the WPM and one uses the SPM. I'd rather stay away from the WPM tbh.

I'll remove the one using the WPM so that we only initialize using the SPM.

src/KokkosComm_communicator.hpp

dssgabriel · 2024-05-23T15:49:35Z

Thank you very much for the first review, @devreal!

Regarding C++ destruction and the SPM finalization semantics: is MPI_Comm_free required to be called before MPI_Session_finalize? And if so, how can I destroy the communicator without having to call its dtor explicitly?

… cleanup

devreal · 2024-05-23T19:02:13Z

Just a PSA: OMPI in combination with UCX won't support Sessions until the next major release of OMPI (6.0): open-mpi/ompi#12566 (comment)

dssgabriel · 2024-05-24T15:39:04Z

It looks like Linux's OpenMPI is too old to support MPI_Sessions (which the initial support for was added in OpenMP 5.0), hence the failing CI tests.

@cwpearson Is there any way we can specifically install OpenMPI 5.x in the CI? The package from Ubuntu repos is simply not up-to-date.

devreal · 2024-05-24T16:04:58Z

Regarding C++ destruction and the SPM finalization semantics: is MPI_Comm_free required to be called before MPI_Session_finalize? And if so, how can I destroy the communicator without having to call its dtor explicitly?

The standard says that the application is required to clean up its objects before MPI_Session_finalize:

The call to MPI_SESSION_FINALIZE does not free objects created by MPI calls; these
objects are freed using MPI_XXX_FREE, MPI_COMM_DISCONNECT, or
MPI_FILE_CLOSE calls.
Once MPI_SESSION_FINALIZE returns, no MPI procedure may be called in the Sessions
Model that are related to this session (not even freeing objects that are derived from
this session), except for those listed in Section 11.4.1.

We had a similar issue in other projects and came down to a registry in which objects that needed destruction were referenced. There are two options they can get destroyed:

Their destructor is called before the Session is destroyed. Then the object just removes itself from the registry.
They are released as part of the destruction of the session object (before MPI_Session_finalize is called) and the object becomes an empty shell, whose destructor does not do anything.

All of this must be thread-safe etc but communicators are heavy objects that are not created and destroyed regularly so I think the overhead involved in such a scheme is acceptable.

janciesko · 2024-10-09T20:24:03Z

Would you like to rebase this on top of current develop?
@devreal, what's the status of sessions in some common MPI implementations? Can we drop this in or rather put it in an Experimental namespace?

devreal · 2024-10-09T20:33:43Z

@devreal, what's the status of sessions in some common MPI implementations? Can we drop this in or rather put it in an Experimental namespace?

Open MPI's session support has not been released yet. It will be included in the next major release (tbd when that comes out)

dssgabriel and others added 15 commits May 16, 2024 00:25

feat: add Universe class to handle KokkosComm initialization/finali…

5614211

…zation

feat: add support for buffered mode send operations

45c695f

Merge branch 'kokkos:develop' into initialize-finalize

ea667db

fix: correctly detach buffer and fix size check

bc31289

feat: first draft of Communicator class

4852d9c

feat(session): implement a first draft of session-based init

e37173b

Merge branch 'kokkos:develop' into initialize-finalize

aac626e

feat: initialization and finalization routines for KokkosComm

4bc55e2

test: send_recv with KokkosComm init and finalize

afbb017

Merge branch 'kokkos:develop' into initialize-finalize

89e3ee3

feat(sessions): move init & finalize w/ MPI_Session to its own file

dcc4584

revert: removed Buffered communication mode

de176b6

feat(sessions): removed dead code in Communicator class

bb58138

test(sessions): update tests with new init/finalize API

c2812bd

chore: format

4ceee5f

dssgabriel marked this pull request as draft May 23, 2024 14:40

devreal suggested changes May 23, 2024

View reviewed changes

dssgabriel added 3 commits May 23, 2024 18:37

fix(sessions): remove code related to MPI's WPM

40ba7e7

refactor(sessions): rename as Context, remove initialize w/ WPM &…

822722d

… cleanup

test(sessions): update test accordingly

8faf205

dssgabriel added 2 commits May 24, 2024 13:36

chore: format

d064469

fix(sessions): only free communicator if it isn't already null

f0d5285

dssgabriel mentioned this pull request Jun 12, 2024

Provide a KokkosCommInitialize #82

Open

cedricchevalier19 mentioned this pull request Jun 12, 2024

Create containers for CI #86

Open

dssgabriel mentioned this pull request Jun 14, 2024

Provide KokkosComm::initialize/finalize #88

Closed

2 tasks


		static auto dup(const Communicator &other) -> Communicator { return Communicator::dup_raw(other.as_raw()); }

		static auto split_raw(MPI_Comm raw, Color color, Key key) -> Communicator {

		inline static auto self(void) -> Communicator { return Communicator::from_raw_unchecked(MPI_COMM_SELF); }

		inline static auto world(void) -> Communicator { return Communicator::from_raw_unchecked(MPI_COMM_WORLD); }

Conversation

dssgabriel commented May 23, 2024

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!

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!

Uh oh!

dssgabriel commented May 23, 2024

Uh oh!

devreal commented May 23, 2024

Uh oh!

dssgabriel commented May 24, 2024

Uh oh!

devreal commented May 24, 2024

Uh oh!

janciesko commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devreal commented Oct 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

janciesko commented Oct 9, 2024 •

edited

Loading