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

Differentiate input and input/output streams in MPAS stream manager #4929

Merged
merged 2 commits into from
May 11, 2022

Conversation

matthewhoffman
Copy link
Contributor

@matthewhoffman matthewhoffman commented May 2, 2022

This PR improves the MPAS Stream Manager by introducing an optional argument to differentiate between streams of direction INPUT and direction INPUT_OUTPUT. Previously, the mpas_stream_mgr_read() routine ignored the difference
between streams of direction INPUT and direction INPUT_OUTPUT (i.e. restart streams). However, there are situation where it would be desirable to read only truly direction INPUT streams. One example is at the initial time of a coldstart run - one would typically want to read any input streams without reading the restart stream (which does not exist yet). This PR adds an optional argument to force mpas_stream_mgr_read to ignore INPUT_OUTPUT streams and only read truly INPUT streams. Its default value maintains the previous behavior. A second commit modifies the MALI driver to make use of this new capability to improve and simplify the input stream handling at the initial time of a MALI simulation.

[BFB]

…reams

Previously, the mpas_stream_mgr_read() routine ignored the difference
between streams of direction INPUT and direction INPUT_OUTPUT (i.e.
restart streams).  However, there are situation where it would be
desirable to read only truly direction INPUT streams.  One example is at
the initial time of a coldstart run - one wants to read any input streams without
reading the restart stream (which does not exist yet).

This commit adds an optional argument to force mpas_stream_mgr_read to
ignore INPUT_OUTPUT streams and only read truly INPUT streams.  Its
default value maintains the previous behavior.
This commit removes the restriction that the initial condition stream
have the name 'input' and now allows any streams of direction input that
have input_interval='initial_only' to be read at the initial time.  This
allows an initial condition to be spread across multiple files.

It also prevents an obscure error that was causing streams of
input_interval='initial_only' to be read on the initial time of a
restart.  With this change, 'initial_only' truly means the initial time
of a simulation, regardless of restarting it or not.

This commit also adds some missing timer calls.
@matthewhoffman
Copy link
Contributor Author

Discussed and tested for standalone MALI usage in the MALI-Dev fork of E3SM here: MALI-Dev#35

I've moved the PR here because it changes MPAS framework. I could move the MALI changes into a separate PR, but I don't see a problem with including it here. If we keep MALI changes here, I would close MALI-Dev#35

@matthewhoffman
Copy link
Contributor Author

@xylar , @mark-petersen , @akturner , I looked into the issues with MPAS_stream_mgr_read that we discussed on the MPAS call today and familiarized myself with why MALI ran into the issue the PR addresses but ocean and sea ice do not. An important detail is that while restart streams have direction INPUT_OUTPUT, the input interval is set to initial_only, meaning a restart stream will only be attempted to read at the initial time. The code that Mark brought up on our call was the stream manager read during timestepping, which is why the issue related to the restart stream does not apply.

On contrast, the relevant bit of of code in MPAS-Ocean is the read on init here:
https://github.com/E3SM-Project/E3SM/blob/master/components/mpas-ocean/src/mode_forward/mpas_ocn_forward_mode.F#L208-L222
Note that there is specific logic ensuring that the stream read on init is only ever called 'input' or 'restart. This is what MALI had also done in the past. However, we wanted to be able to support having potentially our initial condition spread across multiple files. (An example use case is a UQ study where we explore the impact of different basal friction fields, while the rest of the initial condition is otherwise identical.). To have initial conditions spread across multiple files, we wanted to have a call to MPAS_stream_mgr_read on init that just read any input files that are set up to be read at the initial time. But the generic call to MPAS_stream_mgr_read was also trying to read the 'restart' stream at the initial time. This led to an error because the restart files don't even exist yet at the initial time of a cold start, and even if they did, we wouldn't actually want to read it.

So the new optional argument I introduced in this PR tells MPAS_stream_mgr_read to ignore streams that are INPUT_OUTPUT (i.e. restart streams) when it looks for any streams to be read. As mentioned above, without the optional argument, behavior should be identical as before. (For how things are handled in the ocean, adding that argument should not change anything either, but there is no reason to add it.)

@xylar
Copy link
Contributor

xylar commented May 3, 2022

@matthewhoffman, thanks for the clarification. I'll do some standalone testing in compass. @jonbob, do you have a suggestion for what the most relevant test in E3SM would be to make sure restarting is unaffected in both ocean and sea ice? I'm happy to run such a test if it's not already part of your testing.

@jonbob
Copy link
Contributor

jonbob commented May 3, 2022

@xylar -- this one is good for both seaice and ocean:

ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel

But it is one of the tests I almost always run to test a merge

@xylar
Copy link
Contributor

xylar commented May 7, 2022

I am running compass tests on this right now. I tried the E3SM tests but ran into ESMCI/cime#4251. I will try with a clone of E3SM outside of compass.

@xylar
Copy link
Contributor

xylar commented May 8, 2022

I did a test merge of this PR with the latest E3SM-Project submodule in compass (E3SM/master from 4 days ago). Then, I ran the ocean PR test suite from compass in debug and optimized mode for the 5 compiler/MPI configurations we have on Anvil. All passed and were bit-for-bit with the baseline I created when the submodule in compass got updated.

I ran ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel and ERS.ne11_oQU240.WCYCL1850NS.anvil_intel successfully with the same test merge (in this case, I did not compare it with a baseline).

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@matthewhoffman matthewhoffman removed the request for review from mark-petersen May 10, 2022 14:43
Copy link
Contributor

@akturner akturner left a comment

Choose a reason for hiding this comment

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

Tested in standalone MPAS-Seaice

@jonbob
Copy link
Contributor

jonbob commented May 10, 2022

@matthewhoffman - not a big deal this time, but the branch naming convention is to use hyphens and not underscores...

jonbob added a commit that referenced this pull request May 10, 2022
#4929)

Differentiate input and input/output streams in MPAS stream manager

This PR improves the MPAS Stream Manager by introducing an optional
argument to differentiate between streams of direction INPUT and
direction INPUT_OUTPUT. Previously, the mpas_stream_mgr_read() routine
ignored the difference between streams of direction INPUT and direction
INPUT_OUTPUT (i.e.  restart streams). However, there are situation where
it would be desirable to read only truly direction INPUT streams. One
example is at the initial time of a coldstart run - one would typically
want to read any input streams without reading the restart stream (which
does not exist yet). This PR adds an optional argument to force
mpas_stream_mgr_read to ignore INPUT_OUTPUT streams and only read truly
INPUT streams. Its default value maintains the previous behavior. A
second commit modifies the MALI driver to make use of this new
capability to improve and simplify the input stream handling at the
initial time of a MALI simulation.

[BFB]
@jonbob
Copy link
Contributor

jonbob commented May 10, 2022

test merge passes:

  • ERS.ne11_oQU240.WCYCL1850NS.chrysalis_intel
  • SMS_D_Ld3.T62_oQU120.CMPASO-IAF.chrysalis_intel
  • SMS.T62_oQU120_ais20.MPAS_LISIO_TEST.chrysalis_intel

merged to next

@jonbob jonbob merged commit 872c037 into master May 11, 2022
@jonbob
Copy link
Contributor

jonbob commented May 11, 2022

merged to master

@jonbob jonbob deleted the matthewhoffman/mpas/stream_read_input_only branch May 11, 2022 17:47
matthewhoffman referenced this pull request in MALI-Dev/E3SM May 12, 2022
This is desired to get E3SM-Project#4929 and updates for compiling on
Cori.

* e3sm/master: (288 commits)
  Add a fix for restarts generated before the final time step
  Fixes CIME import paths in template files
  Homme genf90 fix
  Add back custom mpaso and elm PEs for Mappy
  Add minor CIME fix for configure tool
  CIME update
  Added fsurdat for simyr2010 on ne120np4
  Add cime update to set component-specific PES_SPEC_FILE
  Simplify & generalize MALI stream read behavior at initial time
  Allow mpas_stream_mgr_read() to read only strictly INPUT direction streams
  Split config_pesall into component-specific config_pes
  run_e3sm.sh: Make it explicit for users to change case name
  run_e3sm: case_group argument activated for create_newcase when valid
  run_e3sm script: Replace default case name and case group placeholders
  Add support for northamericax4v1pg2_EC30to60E2r2 resolution
  Updates seaice BGC namelist defaults to match Registry defaults.
  Summit perf archive: set location to cli115 for all
  Corrected possible division by zero in ice bgc
  Enable performance archiving for all projects on Summit
  added '-O1 -hfp0' flags for crayclang to reduce NaN-related errors
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants