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

[RFC] Wrap and test the non-blocking collective functions #143

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Apr 30, 2016

I lied and wrapped Allreduce AS WELL AS Iallreduce too.

I hope the tests and function names are ok. Would love any feedback!

@codecov-io
Copy link

codecov-io commented Apr 30, 2016

Current coverage is 86.12%

Merging #143 into master will decrease coverage by -0.27%

@@             master       #143   diff @@
==========================================
  Files             3          3          
  Lines           567        663    +96   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            490        571    +81   
- Misses           77         92    +15   
  Partials          0          0          
  1. File src/mpi-base.jl was modified. more
    • Misses 0
    • Partials 0
    • Hits -1

Powered by Codecov. Last updated by c850f40...6694b44

@kshyatt
Copy link
Contributor Author

kshyatt commented Apr 30, 2016

Is this just an old openmpi version we're installing on Linux, or is something more seriously wrong?

@coveralls
Copy link

coveralls commented Apr 30, 2016

Coverage Status

Coverage decreased (-0.3%) to 86.103% when pulling e29e0ee on ksh/icollective into 5d24cc9 on master.

@andreasnoack
Copy link
Member

OpenMPI 1.6.x is old and broken. The joy of Linux package systems. See #144

@kshyatt
Copy link
Contributor Author

kshyatt commented May 3, 2016

OK now passing with newer OpenMPI. Thoughts?

@coveralls
Copy link

coveralls commented May 3, 2016

Coverage Status

Coverage decreased (-0.3%) to 86.124% when pulling e3f68c8 on ksh/icollective into c850f40 on master.

@andreasnoack
Copy link
Member

@davidanthoff Do you know what is happening on Windows here?

@davidanthoff
Copy link
Contributor

Try to add MPI_IALLTOALL, MPI_IEXSCAN, MPI_ISCAN and MPI_IALLTOALLV to src/win_mpiconstants.jl

@kshyatt
Copy link
Contributor Author

kshyatt commented May 6, 2016

But I did add IEXSCAN at least...

@davidanthoff
Copy link
Contributor

Actually, sorry for my comment, I think you actually added all of these already... I just looked at the appveyor output, not at your commits. I'll try this locally and will report back.

@davidanthoff
Copy link
Contributor

I think MS MPI just doesn't support some of these functions at this point. E.g. MPI_IALLTOALL is simply not implemented. This has a list of the non-blocking collective operations that are implemented, and MPI_IALLTOALL is not one of them.

Probably best to put all the stuff that is not supported on Windows in a @unix block, or something?

MPI_IALLTOALL, MPI_IALLTOALLV, MPI_IEXSCAN and MPI_ISCAN are all not supported on MS MPI. There might be more, but those were the ones I saw error messages for, and those are not present in the MS MPI header files.

@eschnett
Copy link
Contributor

eschnett commented May 6, 2016

Instead of using @windows, can we somehow detect which version of the MPI standard the MPI library implements? The non-blocking collective operations are all MPI 3, and since this standard is only 4 years old, I assume that many HPC systems won't have it available either. Detecting this probably needs to be done via cmake.

@davidanthoff
Copy link
Contributor

The fact that the README for MS MPI just listed the functions that are implemented manually makes me kind of suspicious that this is directly linked with the set of functions for a specific MPI version... But if there is a way to get at this info in a more automated way, it would certainly be good.

Having said that, the windows part of this doesn't use cmake at all, it is all manually coded...

@kshyatt
Copy link
Contributor Author

kshyatt commented May 10, 2016

Is there more to be done here? Should we shelf this until we find a nice way to detect MPI versions?

@lcw
Copy link
Member

lcw commented May 10, 2016

Can you just call MPI_Get_version at runtime to get the version?

@eschnett
Copy link
Contributor

It should be simpler to check whether the function exists in the dynamic library.

@lcw
Copy link
Member

lcw commented May 10, 2016

Yea, that could work as well.

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

7 participants