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: refactor mpi-io layer #7232

Merged
merged 6 commits into from
Jan 9, 2025
Merged

romio: refactor mpi-io layer #7232

merged 6 commits into from
Jan 9, 2025

Conversation

hzhou
Copy link
Contributor

@hzhou hzhou commented Dec 6, 2024

Pull Request Description

This PR is a split from #7228. It should not change any of the ROMIO behaviors.

Refactor to split interface and implementation. This prepares for
binding generations.

The binding layer, e.g. open.c, close.c, etc. will be replaced by python
generation when ROMIO is building FROM_MPICH.

Author Checklist

  • Provide Description
    Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
  • Commits Follow Good Practice
    Commits are self-contained and do not do two things at once.
    Commit message is of the form: module: short description
    Commit message explains what's in the commit.
  • Passes All Tests
    Whitespace checker. Warnings test. Additional tests via comments.
  • Contribution Agreement
    For non-Argonne authors, check contribution agreement.
    If necessary, request an explicit comment from your companies PR approval manager.

@hzhou
Copy link
Contributor Author

hzhou commented Dec 6, 2024

test:mpich/ch3/most
test:mpich/ch4/most

@hzhou
Copy link
Contributor Author

hzhou commented Dec 6, 2024

test:mpich/ch4/ofi

@hzhou
Copy link
Contributor Author

hzhou commented Dec 26, 2024

test:mpich/ch4/ofi

Copy link
Contributor

@raffenet raffenet left a comment

Choose a reason for hiding this comment

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

@roblatham00 do you want to take a look at all? It would be good to get these merged early so any issues can be raised in the coming months.

hzhou added 6 commits January 9, 2025 10:25
These looks like vendor-specific code. We can easily generate them if
needed. Remove them for now to simplify refactor.
All romio error check macros takes a myname parameter (which can be
replaced by __func__). Make MPIO_CHECK_INFO_ALL consistent.
Refactor to split interface and implementation. This prepares for
binding generations.

The binding layer, e.g. open.c, close.c, etc. will be replaced by python
generation when ROMIO is building FROM_MPICH.
Add impl functions MPI_File_f2c and MPI_File_c2f for consistency and to
avoid exposing MPIOI functions. They will be used when we generate IO
bindings from MPICH and are not used if we build ROMIO outside MPICH.
Use "goto fn_fail" instead of "goto fn_exit".

Skip MPIO_Err_return_file since we will do that in top-level routines.
We call MPI_File_delete in case there is a leftover testfile, which will
interfere with later MPI_File_open. Not checking error return and call
"delete" on a seemingly non-existent file is myterious. Add a comment
and error checking for maintenance.
@hzhou hzhou merged commit d3d0ce7 into pmodels:main Jan 9, 2025
4 checks passed
@hzhou hzhou deleted the 2412_romio_impl branch January 9, 2025 16:33
@dalcinl
Copy link
Contributor

dalcinl commented Jan 23, 2025

@hzhou @raffenet 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

@dalcinl
Copy link
Contributor

dalcinl commented Jan 23, 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?

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 23, 2025

@hzhou @raffenet 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

Sure. Will fix.

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.

3 participants