-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide KokkosComm::initialize
/finalize
#88
Changes from all commits
6167b91
e055c01
d098a72
0baa4ca
5c5ed7e
8987ce5
a68902e
4868c87
f3c838c
cfdd01e
ee700c6
cfefa4e
d3b00de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
|
||
#pragma once | ||
|
||
#include "KokkosComm_include_mpi.hpp" | ||
#include "KokkosComm_collective.hpp" | ||
#include "KokkosComm_version.hpp" | ||
#include "KokkosComm_isend.hpp" | ||
|
@@ -28,8 +29,69 @@ | |
|
||
#include <Kokkos_Core.hpp> | ||
|
||
#include <algorithm> | ||
#include <cstdio> | ||
#include <string_view> | ||
|
||
namespace KokkosComm { | ||
|
||
enum class ThreadSupportLevel { | ||
Single = MPI_THREAD_SINGLE, | ||
Funneled = MPI_THREAD_FUNNELED, | ||
Serialized = MPI_THREAD_SERIALIZED, | ||
Multiple = MPI_THREAD_MULTIPLE, | ||
}; | ||
|
||
inline void initialize(int &argc, char *argv[], ThreadSupportLevel required = ThreadSupportLevel::Multiple) { | ||
int flag; | ||
MPI_Initialized(&flag); | ||
// Forbid calling this function if MPI has already been initialized | ||
if (0 != flag) { | ||
MPI_Abort(MPI_COMM_WORLD, -1); | ||
} | ||
|
||
// Forbid calling this function if Kokkos has already been initialized | ||
if (Kokkos::is_initialized()) { | ||
std::abort(); | ||
} | ||
Comment on lines
+53
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need this, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I understand. We want to abort if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because Kokkos will do that for you, I believe. I'm not sure MPI will. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it weird that a subproject subsumes the initialization of the main project. I should be able to initialize main Kokkos and KokkosComm from my application. The KokkosComm initializer should initialize main Kokkos if it's not already initialized and otherwise handle that gracefully. Same with MPI above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand your words about "handle that gracefully". Kokkos initialization will abort if it has been initialized/finalized previously: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Are you sure about this? I only see detection of the local MPI rank based on environment variables, but not calls to any MPI routines. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kokkos docs mention one should call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You are telling people not to initialize your project if they initialize its dependencies manually. That is flawed.
I am not arguing to take that away. If users want to shift all initialization to KokkosComm they can do so.
Why is that fundamentally wrong? You can still bail if they did it wrong but there is no reason to always bail even if the configuration they chose is correct (e.g., MPI was initialized with the same thread-level.)
Then let's not tell them one thing now and another then. We can get close to the semantics we will have then and users will not have to change their code. Keep in mind that what you are proposing now forces users to change their code such that later when KokkosComm uses Sessions the WPM will be broken and no communication outside of KokkosComm will work anymore. Or KokkosComm will be broken because users stuck to manually initializing MPI to get the WPM. We can do better than that.
The approach I would use is an inline accessor to avoid explicit global variables that need to go into a compilation unit. In some header file: inline bool& get_state_ref() {
static bool state = false;
return state;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR aimed at unifying two calls —
It means that instead of having two calls, users may now need a third one.
OK, that's fair.
Once #68 gets merged, existing code will most likely have to change anyway. E.g. all of our tests use
I don't think we need to carry any state for the moment, we're only calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The function you're looking for is
I don't see the problem, esp if parts of my application rely on the WPM anyway and I have to ensure that the WPM is initialized. I cannot rely on KokkosComm to do that if I know that at some point the behavior will change. Anyway, maybe this PR isn't ready yet and needs to be tied to #68. Let's discuss this during the Monday call, I will try to join (the MPI Forum potentially conflicts with the call). |
||
|
||
int provided; | ||
MPI_Init_thread(&argc, &argv, static_cast<int>(required), &provided); | ||
// Abort if MPI failed to provide the required thread support level | ||
if (static_cast<int>(required) < provided) { | ||
MPI_Abort(MPI_COMM_WORLD, -1); | ||
} | ||
|
||
int rank; | ||
MPI_Comm_rank(MPI_COMM_WORLD, &rank); | ||
// Strip "--help" and "--kokkos-help" from the flags passed to Kokkos if we are not on rank 0 to prevent Kokkos | ||
// from printing the help message multiple times. | ||
if (0 != rank) { | ||
if (auto *help_it = std::find_if(argv, argv + argc, [](std::string_view const &x) { return x == "--kokkos-help"; }); | ||
help_it != argv + argc) { | ||
std::swap(*help_it, *(argv + argc - 1)); | ||
--argc; | ||
} | ||
} | ||
Kokkos::initialize(argc, argv); | ||
} | ||
|
||
inline void finalize() { | ||
// Forbid calling this function if Kokkos has already been finalized or isn't yet initialized | ||
if (Kokkos::is_finalized() || !Kokkos::is_initialized()) { | ||
MPI_Abort(MPI_COMM_WORLD, -1); | ||
} | ||
Comment on lines
+80
to
+83
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't need it, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same but for finalization. Also, this lets us cleanly abort on all ranks. |
||
Kokkos::finalize(); | ||
|
||
int flag; | ||
MPI_Finalized(&flag); | ||
// Forbid calling this function if MPI has already been finalized | ||
if (0 != flag) { | ||
std::abort(); | ||
} | ||
MPI_Finalize(); | ||
} | ||
|
||
template <CommMode SendMode = CommMode::Default, KokkosExecutionSpace ExecSpace, KokkosView SendView> | ||
Req isend(const ExecSpace &space, const SendView &sv, int dest, int tag, MPI_Comm comm) { | ||
return Impl::isend<SendMode>(space, sv, dest, tag, comm); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not allow MPI to be initialized already as long as the requested thread-level matches?