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

pFUnit and MPI tests #26

Closed
quantheory opened this issue Jun 16, 2015 · 3 comments
Closed

pFUnit and MPI tests #26

quantheory opened this issue Jun 16, 2015 · 3 comments

Comments

@quantheory
Copy link
Member

I know that @jedwards4b is working on adding share code that uses MPI with associated tests, but I don't know if he's hitting the same issue as I am.

The immediate issues, which we already have on master, are problems with the driver_cpl tests added by @billsacks. Right now, each test calls mpi_init and mpi_finalize during the setUp/tearDown subroutines. However, the MPI standard does not require the MPI runtime to be restartable, and in fact most implementations are not. That means that you only get to call mpi_init and mpi_finalize once each during a process's lifetime. It just so happens that this works for mpi-serial right now (and probably in no other case?). pFUnit typically solves this by just calling mpi_init/finalize itself, so that you don't have to. But unless we build it against mpi-serial, it won't know to do this for serial tests.

There's also the fact that the unit tests currently build and link with mpi-serial unconditionally; if you're using mpif90, there could be problems with trying to link against two different MPI implementations.

I'm not 100% sure what to do about this. One option is to always require MPI to run the tests. (I think Bill is OK with this.) Another is to have some option in CMakeLists to handle mpi-serial differently from a "real" MPI implementation. A third is to use pFUnit built against mpi-serial for serial tests (but I don't know if that will work right now, and it would mean that there would have to be a system build of mpi-serial that may be a different version from the one in CIME).

@billsacks
Copy link
Member

I put this note in mpi_wrapper_mod.F90:

! NOTE(wjs, 2015-01-16) Currently this has been set up with serial unit tests in mind -
! using a serial build of pFUnit and the mpi-serial library (although this probably also
! works with a true mpi library). More thought will be needed about how to make this
! work with either serial or parallel pFUnit tests. In particular: I think parallel
! pFUnit tests would do their own mpi_init and mpi_finalize calls, so it would be an
! error for unit tests to make these calls themselves. That explains the motivation for
! setting up this module: When the time comes to introduce that generality, I didn't
! want to have to add logic in every test module regarding whether to call mpi_init and
! mpi_finalize: instead, they can safely call these wrappers, which will do the right
! thing.

I guess I was wrong about the parenthetical in the first sentence, but the general comment holds.

In terms of a way forward, then, I would suggest one of these options... mostly echoing / elaborating on what @quantheory suggested:

(1) Always require a parallel build of pFUnit, with a true mpi library. Then we could do away with mpi_wrapper_mod, and no longer bother with building mpi-serial when building the unit tests. In my opinion, this is probably the best way forward, unless there are downsides that I'm not aware of.

(2) Add logic in CMake about whether we should build and link against mpi-serial... this would be based on whether we're using a serial or parallel build of pFUnit. Also add some logic in mpi_wrapper_mod that only does the mpi_init & mpi_finalize calls if we're using a serial pFUnit. e.g., we could do this with an ifdef, with some CPP token defined by cmake. This sort of thing was my original vision in creating mpi_wrapper_mod - i.e., so it could have some logic like this (I just hadn't figured out quite what that logic should look like).

@quantheory
Copy link
Member Author

For now, I've gone with option (1), and I've added a commit to #25 for that. (Maybe it should be its own PR, but it has the same theme, namely getting driver_cpl to build on more systems.)

The only downside is that you do need an MPI implementation to run the CIME unit tests with this change, which hopefully is not that big a deal. It shouldn't be that hard to build MPICH even on a laptop and get it running.

@quantheory
Copy link
Member Author

Due to recent merges, including #87, I'm going to close this. There are some rough edges here, but I think that we can add them as individual issues, rather than this broader one.

jedwards4b pushed a commit to jedwards4b/cime that referenced this issue Jan 15, 2016
Added ifdefs around the MPIAlltoall call in the case of OPENMPI which…
gold2718 pushed a commit to gold2718/cime that referenced this issue Oct 19, 2023
…esm2_1_rel_06-Nor

fixing write statement for output log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants