Skip to content
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

Initial MPI unit tests implementation #418

Merged
merged 4 commits into from Oct 29, 2020
Merged

Conversation

BerengerBerthoul
Copy link
Contributor

Description

Adds an mpi extension for testing mpi-distributed unit tests

GitHub Issues

Implements the feature described in #413

Co-authored-by: Bruno Maugars <Bruno.Maugars@onera.fr>
@BerengerBerthoul
Copy link
Contributor Author

Please feel free to refactor / criticize the development. Some points to note :

  • there is a TODO item in the .md documentation that is here for information but should probably be removed / moved somewhere else
  • I changed some lines in the top-level CMakeLists to work with the extension, but it could be cleaned
  • There is an example in examples/mpi. I placed it here but it should probably go in another extensions folder (and then the documentation should be updated). I copy/pasted the all_features CMakeLists because I didn't not know how to do this properly, but obviouly it should be replaced by something else

@BerengerBerthoul
Copy link
Contributor Author

Ah yes : the tests are failing because the new MPI test build requires, logically, the MPI package, which seems to not be present by default on the CI machine. I guess there is three possible correction to this problem:

  • Either we don't use CI for extensions ("use at your own perils'), at least for now
  • Always include MPI for the doctest CI
  • Only try to load MPI for the doctest_mpi CI

@onqtam
Copy link
Member

onqtam commented Sep 11, 2020

I'll try to review this whenever possible

@philbucher
Copy link
Member

Awesome feature, looking forward to this!

Regarding the Github CI you could install OpenMPI on the fly like I do here

@BerengerBerthoul
Copy link
Contributor Author

The MPI package has been made optionnal for regular tests. The solution of @philbucher allows to have mpi, but then the mpi test fails because the doctest test executable must be launched with "mpirun -np 3", and we don't know how to do it. But I think @onqtam will be able to fix it easily. Anyway, rather than failing the CI because of this missing "mpirun -np 3", the openmpi installation in the ci file are there but commented out

@philbucher
Copy link
Member

from my experience running (Open)MPI in the CI:

  • don't run it as root, OpenMPI doesn't like this and might abort
  • newer versions of OpenMPI check the available resources. If using more processes than what is available then you need the --oversubscribe option, e.g. like this: mpiexec --oversubscribe -np 3 python3 script.py

@onqtam
Copy link
Member

onqtam commented Oct 12, 2020

I should mention that I definitely haven't forgotten about this - I'll be leaving my company pretty soon and will devote some time to doctest by the end of this month.

@BerengerBerthoul
Copy link
Contributor Author

Ok take your time ;) If it can help, we could set up a quick video call to present the why, how and what remains to be done.

@onqtam onqtam merged commit 8aeb798 into doctest:dev Oct 29, 2020
onqtam added a commit that referenced this pull request Nov 4, 2020
@onqtam
Copy link
Member

onqtam commented Nov 4, 2020

Sorry for the late reply. I merged it last week and just pushed a few small fixes - I played with it a bit locally and overall it looks good. I won't be messing with the TODO roadmap and have not tried to get openmpi installed on the CI but I did fix up a bit the CMakeLists.txt for the mpi test.

There's 1 other suggestion I have in mind: perhaps we should embed mpi_reporter.h into doctest_mpi.h since it's not meant to be used from anywhere else anyway...?

Thanks again for the contribution - future responses will be swifter!

@BerengerBerthoul
Copy link
Contributor Author

Great! Thanks for the merge and the CmakeLists.txt cleaning. I will try to test the CI with openmpi on my fork and submit a new PR once it is done (+ minor corrections/update in the doc)
Regarding mpi_reporter.h, it is up to you. There is no need to have two files, I just found it cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants