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

mpi: add new status_set/get functions #13125

Merged

Conversation

edgargabriel
Copy link
Member

MPI4.1 introduced a few new MPI_Status_get/set functions. This PR is the C API implementations of the functions.

Fixes issue #12073

Note: this is just the C-API, I need potentially help for the Fortran API of these functions.

@jsquyres
Copy link
Member

jsquyres commented Mar 5, 2025

We should probably just add the Fortran bindings in #12226

@edgargabriel edgargabriel force-pushed the topic/mpi4.1-status-set-get-funcs branch from 89977a8 to e61248d Compare March 5, 2025 15:47
@edgargabriel
Copy link
Member Author

edgargabriel commented Mar 5, 2025

@jsquyres do we need to also add man-pages / documentation for these functions?

MPI4.1 introduced a few new MPI_Status_get/set functions.
This PR is the C API implementations of the functions.

Signed-off-by: Edgar Gabriel <Edgar.Gabriel@amd.com>
@edgargabriel edgargabriel force-pushed the topic/mpi4.1-status-set-get-funcs branch 2 times, most recently from 27321d0 to 40e0021 Compare March 6, 2025 10:29
@edgargabriel
Copy link
Member Author

@jsquyres do we need to also add man-pages / documentation for these functions?

Added a second commit to this PR which adds the man-pages for the new functions.

@jsquyres
Copy link
Member

jsquyres commented Mar 6, 2025

@jsquyres do we need to also add man-pages / documentation for these functions?

Yes, please. We have man pages for all public MPI API calls..

Copy link
Member

@bosilca bosilca left a comment

Choose a reason for hiding this comment

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

I left comments for 2 of the man pages, but the same issues exists in all the others.


INPUT/OUTPUT PARAMETER
----------------------
* ``status``: Status with which to retrieve error (status).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* ``status``: Status with which to retrieve error (status).
* ``status``: Status from which to retrieve the error (status).

INTEGER, OPTIONAL, INTENT(OUT) :: ierror


INPUT/OUTPUT PARAMETER
Copy link
Member

Choose a reason for hiding this comment

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

Why is the status inout ? It should be read only for get.


USE mpi_f08
MPI_Status_get_error(status, error, ierror)
TYPE(MPI_Status), INTENT(INOUT) :: status
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TYPE(MPI_Status), INTENT(INOUT) :: status
TYPE(MPI_Status), INTENT(IN) :: status

Copy link
Member Author

@edgargabriel edgargabriel Mar 7, 2025

Choose a reason for hiding this comment

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

@bosilca thank you, yes, the INOUT is correct for the _set_error function, but not for the _get_error. I've updated all _set functions to apply this change

INTEGER, OPTIONAL, INTENT(OUT) :: ierror


INPUT/OUTPUT PARAMETER
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
INPUT/OUTPUT PARAMETER
INPUTPARAMETER

Copy link
Member Author

@edgargabriel edgargabriel Mar 7, 2025

Choose a reason for hiding this comment

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

same comment as above (i.e. difference between _set and _get functions)

DESCRIPTION
-----------

Returns in error the MPI_ERROR field in the status object.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Returns in error the MPI_ERROR field in the status object.
Returns in error the MPI_ERROR field from the status object.

Copy link
Member Author

Choose a reason for hiding this comment

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

also same as above. "from" has to be used for the _get, functions, "in" for the _set functions.

INTEGER, OPTIONAL, INTENT(OUT) :: ierror


INPUT/OUTPUT PARAMETER
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
INPUT/OUTPUT PARAMETER
INPUTPARAMETER


USE mpi_f08
MPI_Status_get_source(status, source, ierror)
TYPE(MPI_Status), INTENT(INOUT) :: status
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TYPE(MPI_Status), INTENT(INOUT) :: status
TYPE(MPI_Status), INTENT(IN) :: status

@edgargabriel edgargabriel force-pushed the topic/mpi4.1-status-set-get-funcs branch from 40e0021 to b726671 Compare March 7, 2025 08:43
Signed-off-by: Edgar Gabriel <Edgar.Gabriel@amd.com>
@edgargabriel edgargabriel force-pushed the topic/mpi4.1-status-set-get-funcs branch from b726671 to 2608b02 Compare March 7, 2025 08:49
@bosilca
Copy link
Member

bosilca commented Mar 7, 2025

I'm really curious how a user is supposed to properly use these new API and obtain a valid status ? Are they supposed to set it to zero before calling any of these functions ? Why would that be valid ?

@edgargabriel
Copy link
Member Author

@bosilca, thank you first of all for your reviews, you caught a number of issues!

I am not entirely sure to be honest, my understanding is that the functions are intended to improve the support of MPI from other languages. (In fact, despite of having attended basically all MPI meetings for 4.1, I don't have a good recollection on these functions, which I find surprising). I just tried to tick off another box for the MPI 4.1. compliance by implementing these functions.

@edgargabriel edgargabriel merged commit 3d4c7d4 into open-mpi:main Mar 8, 2025
15 checks passed
@edgargabriel edgargabriel deleted the topic/mpi4.1-status-set-get-funcs branch March 8, 2025 13:42
@devreal
Copy link
Contributor

devreal commented Mar 9, 2025

I'm really curious how a user is supposed to properly use these new API and obtain a valid status ? Are they supposed to set it to zero before calling any of these functions ? Why would that be valid ?

These functions are not used to get a status. They are meant to extract the fields of a status received from test/wait in languages that cannot deal directly with C struct fields (like Python).

@bosilca
Copy link
Member

bosilca commented Mar 9, 2025

That was absolutely not my question. My question was about the set part of the API.

@devreal
Copy link
Contributor

devreal commented Mar 9, 2025

There is no such thing as a "valid status" in MPI. Different operations touch different fields and leave the rest untouched (source/target/tag for non-persistent recv, error for test/wait_[some,all] in case of an error. The setter functions are relevant for generalized requests so the application sets whatever fields it needs and leaves the rest undefined.

@bosilca
Copy link
Member

bosilca commented Mar 9, 2025

Of course there is a valid status, most operations start from the empty status and fill it up. It is well defined for all types of operations. But here we allow libraries to manipulate statuses without enforcing them to start from a sane value. Basically the only way to use such a status is to force the user is to always know exactly what fields have been set and never access any other fields from the status. Quite a high bar.

@devreal
Copy link
Contributor

devreal commented Mar 10, 2025

If you can point me to where the standard says we start from an empty status everywhere I'd be grateful. I spent time last year dissecting what the different request types yield as status and it is mixed. In general, it is up to the chapter to define what values are defined. Some chapters are explicit about it (RMA, IO, P2P) while others don't say anything. The empty status is only used for NULL requests.

image image

OMPI might start from an empty status but that's not relevant for these functions.

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

Successfully merging this pull request may close these issues.

4 participants