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

romio: mpi4py need ROMIO_VERSION macro #7278

Closed
hzhou opened this issue Jan 24, 2025 · 11 comments · Fixed by #7276
Closed

romio: mpi4py need ROMIO_VERSION macro #7278

hzhou opened this issue Jan 24, 2025 · 11 comments · Fixed by #7276

Comments

@hzhou
Copy link
Contributor

hzhou commented Jan 24, 2025

As a consequence of the changes in this PR, the inclusion of mpi.h no longer defines the macro ROMIO_VERSION. mpi4py uses this macro at compile time to know whether MPICH was or was not configured with MPI I/O support.
https://github.com/mpi4py/mpi4py/blob/master/src/lib-mpi/config/mpich.h#L14

All previously published versions of mpi4py will fail to support mpi4py's File class and its methods. Therefore, for the purposes of backward compatibility, would you consider reinstating the ROMIO_VERSION definition?. This issue could potentially affect other codes relying on the macro to detect whether MPICH builds have MPI I/O enabled or not.
Example: https://gitlab.kitware.com/vtk/vtk/-/blob/master/IO/MPIImage/vtkMPIImageReader.cxx#L18

Originally posted by @dalcinl in #7232 (comment)

@hzhou
Copy link
Contributor Author

hzhou commented Jan 24, 2025

Also - #7232 (comment)

On a related note, maybe the files mpio.h and mpio.h should be emptied (leaving #include "mpi.h") or directly removed? What's the point of keeping it? If it is kept, does it make sense to get it installed with the all the MPI-IO API definitions and declarations?

PS: I think the following may be a leftover, looks like the define is not used anywhere:

$ git grep HAVE_ROMIOCONF_H 
src/mpi/romio/configure.ac:CFLAGS="$CFLAGS -DHAVE_ROMIOCONF_H"

@hzhou
Copy link
Contributor Author

hzhou commented Jan 24, 2025

On a related note, maybe the files mpio.h and mpio.h should be emptied (leaving #include "mpi.h") or directly removed? What's the point of keeping it? If it is kept, does it make sense to get it installed with the all the MPI-IO API definitions and declarations?

ROMIO may be used by Open MPI or build stand-alone, which may need mpio.h for legacy reasons.

@hzhou
Copy link
Contributor Author

hzhou commented Jan 24, 2025

tag @dalcinl

@dalcinl
Copy link
Contributor

dalcinl commented Jan 24, 2025

ROMIO may be used by Open MPI or build stand-alone, which may need mpio.h for legacy reasons.

But then, do MPICH really need to install a fully populated mpio.h? Moreover, anyone previously including both mpi.h and mpio.h things would have worked. But after your changes, I think users now will get duplicated MPI I/O function declarations in the translation unit. I don't have any strong feelings, just trying to point out where things could go wrong.

In short, what I'm arguing is to keep mpio.h as it is now as an internal header in the ROMIO directory, but install in $prefix/include a trivial mpio.h with basically just #include "mpi.h".

@hzhou
Copy link
Contributor Author

hzhou commented Jan 24, 2025

But then, do MPICH really need to install a fully populated mpio.h?

No. We can skip installing mpio.h if ROMIO is building FROM_MPICH.

@dalcinl
Copy link
Contributor

dalcinl commented Jan 24, 2025

BTW, as a reminder: if --enable-romio survived, then the #define ROMIO_VERSION ... macro should be defined only if ROMIO is built.

@dalcinl
Copy link
Contributor

dalcinl commented Jan 28, 2025

@hzhou @raffenet I'm wondering whether preserving the --enable/disable-romio option to configure makes sense.
If you --disable-romio, then we get the MPI I/O APIs in mpi.h but not in libmpi.so.
This could easily confuse/break third-party configure tests using try-compile instead of try-link, and IMHO that's bad.

@hzhou
Copy link
Contributor Author

hzhou commented Jan 28, 2025

The --disable-romio option is for developers who want to cut off some compile time during build cycles. We do not recommend 3rd party to use that option and do not officially support it . MPI-IO is part of MPI standard and users should expect it to be available if an implementation claim a full support. I think it is a reasonable expectation for mpi4py to not support implementation that have broken MPI-IO. It is also within your choice if you want to use it to save some of your own development cycle time.

@dalcinl
Copy link
Contributor

dalcinl commented Jan 28, 2025

The --disable-romio option is for developers

OK, fair enough. Maybe a note could be displayed in configure --help. Otherwise regular users may think it is something worth caring about and passing explicitly, just as we had to deal with in conda-forge/mpich-feedstock#111.

@hzhou
Copy link
Contributor Author

hzhou commented Jan 28, 2025

The --disable-romio option is for developers

OK, fair enough. Maybe a note could be displayed in configure --help. Otherwise regular users may think it is something worth caring about and passing explicitly, just as we had to deal with in conda-forge/mpich-feedstock#111.

Will do.

In general, most options should be left to default unless one knows exactly the reasons for specific choice. Most of the configure options are for experimental features or to shortcut or overwrite the configure logic. Sometime we do suggest certain configure option as temporary workaround, but the goal is always to fix the underlying issue and enhance the configure logic to make the work around unnecessary at some point. Thus, if the distribution is manually setting certain configure option, it should take up the responsibility of keeping watch between upgrades when the option no-longer necessary.

@dalcinl
Copy link
Contributor

dalcinl commented Jan 28, 2025

I don't know autotools enough, but... Would be easy to add a help section Developer options (not recommended for generic users) and move there a bunch of stuff like the ROMIO option? Anyway, this is very low priority. I just wanted to point out that users may get confused.

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 a pull request may close this issue.

2 participants