-
Notifications
You must be signed in to change notification settings - Fork 912
add support for request_get_status_any/all/some #13279
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
base: main
Are you sure you want to change the base?
add support for request_get_status_any/all/some #13279
Conversation
d233c73
to
723b84e
Compare
Let me know if you want help with the python binding infrastructure. |
@hppritcha yes, I would appreciate some help on that front (and the same applies for the fortran bindings, even if its just a conversation that I understand what needs to be done). |
efd9cf5
to
f21dff7
Compare
@edgargabriel added new data type to the bindings code to add this new use of |
I'll work on the fortran but that will have to wait till later in the week. |
note that mpi4py is probably not testing this since it keys off our MPI_Get_version return values for major/minor. |
@hppritcha thank you very much, I will test it later this week. I have not done the mpi4py test that you mentioned, will look into this as well. |
a2aa8f9
to
81e3492
Compare
faking MPI 4.1. for mpi4py doesn't work unfortunately, we fail the compilation of mpi4py due to other missing function (MPI_Buffer_flush etc.). I will write some tests for the new functions. |
@edgargabriel i pushed the remainder of the fortran related files to the PR - hopefully. |
@hppritcha thank you! I hope to finish up adding tests to the ibm testsuite for the new function this weekend, and hopefully next week we could merge the PR if all tests pass. |
I added some tests for request_get_status_any/all/some to the ompi-tests private testsuite, and they pass. This is however only testing the C-interfaces of the function. The tests are also probably not entirely bullet-proof in terms of that they might miss some corner cases, but the base function is shown to be working. |
360c797
to
47fbc39
Compare
Signed-off-by: Edgar Gabriel <[email protected]>
add the man pages for the newly implemented MPI_Request_get_status_all/any/some functions. Signed-off-by: Edgar Gabriel <[email protected]>
Three new functions were added to the MPI API as part of the 4.1 standard. These used an array of const MPI_Request s which the bindings code didn't support. This commit also fixes back the mpi.h prototypes to include the constants and required changes to the new template files. Signed-off-by: Howard Pritchard <[email protected]>
also switch MPI_Request_get_status to use new method for generating f08 bindings. Update fortran bindings interfaces generation code. f90/f77 interfaces will be added as another commit. Signed-off-by: Howard Pritchard <[email protected]>
for new fortran methods starting with MPI_Request_get_status_(any/all/some) plus switch MPI_Request_get_status to use the binding infrastructure as well. Signed-off-by: Howard Pritchard <[email protected]>
Signed-off-by: Howard Pritchard <[email protected]>
Signed-off-by: Howard Pritchard <[email protected]>
Incorporate the comments received during the review of the PR. Signed-off-by: Edgar Gabriel <[email protected]>
Signed-off-by: Edgar Gabriel <[email protected]>
2963f48
to
f326157
Compare
did we add the tests for this to ompi-tests? i'm triaging compile failures for the tests in my mtt runs:
|
@hppritcha I added these tests to the ibm testsuite https://github.com/open-mpi/ompi-tests/pull/189 |
It was probably too early to merge it, should have waited until we merge the actual code. Sorry, I didn't think it through :-( |
related to #13279 |
We only support named types with this first pass. related to open-mpi#12076 Fortran interfaces will be added once Fortran infrastructure additions in PR open-mpi#13279 are merged in to main. Signed-off-by: Howard Pritchard <[email protected]>
Signed-off-by: Howard Pritchard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Can we squash these down (don't have to squash to 1 commit -- just get rid of the "fixup" commits)? Then let's merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some major issues with this PR. Please check my comments.
@@ -2227,6 +2227,15 @@ OMPI_DECLSPEC MPI_Request MPI_Request_f2c(MPI_Fint request); | |||
OMPI_DECLSPEC int MPI_Request_free(MPI_Request *request); | |||
OMPI_DECLSPEC int MPI_Request_get_status(MPI_Request request, int *flag, | |||
MPI_Status *status); | |||
/* should be 'const MPI_Request array_of_requests[]' */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok ? It actually is marked as const
!
#include "ompi/request/grequest.h" | ||
#include "ompi/memchecker.h" | ||
|
||
/* Non blocking test for the request status. Upon completion, the request will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not technically a test
but a non blocking status retrieval.
** MPI functions invoking this functionality does not have the const | ||
** argument | ||
*/ | ||
if(!ompi_request_check_same_instance((MPI_Request *)requests, count) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing ompi_request_check_same_instance
to use const make sense as part of this PR because it is the first time it is useful to have const in the API instead of casting everywhere.
(requests[i]->req_complete) ) { | ||
continue; | ||
} | ||
all_done = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are plenty if incorrect indentations (mix between tab and spaces). Please fix before merge.
OMPI_COPY_STATUS(&statuses[i], ompi_status_empty, false); | ||
} | ||
} | ||
if (requests[i]->req_complete ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this test necessary ? The requirement to get to this code is that all requests are completed, which means that if they are not NULL or inactive they must be completed.
} | ||
|
||
if (all_inactive) { | ||
*flag = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indentation.
Signed-off-by: Howard Pritchard <[email protected]>
Add support for the new
MPI_Request_get_status_any/all/some
functions introduced with MPI 4.1This PR contains the C implementation for the functions as well as the man-pages.
There are however also some things missing:
const MPI_Request[]
, (I have currently removed theconst
from the function definitions in mpi.h.in in order to make the code compile).