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

Avoid initializing MPI in regression tests #320

Merged
merged 3 commits into from
Oct 13, 2020

Conversation

AndrewGaspar
Copy link
Contributor

@AndrewGaspar AndrewGaspar commented Oct 12, 2020

Addresses an issue with hygiene when importing the h5py module.

Prevent mpi4py from initializing MPI.

Fixes #312

PR Summary

Importing h5py causes future MPI operations in the process or child process to fail (probably due to unpaired MPI_Init) for some implementations of MPI, such as openmpi 4.0.2 on Darwin Power9. Therefore, this change runs restart.py's TestCase.Analyse script in a child process to maintain hygiene in the test process. This change stops the mpi4py transitive dependency from initializing MPI when present.

A possible alternative solution would be to import h5py in the scope of the Analyse function. My concern with this is that future mpiexec commands would fail, and I don't know if we're guaranteed to never have any more. This solution is more complete, but less pretty.

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md (N/A - test fix)

Addresses an issue with hygiene when importing the
h5py module. Importing h5py causes future MPI operations
in the process or child process to fail (probably due to
unpaired MPI_Init).

Fixes #312
@pgrete
Copy link
Collaborator

pgrete commented Oct 12, 2020

I'm not sure I ran across the same problem, but in a different context I "fixed" an issue by using the following initialization order:

import mpi4py
mpi4py.rc(initialize=False)
from mpi4py import MPI
import h5py
MPI.Init()

Would you mind testing that approach and see if this fixes it? (I don't have access to the system where it's nor working).

@AndrewGaspar
Copy link
Contributor Author

@pgrete I'm glad somebody else ran into this before. :) Let me see... the thing is, the MPI initialization seemed to be coming out of the loading of a native library in h5py, so I'm worried mpi4py isn't involved here...

@AndrewGaspar
Copy link
Contributor Author

That did work! I'll promote it to run_test.py.

I didn't seem to actually need to call MPI.Init, so I'm going to leave it out for now... It's possible we don't actually need to build h5py with mpi support? Probably shouldn't break if it was, though.

@Yurlungur
Copy link
Collaborator

That did work! I'll promote it to run_test.py.

I didn't seem to actually need to call MPI.Init, so I'm going to leave it out for now... It's possible we don't actually need to build h5py with mpi support? Probably shouldn't break if it was, though.

Just wrap the MPI import in a try-catch block, in case MPI4Py is not available.

I was going to suggest moving the script in the string into it's own file in scripts/python, since this comparison of two dump files is probably a fairly generic operation. We might as well genericize it and pull it out of the testing infrastructure. But looks like that's not needed.

@JoshuaSBrown
Copy link
Collaborator

JoshuaSBrown commented Oct 12, 2020

@AndrewGaspar Thanks so much for fixing this, @pgrete thanks for the suggestion. This is great! And @Yurlungur thanks for your suggestions!

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. 👍

Thanks for digging into this.

@AndrewGaspar AndrewGaspar changed the title Run Restart TestCase.Analyse in a subprocess Avoid importing MPI in regression tests Oct 12, 2020
@AndrewGaspar AndrewGaspar changed the title Avoid importing MPI in regression tests Avoid initializing MPI in regression tests Oct 13, 2020
@AndrewGaspar
Copy link
Contributor Author

@pgrete I assume you're good with this change then?

@JoshuaSBrown JoshuaSBrown added the bug Something isn't working label Oct 13, 2020
@AndrewGaspar
Copy link
Contributor Author

Phil signed off on the call.

@AndrewGaspar AndrewGaspar merged commit 5fdfa04 into develop Oct 13, 2020
@AndrewGaspar AndrewGaspar deleted the AndrewGaspar/h5py-import-fix branch October 13, 2020 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in mpi restart regression test/infrastructure
4 participants