MPI communicator generalizations #1277

jmsexton03 · 2024-07-19T22:04:20Z

Pull Request Summary

Description

Summary of changes:

add mpicomm module to track split communicator
use mpicomm module to replace MPI_COMM_WORLD statements

These changes extend some of the OASIS related changes by using a separate communicator that is specifically WW3-related for all routines. This makes communication safer by avoiding conflicting tags, and makes collective communications more maintainable.

Issue(s) addressed

This PR is associated with issue #1261
This PR addresses changes to make the use of MPI more general to allow for easier coupling.

Commit Message

MPI communicator generalizations

Check list

Branch is up to date with the authoritative repository (NOAA-EMC) develop branch.
Checked the checklist for a developer submitting to develop.
If a version number update is required, checked the updating version number checklist.
If a new feature was added, a regression test for testing the new feature is added.

Testing

How were these changes tested?
- These were tested by the github actions and were tested locally on a subset of the regtests
Are the changes covered by regression tests? (If not, why? Do new tests need to be added?)
- These are covered by the regression tests, although they won't significantly affect some exe if the regtests don't launch with MPI such as using mpirun or mpiexec
Have the matrix regression tests been run (if yes, please note HPC and compiler)?
- The matrix regression tests have not been run
Please indicate the expected changes in the regression test output, (Note the list of known non-identical tests.)
Please provide the summary output of matrix.comp (matrix.Diff.txt, matrixCompFull.txt and matrixCompSummary.txt):

MatthewMasarik-NOAA · 2024-07-22T21:50:38Z

Hi @jmsexton03, thank you for submitting this fix to #1261. I've started the regression testing for it. Just a note that @JessicaMeixner-NOAA is on travel this week, and we'd also like to allow time for trusted repo managers to do their own tests, so please allow some extra processing time for that. Early next week is a good tentative time to hear back.

JessicaMeixner-NOAA

I've added a few comments for consideration now. Additional testing and further review is likely necessary as I have only been able to have a quick look, but wanted to provide feedback as soon as possible.

JessicaMeixner-NOAA · 2024-07-23T16:34:39Z

model/src/cmake/switches.json

@@ -282,7 +282,7 @@
            },
            {
                "name": "NL2",
-                "build_files": ["w3snl2md.F90", "mod_xnl4v5.f90", "serv_xnl4v5.f90", "mod_fileio.f90", "mod_constants.f90"],
+                "build_files": ["w3snl2md.F90", "mod_xnl4v5.f90", "serv_xnl4v5.f90", "mod_fileio.f90", "mod_constants.f90", "mod_mpicomm.f90"],


I do not see a file mod_mpicomm.f90.

Also I think mpicomm.F90 should be adde with the MPI switch not NL2.

I can revert this change, I was initially confused about whether mod files were separately needed
(I will wait on this fix for further clarity on whether a separate module is needed)

JessicaMeixner-NOAA · 2024-07-23T16:35:18Z

model/src/mpicomm.F90

+!> @author  J. M. Sexton  @date 19-Jul-2024
+!>
+!
+#ifndef ENDIANNESS


Can you help me understand why this is added here?

This file was modelled off of constants, this ifdef is probably not needed.

JessicaMeixner-NOAA · 2024-07-23T16:36:36Z

model/src/w3initmd.F90

@@ -410,6 +410,7 @@ SUBROUTINE W3INIT ( IMOD, IsMulti, FEXT, MDS, MTRACE, ODAT, FLGRD,  FLGR2, FLGD,
         UA, UD, U10, U10D, AS
 #ifdef W3_MPI
    USE W3ADATMD, ONLY: MPI_COMM_WAVE, MPI_COMM_WCMP
+    USE MPICOMM


For all of these USE MPICOM, if we can add the "only" statement that would be nice. I know it's not uniformly done in WW3 right now, but since this is an addition, it would be good to add it.

I'll push a commit to make these all have an only statement

JessicaMeixner-NOAA · 2024-07-23T16:37:55Z

regtests/bin/matrix_cmake_datarmor

@@ -46,7 +46,7 @@ fi
 #convert main_dir to absolute path
 main_dir="`cd $main_dir 1>/dev/null 2>&1 && pwd`"

-  cmplr=datarmor_intel


@mickaelaccensi is this change okay?

This change was for testing only, I will push a commit reverting it, or if you want to do "Applying suggestions" through github I can also do that

JessicaMeixner-NOAA · 2024-07-23T16:39:23Z

model/src/mpicomm.F90

+  !/ ------------------------------------------------------------------- /
+  !/
+  !
+!#ifdef W3_MPI


Please clean up this commented out code.

are you referring to the ifdef W3_MPI here? I was unsure if I followed the header convention correctly

JessicaMeixner-NOAA · 2024-07-23T16:40:04Z

model/src/cmake/src_list.cmake

@@ -3,6 +3,7 @@ set(c_src w3getmem.c)
 # Core files always built
 set(ftn_src
  constants.F90
+  mpicomm.F90


It's unclear to me if this should only be included if you have the MPI switch and therefore be added in model/src/cmake/switches.json instead.

I did try to include ifdefs to make mpicomm.F90 not affect anything if it was compiled without MPI, but I can try to change this to model/src/cmake/switches.json since that sounds cleaner (I will wait on this fix for further clarity on whether a separate module is needed)

JessicaMeixner-NOAA · 2024-07-23T16:41:42Z

model/src/mpicomm.F90

+#endif
+!
+!/ ------------------------------------------------------------------- /
+MODULE MPICOMM


This is a fairly lightweight routine, should we add this to an existing file such as constants?

I'm not sure what the ramifications would be here, I'll consult with one of my collaborators who'd originally suggested having it as a separate file. I believe the intent was to make it more portable and obviously defined. Would w3parall.F90 be appropriate? I do see that IS_ESMF_COMPONENT is in constants.F90

I can move things to another location if that's your preference. When you know, can you point me to where specifically so that I can push an update?

w3parall is mostly for unstructured grids, where as this is for all grids, so I think constants might be a better choice. Others can weigh in on this thought too.

Thanks for the explanation, and I'm also interested in others thoughts 🙂 I was originally looking at w3parall because the SYNCHRONIZE_GLOBAL_ARRAY has been useful in our coupling tests so far, although I do see now that the header for this file indicates it's "Parallel routines for implicit solver"

JessicaMeixner-NOAA · 2024-07-23T16:44:06Z

.github/workflows/regtest_gnu.yml

@@ -102,10 +102,10 @@ jobs:
          export CC=mpicc
          export FC=mpif90
          export OASISDIR=${GITHUB_WORKSPACE}/work_oasis3-mct
-          # mkdir build && cd build


Are these changes intentional?

These changes were from attempting to test with debug flags on, but I should revert them (particularly as that seemed to not be a good way to build with debug)

mickaelaccensi

I agree to add it into constants.ftn.
I approve this PR with the condition it must pass all the regtests done by Matt

MatthewMasarik-NOAA · 2024-08-12T20:25:26Z

I agree to add it into constants.ftn. I approve this PR with the condition it must pass all the regtests done by Matt

@mickaelaccensi it looks like there are still many unresolved conversations above. I'll be posting this shortly, but we will be taking a pause on this and other PR processing temporarily. This is a significant enough of a PR that we'll want to have all code managers test on their respective machines when we resume work on this.

MatthewMasarik-NOAA · 2024-08-12T20:39:08Z

Hi @jmsexton03, thank you for your patience on this, and I apologize I haven't given a status update sooner. Our team has had some temporary changes that affect our ability to process PR's, so thanks for bearing with us while we worked behind the scenes to adjust. I just posted our group statement regarding this on the Commit Queue: Temporary PR Policy. We tentatively hope to resume mid to late November. This PR has some great advantages, we'll look forward to being able to look at it carefully when we pick back up our processing in November.

JessicaMeixner-NOAA

@jmsexton03 -Thank you for your patience with the delayed follow up. Let's move forward with moving your module to constants and then we can start re-reviewing from there.

jmsexton03 added 16 commits June 24, 2024 10:22

Make a module mpicomm to store MPI_COMM_WW3

1ef96e9

Merge remote-tracking branch 'develop' into mpi_comm_ww3

51967ea

Test with debug

1e561f8

Test with change to matrix

6a4c276

Remove debug compiler, uncomment cmake line

98b8425

Fix path

f2810fb

Test revert using all of them

5805c9b

Test revert PDLIB changes as well

ba8db4c

Small w3 add back

7805f33

Big w3 add back

53bbe18

Add to ww3_shel

a285a6a

Move world setup

9f34f74

Add all back and tweak multi similar to shel

863f97e

Tweak other ww3 similar to shel

ee61569

Merge branch 'develop' into mpi_comm_ww3

3cb627c

Minor header updates

c9bd906

JessicaMeixner-NOAA reviewed Jul 23, 2024

View reviewed changes

JessicaMeixner-NOAA requested review from thesser1, ukmo-ccbunney, mickaelaccensi and MatthewMasarik-NOAA July 23, 2024 16:45

jmsexton03 added 2 commits July 23, 2024 10:23

Revert unneeded changes and remove unneeded comment

cf3d25c

Change to using ONLY

18a9e51

mickaelaccensi approved these changes Aug 12, 2024

View reviewed changes

JessicaMeixner-NOAA requested changes Nov 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPI communicator generalizations #1277

MPI communicator generalizations #1277

jmsexton03 commented Jul 19, 2024

MatthewMasarik-NOAA commented Jul 22, 2024

JessicaMeixner-NOAA left a comment

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024 •

edited

Loading

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024 •

edited

Loading

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

jmsexton03 Jul 23, 2024

JessicaMeixner-NOAA Jul 24, 2024

jmsexton03 Jul 24, 2024

JessicaMeixner-NOAA Jul 23, 2024

jmsexton03 Jul 23, 2024

mickaelaccensi left a comment

MatthewMasarik-NOAA commented Aug 12, 2024

MatthewMasarik-NOAA commented Aug 12, 2024

JessicaMeixner-NOAA left a comment

MPI communicator generalizations #1277

Are you sure you want to change the base?

MPI communicator generalizations #1277

Conversation

jmsexton03 commented Jul 19, 2024

Pull Request Summary

Description

Issue(s) addressed

Commit Message

Check list

Testing

MatthewMasarik-NOAA commented Jul 22, 2024

JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsexton03 Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmsexton03 Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickaelaccensi left a comment

Choose a reason for hiding this comment

MatthewMasarik-NOAA commented Aug 12, 2024

MatthewMasarik-NOAA commented Aug 12, 2024

JessicaMeixner-NOAA left a comment

Choose a reason for hiding this comment

jmsexton03 Jul 23, 2024 •

edited

Loading

jmsexton03 Jul 23, 2024 •

edited

Loading