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

Support unit tests with MPI #413

Closed
BerengerBerthoul opened this issue Aug 17, 2020 · 8 comments
Closed

Support unit tests with MPI #413

BerengerBerthoul opened this issue Aug 17, 2020 · 8 comments

Comments

@BerengerBerthoul
Copy link
Contributor

Hello, thanks for this very useful and convenient library!

I work in the field of computational fluid mechanics and particularly on HPC topics. In our field, it means we have distributed memory over multiple processors, communicating with the MPI standard.

Thus we need a way to unit-test mpi-distributed code. For that, we developped (Bruno Maugars and myself) a doctest extension that copes with distributed tests. The extension is working well and we are currently using it for two projects at ONERA, our company.

The extension is pretty small (<500 loc) and non-intrusive to doctest.h (in the sense that it does not change its sources). I forked the doctest repository on github (branch mpi_tests). The documentation is here.

If you are interested, we would like to have this functionnality integrated to doctest. It seems useful to have, and even if MPI uses are marginal, I think that there is still a fair number of people using it. The MPI standard is used by almost everyone doing HPC-related developments, and it will stay that way for still many years. To our knowledge, no unit test framework supports mpi-distributed memory well.

Regarding the packaging, I don't know how it should be done. My guess is that the sources for MPI should be somehow optional. For now I put the sources in doctest/mpi/ and the examples in examples/mpi/. Regarding CMake, I just copy-pasted-edited the all_features one as a proof-of-concept solution (hence ctest is not working since it doesn't know it should run the mpi tests with the mpirun command). Personnally, I ran the example test by hand with cd $BUILD_DIR/examples/mpi && mpirun -np 3 ./mpi_features.

The build process was done with CMake 3.12 and gcc/8.3 with C++17 (use of [[maybe_unused]] to quickly remove the unused-parameter -Werror).

I am ready to discuss the technical details, but the first question is: are you interested in supporting distributed-memory unit tests ?

@onqtam
Copy link
Member

onqtam commented Sep 4, 2020

Hello, and sorry for the really late reply on my side - I'll take a look at what work has accumulated around doctest in the following days/weeks.

My initial thoughts: Yes, I'd be happy to include this extension in the repository, given that you'll be the maintainers of it (since I probably won't be using it anytime soon), but I assume that's assumed. One little feedback I can give right now: I've been planning on introducing a folder called extensions next to the doctest.h header and my plans were to name the extension headers doctest_<something>.h so for mpi it would be doctest_mpi.h.

I'll take a deeper look and provide more feedback soon. Thanks again for the interest in upstreaming this :)

@BerengerBerthoul
Copy link
Contributor Author

Great!

given that you'll be the maintainers of it

Yes we can do that. One naive question though: the implementation relies on the private doctest API. Does it also include updating the extension when the private API changes?

One little feedback I can give right now: I've been planning on introducing a folder called extensions next to the doctest.h header and my plans were to name the extension headers doctest_.h so for mpi it would be doctest_mpi.h.

That would be perfect. I can adapt the fork to reflect that, or I can leave it to you for later.

I'll take a deeper look and provide more feedback soon

Is it premature to submit a merge request? The idea is not to merge immediately or anything, but it may be simpler to iterate on that, especially if you want to refactor on the fly (I'm pretty sure that things could be written more elegantly with more knowledge of the framework)

@onqtam
Copy link
Member

onqtam commented Sep 6, 2020

given that you'll be the maintainers of it

Yes we can do that. One naive question though: the implementation relies on the private doctest API. Does it also include updating the extension when the private API changes?

Well I think even the private API won't be changing much anytime soon, but if it does - I'll take a look at all affected extensions and either correct them or raise the alarm.

One little feedback I can give right now: I've been planning on introducing a folder called extensions next to the doctest.h header and my plans were to name the extension headers doctest_.h so for mpi it would be doctest_mpi.h.

That would be perfect. I can adapt the fork to reflect that, or I can leave it to you for later.

In the fork would be petter I think.

I'll take a deeper look and provide more feedback soon

Is it premature to submit a merge request? The idea is not to merge immediately or anything, but it may be simpler to iterate on that, especially if you want to refactor on the fly (I'm pretty sure that things could be written more elegantly with more knowledge of the framework)

A PR would definitely be preferable! :)

I'm also thinking of a single extensions.md file for documenting all extensions, where each will have a heading of their own, and would think about splitting that only if it gets too big, but I don't think that would be anytime soon - I want to keep the number of documentation files small.

@onqtam
Copy link
Member

onqtam commented Sep 7, 2020

just pushed a commit with the first extension header (albeit pretty meaningless): af25c1a

@BerengerBerthoul
Copy link
Contributor Author

Associated pull request : #418

@onqtam
Copy link
Member

onqtam commented Nov 4, 2020

Just commented on the merged PR about this: #418 (comment)
Will be closing this issue - for any further work feel free to open new issues/PRs.
Thanks again for the contribution!

@onqtam onqtam closed this as completed Nov 4, 2020
@philbucher
Copy link
Member

@onqtam will this be part of the next release?

@onqtam
Copy link
Member

onqtam commented Nov 4, 2020

@philbucher Yes, it will :)

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

No branches or pull requests

4 participants
@onqtam @philbucher @BerengerBerthoul and others