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

Removal of FMS1 I/O from FMS2 I/O infra #343

Merged

Conversation

marshallward
Copy link
Member

This patch removes the calls to FMS1 I/O (fms_io_mod, mpp_io_mod) from the FMS2 infra layer, and now exclusively uses FMS2 for those operations.

FMS2 I/O is currently restricted to files which use domains; files which do not use them are delegated to the native netCDF layer. The reasoning for this is that FMS is required to define the formatting of domain-decomposed I/O; for single-file I/O, this is not necessary.

This does not remove all references to FMS1 I/O from MOM6, only those in the I/O layer.

Several minor changes are included to accommodate the change:

  • MOM restart I/O now always reports its MOM domain. Previously, the domian was omitted when PARALLEL_RESTARTFILES was false, in order to trick FMS into handling this as a single file. We now generate a new domain with an IO layout of [1,1] when single-file restarts are requested.

  • The interface acceleration (g') was incorrectly set to the layer grid (Nk) rather than the interface grid (Nk+1). This did not appear to change any answers, but when Vertical_coordinate.nc was moved to the netCDF layer, it detected this error. This is fixed in this patch.

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #343 (d7d123b) into dev/gfdl (ded1382) will increase coverage by 0.01%.
The diff coverage is 72.00%.

❗ Current head d7d123b differs from pull request most recent head 411dde5. Consider uploading reports for the commit 411dde5 to get more accurate results

@@             Coverage Diff              @@
##           dev/gfdl     #343      +/-   ##
============================================
+ Coverage     38.12%   38.14%   +0.01%     
============================================
  Files           269      269              
  Lines         75622    75635      +13     
  Branches      13911    13915       +4     
============================================
+ Hits          28832    28849      +17     
+ Misses        41588    41584       -4     
  Partials       5202     5202              
Impacted Files Coverage Δ
src/framework/MOM_restart.F90 32.76% <0.00%> (ø)
src/framework/MOM_io_file.F90 50.00% <60.00%> (+0.71%) ⬆️
config_src/infra/FMS1/MOM_domain_infra.F90 36.44% <80.00%> (+0.32%) ⬆️
src/ALE/MOM_ALE.F90 45.09% <100.00%> (ø)
src/ALE/MOM_regridding.F90 28.73% <100.00%> (ø)
src/framework/MOM_io.F90 30.49% <100.00%> (+0.06%) ⬆️
src/initialization/MOM_coord_initialization.F90 55.81% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@marshallward marshallward force-pushed the purge_fms2_track_domain branch from 37f2e02 to 331d5ac Compare March 30, 2023 20:54
@marshallward
Copy link
Member Author

I've tested this in our regression suite, but there's lots of potential points of I/O failure. @MJHarrison-GFDL @kshedstrom If either of you could test this on any OBC experiments, or point me to them, that would be very helpful.

@kshedstrom
Copy link

I see no problems except that some of my cases generated a Vertical_coordinate file instead of a Vertical_coordinate.nc file. The contents appear to match, though.

@marshallward
Copy link
Member Author

Thanks, that is reassuring. As for Vertical_coordinate, we removed the logic which checks and adds the .nc if it's missing. We also set the default name to Vertical_coordinate.nc. Is it set to Vertical_coordinate in one of your files maybe?

@kshedstrom
Copy link

I don't think so. One example case is here: https://github.com/ESMG/ESMG-configs/tree/dev/esmg/ocean_only/seamount/z
Its friend the layer case has it right, while the sigma case is also wrong.

@klindsay28
Copy link

There's an occurrence of the string Vertical_coordinate without an .nc suffix in MOM_ALE.F90, in subroutine ALE_writeCoordinateFile, that this PR doesn't modify.

MOM6/src/ALE/MOM_ALE.F90

Lines 1457 to 1461 in 1bb66a4

character(len=240) :: filepath
filepath = trim(directory) // trim("Vertical_coordinate")
call write_regrid_file(CS%regridCS, GV, filepath)

Could that be the cause of what @kshedstrom is seeing?

@kshedstrom
Copy link

Also, that's ocean_only behaving. Is there a SIS2 patch coming?

//import/c1/AKWATERS/kate/ESMG/ESMG-configs/src/SIS2/src/SIS_sum_output.F90:373:74:

                        action=APPEND_FILE, form=ASCII_FILE, nohdrs=.true.)
                                                                          1
Error: Type mismatch in argument ‘io_handle’ at (1); passed INTEGER(4) to TYPE(file_type)

@marshallward
Copy link
Member Author

@klindsay28 That certainly looks like it would drop the .nc, thanks for finding that. I'll see if it can be safely renamed.

@kshedstrom The plan to is to get MOM6 updated while supporting the legacy API for SIS2, and then flip the SIS2 API. Not always possible, but I think that one might work.

(I guess none of our tests are calling write_ice_statistics...)

@kshedstrom
Copy link

That patch took care of the one problem.

@marshallward
Copy link
Member Author

open_ASCII_file was added to MOM6 framework some time ago, so it should be safe to flip over to that in SIS2. I will test it at some point today.

@marshallward
Copy link
Member Author

Hopefully this patch to SIS2 will fix the ASCII I/O issue.

https://github.com/NOAA-GFDL/SIS2/pull/193/files

@kshedstrom
Copy link

Yes, good to go!

This patch removes the calls to FMS1 I/O (fms_io_mod, mpp_io_mod) from
the FMS2 infra layer, and now exclusively uses FMS2 for those
operations.

FMS2 I/O is currently restricted to files which use domains; files which
do not use them are delegated to the native netCDF layer.  The reasoning
for this is that FMS is required to define the formatting of
domain-decomposed I/O; for single-file I/O, this is not necessary.

This does not remove all references to FMS1 I/O from MOM6, only those in
the I/O layer.

Several minor changes are included to accommodate the change:

* MOM restart I/O now always reports its MOM domain.  Previously, the
  domian was omitted when PARALLEL_RESTARTFILES was false, in order to
  trick FMS into handling this as a single file.  We now generate a new
  domain with an IO layout of [1,1] when single-file restarts are
  requested.

* The interface acceleration (g') was incorrectly set to the layer grid
  (Nk) rather than the interface grid (Nk+1).  This did not appear to
  change any answers, but when Vertical_coordinate.nc was moved to the
  netCDF layer, it detected this error.  This is fixed in this patch.
The `Vertical_coordinate.nc` files has two points of creation,
MOM_coord_initialization and MOM_ALE.  Having moved the file from the
infra to netCDF I/O layer, the .nc extension is no longer automatically
applied.

The extension was explicitly added to `Vertical_coordinate` in
MOM_coord_initialization, but not to MOM_ALE.  This patch adds the
extension.

Thanks to Kate Hedstrom for detecting this and Keith Lindsay for the
proposed fix.
@marshallward marshallward force-pushed the purge_fms2_track_domain branch from 4c7a691 to 411dde5 Compare April 24, 2023 20:56
@marshallward
Copy link
Member Author

I've pushed the requested cleanups here (mainly refactor + resolving conflicts).

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

All of my concerns with this PR have been addressed, and this PR is passing the pipeline testing at
gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/18922.

@Hallberg-NOAA Hallberg-NOAA merged commit f5423cb into NOAA-GFDL:dev/gfdl Apr 27, 2023
@marshallward marshallward deleted the purge_fms2_track_domain branch May 8, 2024 14:58
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.

4 participants