Add support to use mpi_f08 MPI module #523
Conversation
The changes were made by Dom Heinzeller <dom.heinzeller@noaa.gov>
DusanJovic-NOAA
requested review from
climbfuji,
grantfirl,
gold2718 and
mkavulich
as code owners
There was a problem hiding this comment.
It would really help if you would open an issue before coding so we can discuss the idea (see workflow at https://github.com/NCAR/ccpp-framework/wiki/Development-Workflow).
For instance, what is the plan to align this work with the CCPP Standard Naming conventions (see https://github.com/ESCOMP/CCPPStandardNames/blob/main/Metadata-standard-names.md) which still say that an mpi_communicator is an integer?
Note that there is an existing issue for capgen (#390), however, that will require a different solution than presented here (it only comes into play when schemes have an MPI communicator as a dummy argument).
Separate question, does this proposed solution work if a scheme uses mpi and the host model uses mpi_f08 or vice versa?
|
FWIW: I ran all capgen and prebuild unit tests that are in the main ccpp-framework branch, and they all passed. |
|
Both host model and physics scheme must use the same type of mpi communicator as subroutine argument (integer or MPI_Comm). In this case we changed ccpp physics dummy arguments and corresponding actual arguments in fv3atm. |
|
For reference, the ccpp-physics PR (to the ufs/dev branch) is here: ufs-community/ccpp-physics#160 I think @gold2718's question about interoperability Looking at the changes in the ccpp-physics PR, I see a lot of really nice cleanup that has to do with calls to NCEPLIBS utilities - these are as far as I understand independent of the @gold2718 My pragmatic approach would be to implement that in a simplified way in An interesting idea could be to task the CCPP framework with always exposing a MPI_comm DDT, which is either integer or the f08 mpi_comm type, depending on how it is configured. This way the metadata can always use MPI_comm. We'd have to think about using ifdefs for the @dustinswales @peverwhee @mwaxmonsky @nusbaume @mkavulich @grantfirl |
|
Apologies if this is already known but these are resources that were helpful for me when looking at
|
|
@DusanJovic-NOAA @climbfuji @gold2718 |
|
In fv3atm we will (if/when this set of PR is merged) switch to just using mpi_f08. It is supported on all platforms we currently support fv3atm. In terms of ccpp-framework/physics if there's a need to support both f77 and f08 interfaces, as Dom explained, some kind of build/preprocessing macro can be added to choose one or the other at build time. |
|
@dustinswales @gold2718 @grantfirl @DusanJovic-NOAA How do we make progress here? I understand that this PR is holding up a set of PRs that would be good to merge. I also understand that the ccpp-framework changes should have been communicated/proposed a little earlier, but I am hoping we can come to an agreement on how to implement this in Should I convert this PR back to draft and propose a solution that we can then iterate on by updating it (that's what draft PRs are for)? |
@DomHeinzeller What is the recommended solution? Any solution that will allow us to compile ccpp using mpi_f08 module is acceptable. |
|
@climbfuji I will see the documentation is updated. |
…a now that mpi_f08 is a mandatory dependency
Update branch no_arg_mismatch
CMakeLists.txt
Outdated
| @@ -10,6 +10,13 @@ set(PACKAGE "ccpp-framework") | |||
| set(AUTHORS "Dom Heinzeller" "Grant Firl" "Mike Kavulich" "Steve Goldhaber") | |||
| string(TIMESTAMP YEAR "%Y") | |||
|
|
|||
| #------------------------------------------------------------------------------ | |||
| # Set MPI flags for C/C++/Fortran with MPI F08 interface | |||
| find_package(MPI REQUIRED C Fortran) | |||
There was a problem hiding this comment.
We need to check if this works - in my experience, if you as for MPI REQUIRED C Fortran you also need to define LANGUAGES C Fortran in line 5. Better would be to remove the C dependency here, since there is no C code in ccpp-framework?
|
|
||
| project(ccpp_blocked_data | ||
| VERSION 1.0.0 | ||
| LANGUAGES Fortran) | ||
| LANGUAGES C Fortran) |
There was a problem hiding this comment.
Note. I will clean this and line 14 up in a follow-up PR; similar for test_chunked_data.
|
@DusanJovic-NOAA @climbfuji Should I go ahead and merge this PR now? Last framework meeting we had discussed some needed changes to the weather model PR, but it looks like those are now complete? |
This needs to bubble up in the commit queue and go through the regression testing procedure first. Hopefully within the next two weeks! |
|
@DusanJovic-NOAA Do you know what the timeline is for this PR and the associated ccpp-physics PR? Thanks! |
|
@DusanJovic-NOAA PR #548 was merged inadvertently. If there's still time before EPIC starts testing your PRs, would you mind updating your framework PR? This shouldn't affect anything. Sorry for the inconvenience. |
@climbfuji I updated this branch with the NCAR:main |
|
Thanks very much @DusanJovic-NOAA. Following your PR we need to add another PR that repairs CI (doesn't have MPI dependency). |
|
Hi, @grantfirl . Testing is complete on ufs-wm PR #1147 . Please feel free to merge this PR. |
Add support to use mpi_f08 MPI module
These changes are needed in order to switch to Fortran 2008 MPI module, which provides generic interfaces (overloaded MPI functions/subroutines) and avoids mismatches between actual and dummy argument lists in mpi calls. Starting with version 10 of GNU Fortran, argument mismatches are treated as errors, and must be silenced my adding '-fallow-argument-mismatch' compiler flag, which is undesirable.
If code is compiled without MPI support dummy definition of MPI_Comm is provided. This type is used instead of equivalent integer communicator in various MPI calls.
User interface changes?: No
Testing: Full regression test of the ufs-weather-model on Hera and Hercules.
ufs-weather-model PR: ufs-community/ufs-weather-model#1147