Make sub-communicators available for broadcasts and other uses such as MPI parallel I/O

NOAA-GFDL / FMS

GFDL's Flexible Modeling System

Other

87 stars 128 forks source link

Make sub-communicators available for broadcasts and other uses such as MPI parallel I/O #1457

Closed bensonr closed 4 months ago

bensonr commented 5 months ago

Description Exposes MPI_INFO_NULL as an MPP macro and give it a default value for compiles without MPI. Store MPI sub-communicators within the MPP domains type database for the domain and mosaic tiles. Expose MPI sub-communicators via getter functions.

Fixes #1456

How Has This Been Tested? Tested with temporary output and debug functions (removed) and ensuring updates pass manual CI tests on an internal dev box.

Checklist:

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] I have commented my code, particularly in hard-to-understand areas
[x] I have made corresponding changes to the documentation
[x] My changes generate no new warnings
[x] Any dependent changes have been merged and published in downstream modules
[x] New check tests, if applicable, are included
[x] make distcheck passes

bensonr commented 5 months ago

@rem1776 @thomas-robinson - it looks like something went wrong with the CI system, but I can't really tell for sure. Can you take a look and let me know?

rem1776 commented 5 months ago

@bensonr The intel CI failed due to the storage issues we've been seeing with github, so that's unrelated to this PR.

It also looks like the GNU CI failed initially with one of the data_override tests. I reran it cause it seemed unrelated, something about input files so probably a one-off error. It passed this time around.

bensonr commented 5 months ago

@rem1776 - thanks for making this happen. Can we get it on the upcoming release (2024.01)

rem1776 commented 5 months ago

@rem1776 - thanks for making this happen. Can we get it on the upcoming release (2024.01)

Yeah we should be able to get this in before we merge the diag manager updates.

thomas-robinson commented 5 months ago

This is going to need a unit test.

bensonr commented 5 months ago

@thomas-robinson - thanks for the review. I'll get to the documentation updates shortly. As for a unit test, what exactly are you hoping to see unit tested?

dkokron commented 5 months ago

Any estimate for when this could be available for friendly testers?

thomas-robinson commented 5 months ago

@thomas-robinson - thanks for the review. I'll get to the documentation updates shortly. As for a unit test, what exactly are you hoping to see unit tested?

I looked it over again, and I don't think there is anything to test yet.

bensonr commented 5 months ago

@dkokron - I've made the requested updates. I'll need @thomas-robinson and @rem1776 to comment on when we'd have a beta tag with this PR included.

rem1776 commented 5 months ago

@bensonr @dkokron We should be able to get this into main this week, possibly today depending on Tom's review.

Just want to also note that the 2024.01-beta4 tag will be on the dmUpdate development branch instead of main, but both main and the new tag will include this pr.