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

Use mpirun when testing MPI #1255

Merged
merged 2 commits into from Nov 26, 2019
Merged

Use mpirun when testing MPI #1255

merged 2 commits into from Nov 26, 2019

Conversation

aragilar
Copy link
Member

This provides more freedom to specify HDF5 locations (which I needed for testing the MPI builds), and runs the tests under MPI (apart from those known to not work, e.g. those using subprocess). I fixed the existing MPI tests, and added some docs about how to test with MPI.

This depends on https://pypi.org/project/pytest-mpi/, which I created because it seems that the conftest.py must the at the root level for cli args to work (see https://stackoverflow.com/questions/52447767/using-pytest-addoptions-in-a-non-root-conftest-py), I'm happy to move it under the h5py org and/or give people access (on github, pypi and other services) if people think that's appropriate (I plan on adding docs and tests, happy to restructure it also, I basically used an existing pytest plugin I wrote as a template).

Before this gets merged, can people run this on their systems (and if report any lockups, the tests should run very fast, so if you're waiting a while, there's probably a problem).

#1206 will need to be rebased on this (let me know if there are more features we should expose with pytest-mpi).

@takluyver
Copy link
Member

I don't have time to look at this in detail right now, but this sounds excellent - having a decent testing infrastructure for MPI will make things like #1206 much simpler to address.

For the plugin: I think it's fine to add it to the h5py org if you'd like to maintain it collaboratively. But we can also depend on your project if you'd rather keep it separate.

@codecov
Copy link

codecov bot commented Sep 19, 2019

Codecov Report

Merging #1255 into master will decrease coverage by 0.71%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1255      +/-   ##
==========================================
- Coverage   84.95%   84.24%   -0.72%     
==========================================
  Files          17       17              
  Lines        2107     2107              
==========================================
- Hits         1790     1775      -15     
- Misses        317      332      +15
Impacted Files Coverage Δ
h5py/_hl/files.py 91.26% <0%> (-4.37%) ⬇️
h5py/_hl/dataset.py 87.23% <0%> (-1.26%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eede146...c71ca9d. Read the comment docs.

@aragilar aragilar force-pushed the mpi-ci-fixes branch 2 times, most recently from 0be6d69 to 0fe744f Compare September 19, 2019 14:07
@aragilar aragilar added this to In progress in h5py code camp Sep 20, 2019
@aragilar aragilar force-pushed the mpi-ci-fixes branch 2 times, most recently from 5861a69 to 94f6a1b Compare September 24, 2019 13:46
@aragilar aragilar force-pushed the mpi-ci-fixes branch 2 times, most recently from be1d460 to 5769e39 Compare October 6, 2019 03:14
@aragilar
Copy link
Member Author

aragilar commented Oct 6, 2019

I've switched back to using the system HDF5 (on azure now, rather than travis). I won't do any more experimentation on this branch, I'll leave getting a newly-built version of HDF5 to a different PR/branch.

@aragilar aragilar force-pushed the mpi-ci-fixes branch 2 times, most recently from 4439ff3 to dfb23ed Compare October 11, 2019 03:35
docs/contributing.rst Outdated Show resolved Hide resolved
setup_configure.py Outdated Show resolved Hide resolved
@takluyver
Copy link
Member

It looks like the tests crashed immediately while trying to import h5py, but then sat until they timed out after an hour. Before making the tests work, can we ensure that it stops correctly on failure?

Running the MPI tests with coverage sometimes causes the tests to fail
(as it appears coverage is writing to the same file twice), skip it for
now, until we can tell coverage to use multiple files.
@aragilar
Copy link
Member Author

I've dropped the CI changes from this, so hopefully the tests now pass (leaving the current MPI tests on travis). I'll look at expanding the CI factors the MPI tests run on in a different PR.

@takluyver takluyver added this to the 3.0 milestone Nov 26, 2019
@takluyver
Copy link
Member

The test changes here look good to me; thanks.

@takluyver takluyver merged commit 0756395 into h5py:master Nov 26, 2019
boegel added a commit to boegel/h5py that referenced this pull request May 11, 2020
@aragilar aragilar deleted the mpi-ci-fixes branch August 1, 2020 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
h5py code camp
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants