[python,parallel] improve compatibility of Dune::Communication<...> and mpi4py

• add MPI_Comm wrapper
• add conversion from mpi4py.MPI.Comm to MPI_Comm wrapper
• allow to construct dune.common.Communication from mpi4py.MPI.Comm
• additional tests
• possibly move to a submodule parallel (discussion postponed)

@andreas.dedner this follows your suggestion

I'm currently wondering

1. we already have dune.common.comm which should basically be the same as your suggested dune.common.defaultCommunicator
2. I'd rather move things to dune.common.parallel to mimic the C++ structure and add a clearer separation

feedback would be welcomed

added discussion label

This MR should be the basis for dune-grid!696 (merged)

changed the description

added 1 commit

• 08b44223 - add missing CMakeLists.txt entry for new python file

Compare with previous version

added 1 commit

• 0a3d6bc1 - [python,parallel] add additional test for Communication bindings

Compare with previous version

marked the checklist item additional tests as completed

@andreas.dedner what do you think about a submodule dune.common.parallel?

Besides this question, we should discuss:

1. dune.common.defaultCommunicator: I believe we can drop it, as this is mostly the same as dune.common.comm.
2. dune.common.withMPI: I added this, as it is not straight forward to extract the information, whether or not we are working with MPI. This would imho read better with the parallel submodule, but in general the question is, whether we want to export this information at all.

Once these questions are sorted out, this MR should be ready to merge.

mentioned in commit dune-grid@0c4d5696

mentioned in merge request dune-grid!696 (merged)

added 1 commit

• 3e927c6b - cleanup

Compare with previous version

added 1 commit

• f8246a46 - [python,parallel] add support for No_Comm

Compare with previous version

added 5 commits

• f07cdf7c - [python,parallel] improve compatibility of Dune::Communication<...> and mpi4py
• d0491e32 - add missing CMakeLists.txt entry for new python file
• d39b7c7b - [python,parallel] add additional test for Communication bindings
• 6361b863 - cleanup
• afd68328 - [python,parallel] add support for No_Comm

Compare with previous version

added 1 commit

• f3260bbd - fixup test

Compare with previous version

added 3 commits

• b2782f4d - [python,parallel] improve compatibility of Dune::Communication<...> and mpi4py
• 9326a5e9 - [python,parallel] add support for No_Comm
• 2c3639b2 - [python,parallel] add additional test for Communication bindings

Compare with previous version

marked this merge request as ready

From a functional point of view this MR is now now ready to merge.

One question would be whether or not to move the parallel infrastructure to dune.common.parallel. IMHO this would be preferable, but I guess there might be other opinions. In any case ... if we want to introduce such a move, we can still do this in an other MR.

assigned to @andreas.dedner

changed the description

resolved all threads

@christi: I'm much in favor of this. But I would also like to have a MPI_Clone (MPI_Comm_dup) in mpihelper.hh to avoid clash with the standard MPI_COMM_WORLD, if this fits into this MR.

resolved all threads

merged

mentioned in commit 4eee0b3a

This introduces a new dependency on mpi4py in the C++ part. HAVE_MPI true doesn’t mean that the header mpi4py.hh is available. There should be separate CMake stuff for that I think. At last the Dumux CI breaks with this change.

As far as @andreas.dedner explained it to me, the python bindings have hard dependency on mpi4py if MPI is used. This code is only part of the python bindings, so I don't see how we can make this optional, but perhaps there is something that needs to be fixed in the handling of python dependencies?

@tkoch @andreas.dedner @samuel.burbulla can you python gurus please explain how to handle this properly. We have a dependency, but apparently this is now checked at a suitable place and then breaks dumux some strange way. Either we have to adapt our pipelines such that we catch similar errors early, or we have to state clearly how to use dune. In any case it is a problem that I'm currently not able to solve as I don't have sufficient understanding of the whole dune/python/venv magic and I currently don't have sufficient time to dig myself into this.

See discussion in !1235 (merged). I think there needs to be a CMake feature check to catch this dependency.

As far as @andreas.dedner explained it to me, the python bindings have hard dependency on mpi4py if MPI is used.

This is currently only checked at runtime. In order to make it a hard dependency at compile time there needs to be a feature check at configure time.

@christi Concerning the CI: I think this is currently very impracticable and makes Dune difficult to maintain, but there is a secret Repo on Github which runs different Python setups. There you also see that a lot of configurations failed with this MR merged:

https://github.com/dune-project/dune-testpypi/actions/runs/4288731090

Basically, this means no changes affecting Python (and that maybe means no changes) can be merged anymore just based on the Dune CI alone. But in addition, one has to manually trigger an action in GitHub.

I don't know how it got to this double CI situation. I ran in the same issue a week ago. Everything passes in the Dune pipelines, I merge, but it turns out the additional tests in dune-testpypi failed. Reverted. Not really a good developer situation right now...

IMHO this means that our current python setup is so broken (or over-engineered) that people like me can't contribute.

mentioned in commit 5b178b08

mentioned in merge request !1235 (merged)

mentioned in merge request dune-grid!699 (merged)