Skip to content

Conversation

@jsquyres
Copy link
Member

Ensure to #include <mpi.h> at the top of mpi-ext.h, just so that it includes files that it depends on.

Thanks to @eschnett for raising the issue.

Refs #12111

Copy link
Contributor

@devreal devreal left a comment

Choose a reason for hiding this comment

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

LGTM

@bosilca
Copy link
Member

bosilca commented Nov 23, 2023

I understand the argument of self-sufficiency, it makes sense in general but not here. These are extensions, they shall not exists without the rest of MPI, so hiding the mpi.h header makes it less clear. At the end I think I would be more in favor of the approach taken by other implementations, and merge the extensions directly into mpi.h

@devreal
Copy link
Contributor

devreal commented Nov 23, 2023

@bosilca You are saying that this:

#include <mpi.h>
#include <mpi-ext.h> 

works but this:

#include <mpi-ext.h> // fails today because some symbols from mpi.h are not available yet
#include <mpi.h>

must not work because dependencies are not yet fulfilled? This should at least be properly documented as that is utterly confusing and goes against most people's intuition (a precompiler warning would be helpful then).

@bosilca
Copy link
Member

bosilca commented Nov 23, 2023

Ideally I would like to be able to support both, but that is not possible without splitting mpi.h, a lot of efforts for mostly nothing. Thus we have 3 choices:

  1. include mpi.h into mpi-ext.h, which would allow both but will also allow just mpi-ext.h
  2. require mpi.h before mpi-ext.h, aka. current situation for the last two stable cycles
  3. merge mpi-ext.h into mpi.h and deprecate mpi-ext.h. This will bring OMPI in sync with other MPI, and will still support what we had for the last two stable cycles, until we deprecate it in 6.0

@jsquyres
Copy link
Member Author

@bosilca Can you expound on your statement "This will bring OMPI in sync with other MPI"? I.e., are you saying that MPICH has their own extensions in their mpi.h?

I know that the original design intent for Open MPI's mpi-ext.h was to reinforce the idea to users who don't follow the details of the MPI Forum and standards development that the various OMPI_* and MPIX_* APIs are not part of the official MPI API. E.g., it really should be:

#include <mpi.h>
#if defined(OPEN_MPI) and OPEN_MPI
#include <mpi-ext.h>
#endif

void foo(void)
{
#if defined(OPEN_MPI) and OPEN_MPI
    MPIX_Some_open_mpi_specific_API(...);
#endif
}

I think @bosilca enumerated the options clearly; we could choose to do something different than we have in the past, if we want to.

If we stay the course, however, (i.e., keep our extensions in a must-be-included-separately mpi-ext.h), I'm not opposed to having mpi-ext.h automatically include mpi.h (obviously, since I filed this PR) on the argument that a header file should include what it requires. But I can also see the point that if we really want to be pure to the concept of "emphasize that these APIs are not part of the standard", then requiring a little extra work by the user to use the extensions is not a bad thing. In that case, I agree that putting in some kind of preprocessor warning if the #include order is incorrect would be beneficial. Perhaps something like:

// At the top of mpi-ext.h
#if !defined(OPEN_MPI) or !OPEN_MPI
#error You must include Open MPI's <mpi.h> before including <mpi-ext.h>
#endif

@bosilca
Copy link
Member

bosilca commented Nov 24, 2023

Yes, mpich's MPIX_* namespace is part of the mpi.h.

@wenduwan
Copy link
Contributor

@jsquyres @bosilca What is the plan for this PR? I can merge it as-is.

@bosilca
Copy link
Member

bosilca commented Mar 22, 2024

Taking in account the very limited number of extensions we provide it would make sense to use the same approach as MPICH and include the extensions in our mpi.h header.

@bosilca bosilca closed this Mar 22, 2024
@bosilca
Copy link
Member

bosilca commented Mar 22, 2024

wrong button, I really didn't mean to close it.

@bosilca bosilca reopened this Mar 22, 2024
Ensure to #include <mpi.h> at the top of mpi-ext.h, just so that it
includes files that it depends on.

Thanks to Erik Schnetter for raising the issue.

Signed-off-by: Jeff Squyres <[email protected]>
@jsquyres jsquyres force-pushed the pr/make-mpi-ext-h-include-mpi-h branch from 25f1656 to 06b77d4 Compare October 8, 2024 16:12
@jsquyres
Copy link
Member Author

jsquyres commented Oct 8, 2024

Re-noticed / remembered this still-open PR in Oct 2024. Rebased / freshened up this PR to current tip of main.

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