Add a MPI layer, some bindings, CMake setup and MPI test framework

[pratikvn]

This PR aims to add the MPI base framework to Ginkgo in a experimental distributed-develop branch.

Partially closes #907

[upsj]

I think there are a few areas that need some more discussion before we can merge this, namely:

• ginkgo_mpi as a separate shared library probably won't work consistently, especially with the deep integration into solvers.
• declaring MPI functionality in headers with GKO_BUILD_MPI could lead to a lot of issues with binary incompatibility, I think we should consider including these files/their contents only conditionally. For an example, check distributed-matrix
• personally I prefer to have the wrappers available as function templates in the headers, since they can then be easily extended to custom types

[codecov]

Codecov Report

Merging #908 (37b8a44) into distributed-develop (2a34e0e) will decrease coverage by 1.08%.
The diff coverage is 90.00%.

@@                   Coverage Diff                   @@
##           distributed-develop     #908      +/-   ##
=======================================================
- Coverage                94.39%   93.30%   -1.09%     
=======================================================
  Files                      453      459       +6     
  Lines                    36907    37578     +671     
=======================================================
+ Hits                     34838    35063     +225     
- Misses                    2069     2515     +446     

Impacted Files	Coverage Δ
core/test/utils.hpp	100.00% <ø> (ø)
include/ginkgo/core/base/exception_helpers.hpp	90.90% <ø> (ø)
include/ginkgo/core/base/types.hpp	92.59% <ø> (ø)
third_party/gtest/gtest_mpi_listener.cpp	55.72% <55.72%> (ø)
include/ginkgo/core/base/mpi.hpp	92.39% <92.39%> (ø)
core/mpi/exception.cpp	100.00% <100.00%> (ø)
core/test/mpi/base/bindings.cpp	100.00% <100.00%> (ø)
core/test/mpi/base/communicator.cpp	100.00% <100.00%> (ø)
core/test/mpi/base/exception_helpers.cpp	100.00% <100.00%> (ø)
include/ginkgo/core/base/exception.hpp	100.00% <100.00%> (ø)
... and 9 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a34e0e...37b8a44. Read the comment docs.

[pratikvn]

@upsj, declaring files conditionally has some side effects:

1. You cannot have one code base compiled once, which allows you to switch between the cases when MPI is available or not. For example, gko::distributed::Matrix would not even be defined when MPI is off. I think this would cause more issues because you then have non-existent symbols instead of hooks like we do in Ginkgo currently. AFAIK, this need to write both MPI and non-MPI versions is a desired feature in many applications.
2. In some solvers, it might be necessary to switch between algorithms or do something else for MPI, which again would not be available because the symbols are no longer defined.

Can you elaborate on what you mean by possible binary incompatibilities ?

[pratikvn]

ginkgo_mpi as a separate shared library probably won't work consistently, especially with the deep integration into solvers.

I am not sure what else we can do here. I think putting the MPI symbols into ginkgo_core conditionally is not good either and I don't think we can put it anywhere else. What kind of problems do you foresee with ginkgo_mpi as a separate shared library ?

[pratikvn]

personally I prefer to have the wrappers available as function templates in the headers, since they can then be easily extended to custom types

With the current approach as well, the user facing wrappers are templated. This should allow for easy extension to custom types as well, with automatic mpi_type deduction and unwrapping of the data of custom types such as Arrays, maybe inside a specific instantiation.

Also, extension to custom types usually needs specific overloads for the reason that you usually don't need to specify counts, which you need to for the POD types, so the function signature needs to be different in my opinion.

[upsj]

@pratikvn yes, unfortunately I see no nice solution to this dilemma - distributed matrix and vector need MPI functionality, i.e. they need to store a MPI communicator, i.e. the type of MPI_Comm must be known in the headers.
If you were to declare MPI_Comm = int, sizeof(MPI_Comm) is wrong compared to an actual MPI implementation (and may even differ between implementations), which leads to subtle breaks when trying to pass a MPI-enabled type into a non MPI-enabled Ginkgo. That is a simple example for a ABI break that can easily occur in such a setting.
There are ways around this (pimpl idiom), but those also require some place where the actual MPI type can be used, so we don't really get clean separation.

If you compile Ginkgo without MPI, then the corresponding headers are empty thanks to the #ifdef, so you can't even compile against such a library if you try to use distributed parts of Ginkgo, no potential for missing symbols here.

If you tried to pass a distributed vector (which you got by hacking around in the types) to a non-distributed solver, you'd simply get a runtime error, since it expects Dense instead of distributed::Vector.

[upsj]

With the current approach as well, the user facing wrappers are templated. This should allow for easy extension to custom types as well, with automatic mpi_type deduction and unwrapping of the data of custom types such as Arrays, maybe inside a specific instantiation.

It sounds to me like you are talking about the function declarations here, but without a function definition available in the header, you as a user cannot instantiate the function for custom types, even if they would have nicely corresponding MPI types.
If we provide such wrapper declarations, but limit their definitions/instantiations to our internally used value and index types, how useful are they as part of the public interface then?

[upsj]

I am not sure what else we can do here. I think putting the MPI symbols into ginkgo_core conditionally is not good either and I don't think we can put it anywhere else. What kind of problems do you foresee with ginkgo_mpi as a separate shared library ?

The issue is at the interaction with solvers - we need three things: A dispatch that takes care of the distinction between Dense and distributed Vector, a unpacking function that returns the underlying local Dense vector for kernel calls, and a reduction function that can be used to allreduce the result of custom reductions like in CGS GMRES. All of them need distributed::Vector to be a complete type with known binary layout etc, so for the aforementioned reasons, we cannot simply provide a dummy distributed::Vector with the correct binary layout, because they can differ between MPI implementations.

So with fundamental parts of the distributed integration not being part of ginkgo_mpi, I am not sure what we would gain from this approach otherwise.

Don't get me wrong, it it quite possible that there is a clean solution to implement this, but so far I haven't seen anything close to it that doesn't require large amounts of boilerplate code, or complicates user interaction.

[pratikvn]

Maybe I need to clarify what I meant. We need to decide on a couple of things which will probably push us towards one decision or the other.

1. Do we allow users to write code such as this:

std::ifstream stream("fname");
auto data = gko::read_raw<>(stream);

auto mat = gko::matrix::Csr<>::create(exec); // single exec
auto dist_mat = gko::distributed::Matrix<Csr<>>::create(exec, comm); // distributed object

auto partition = gko::Partition::build(exec, ranges); // build partition

if(is_distributed()){ // Some way to say distributed functionality has been compiled for. 
    dist_mat->read_distributed(data, partition);
} else {
    mat->read(data);
}

If we do, then we need a dummy implementation. In mpi-base-dist-mat, we did provide the dummy symbols and it worked fine as far as I could see. It may be that there could have been issues we did not face yet. Of course, as you say there is some boiler plate involved, which we have for most of Ginkgo anyway.

This needs some input from @tcojean and @fritzgoebel as they have some experience/input from the OpenCARP side.

2. Can we completely separate the distributed and the non-distributed solvers and preconditioners ? For example, is there no place we would have the same core, but a branch to select between a distributed and non-distributed algorithm ?

// Some core algorithm
// Do common stuff with LinOp's (both non-distributed and distributed have that as base type)

if(is_distributed()){
// Call MPI functions or work on distributed objects with dynamic_casts.
} else {
// Do single executor stuff.
}

// Do common stuff with LinOp's (both non-distributed and distributed have that as base type)

In other words, can we guarantee that the core algorithm will be the same for non-distributed and distributed for all current and future algorithms. I think this would be a pretty strict restriction.

Both of the above, will also work with #ifdef but will require re-compilation every time either is required. If a dummy distributed is provided, then when MPI is off, only non-distributed will work, but the distributed code still compiles. With MPI switched on, you have the choice to run either non-distributed or distributed without re-compiling.

[tcojean]

I'm not in a position yet to give much feedback on all those issues, but I think the #ifdef way might be fine as well. They would probably anyway need to recompile themselves for enabling or not MPI in their own code after all (there shouldn't be much use case where Ginkgo is the only distributed code in the stack?). But the questions of how much code changes are required from standard Ginkgo to Ginkgo distributed, if distributed/non distributed Ginkgo code can live together from a user perspective are important ones.

[pratikvn]

To collect the summary of our discussions and decisions:

1. We go with having no distributed functionality when MPI is off. The distributed files (MPI bindings, distributed classes) are all not compiled when MPI is off.
   a. We try to unify the core algorithms as much as possible (for most solvers that we use this should be straightforward), for preconditioners in case we need different execution paths, we duplicate the code and enable conditional compilation.
2. Distributed functionality (distributed::Matrix<> and distributed::Vector and their functions) will be within the ginkgo_core library. To separate MPI bindings and functions, a separate library, ginkgo_mpi will be added which only contains wrappers for MPI functionality. This would allow us to see MPI as a backend of sorts, and keep separation between core library (ginkgo related functionality) and external functionality (through wrappers).
3. To allow for wrappers for MPI functions to work with user-defined custom types, move wrappers to a public headers with templated defintions.

@upsj and @MarcelKoch, please feel free to correct me if I am missing something.

[MarcelKoch]

1. We go with having no distributed functionality when MPI is off. The distributed files (MPI bindings, distributed classes) are all _**not compiled**_ when MPI is off.

Just to clarify, the executor specific kernels of some distributed classes/functions will still be compiled into the corresponding device library regardless of the MPI status, since these kernels do not require MPI in any way.

[MarcelKoch]

LGTM, thanks for adding all of this, especially the CI. I have some mostly smaller remarks, below. Larger issues like using value semantics for most mpi wrapper classes or using fully templated functions can be addressed at a later point.

[MarcelKoch]

One slight issue with using value semantics, is that it requires a call to MPI_COMM_DUP, which is a collective operation, so the overhead would probably increase.

[Slaedr]

LGTM! Nice job with the communicator wrapper. I noticed one mistake in the tests, and there are other relatively minor comments.

[upsj]

The communicator implementation looks good if I look at the owning case, the non-owning case and its interaction with owning communicators still has a few rough edges to iron out. They are mainly visible in the assignment operators, but also to a lesser degree in the constructors:

Copy-assignment a = b

	a owning	a non-owning
b owning	duplicate	duplicate
b non-owning	duplicate?	duplicate?

Move-assignment a = std::move(b) should never duplicate, but should b be empty afterwards, if it was non-owning? Should a be turned from owning to non-owning by this operation?

[upsj]

Pratik and I discussed this behavior conundrum, and following the design of Boost.MPI, came to the conclusion that it makes sense to take care of conditional ownership + duplication only at the application-Ginkgo boundary, i.e. the communicator constructor, and store the communicator in a shared_ptr with a custom deleter to avoid unnecessary duplication if we have many distributed objects in a custom deleter.

[tcojean]

What are the implications of this? What I understand from this is that you would then:

• Decide owning/non-owning only at construction time (basically, Ginkgo's comms are owning, MPI non owning)
• Do not change the owning/non-owning property on copy/move, maybe even remove that capacity.

Is there something I misunderstood?

[upsj]

Yes, basically this means that there are two states for the shared_ptr<MPI_Comm> internally, ones with a plain deleter (non-owning) that just deletes the MPI_Comm pointer, and ones with a deleter that calls MPI_Comm_free and deletes the pointer afterwards. This state is kept during copy construction and assignment (move construction and assignment are disabled/handled by copy construction and assignment)

[upsj]

LGTM! Just a few remaining small nits:

• Formatting: We usually use single empty line between member functions, that's a bit inconsistent in the file right now
• Caching rank/size/local rank inside the communicator
• is_gpu_aware is vendor-specific, since we can now compile CUDA and ROCm simultaneously, maybe DPC++ in the future, as well?
• you removed the request type, I think that could still be really useful with appropriate member functions .wait() and .test() and maybe static member functions wait_all() and test_all()?

[upsj]

Just a note, not necessarily something we need to change: This throws/aborts inside a destructor, which may not be ideal. MPI_Comm_free probably also checks against MPI_COMM_NULL (considering how eager the MPI impls I checked are to abort on slight issues)

Suggested change

	GKO_ASSERT(*comm != MPI_COMM_NULL);
	GKO_ASSERT_NO_MPI_ERRORS(MPI_Comm_free(comm));
	if (MPI_Comm_free(comm) != MPI_SUCCESS) {
	// something like in CudaExecutor::raw_free
	}

[MarcelKoch]

LGTM, thanks for the excellent work. I have only a minor remark on the constness of the member functions. Nearly every member function of communicator and window can be marked const. The underlying MPI object is always copied, and I think very few calls are documented to change the MPI object.

Also, I second @upsj suggestion to add a request type, to really make it consistent.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information