Fix physgrid issues with PE layout and aqua restarts #3204

whannah1 · 2019-09-25T17:15:28Z

This fixes an issues where aqua planet cases were crashing on restart due to not correctly setting the topography to zero. Instead the realistic topography was added on restart, which caused the run to crash. This PR resolves this issue by explicitly resetting the topography to zero when restarting a physgrid run.

This also fixes an issue running certain physgrid configurations, such as ne4pg2, because the default NTASK was set higher than the number of physics columns. To get the physgrid specification to match correctly in config_pesall.xml an underscore was to be added to other grid query strings. This should not affect non-physgrid cases. The underscore is used because it always separates the atmosphere grid from the other grids. For example, A typical ne4 grid string is
a%ne4np4_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240
But the string for ne4pg2 is
a%ne4np4.pg2_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240
so in order to differentiate the grids we use "ne4np4_" and "ne4np4.pg2" to get different default PE layouts. Using "ne4np4" without the underscore leads to an error when running physgrid cases.

After some discussion we decided to also include a change to the default physgrid mapping algorithm to use the high-order option. This will make all pg2 tests non-BFB.

[BFB] except for SMS_Ln5.ne4pg2_ne4pg2.FC5AV1C-L

modified: components/cam/src/dynamics/se/inidat.F90

modified: components/cam/src/dynamics/se/dyn_grid.F90 modified: components/cam/src/dynamics/se/fv_physics_coupling_mod.F90 modified: components/cam/src/dynamics/se/inidat.F90

modified: components/cam/src/dynamics/se/inidat.F90

modified: components/cam/src/dynamics/se/fv_physics_coupling_mod.F90

modified: cime/config/e3sm/allactive/config_pesall.xml

modified: components/cam/src/dynamics/se/inidat.F90

modified: cime/config/e3sm/allactive/config_pesall.xml

ambrad · 2019-09-25T18:01:03Z

components/cam/src/dynamics/se/inidat.F90

@@ -475,6 +471,13 @@ subroutine read_inidat( ncid_ini, ncid_topo, dyn_in)
    end if

    if (fv_nphys == 0) then
+      ! Map phis data to FV physics grid


This appears not to be right. The conditional at line 473 is fv_nphys == 0, but lines 474 to 479 then perform physgrid operations.

oh, shoot, good catch. That's a pretty dumb mistake.
Now I'm curious how my restart tests even passed...

ambrad · 2019-09-25T18:01:40Z

components/cam/src/dynamics/se/inidat.F90

    else    
      fieldname = 'PHIS'
      tmp(:,1,:) = 0.0_r8
      if (fv_nphys > 0) then
-         ! Copy phis field to GLL grid
+         ! Load phis field to GLL grid first


I believe this comment is incorrect when separated from the fv_phys_to_dyn_topo function calls. Here, infld is loading physgrid topo data, not GLL data.

Right, I'll correct that.

modified: components/cam/src/dynamics/se/inidat.F90

ambrad

I can't comment on the config_pesall.xml changes; I can guess why the underscore is needed but I don't have sufficiently detailed knowledge of the pes mechanics to be sure of the changes. The inidat changes look fine now. The other F90 changes are specific to the LO remap code, and I know you've tested these a fair bit already.

AaronDonahue

It looks good to me. Do you check and make sure there were no other locations where the phys_tmp variable should be changed to phis_tmp?

whannah1 · 2019-09-26T18:06:46Z

@AaronDonahue , I couldn't find any other instances of phys_tmp

modified: components/cam/src/dynamics/se/inidat.F90

oksanaguba · 2019-10-01T21:53:53Z

@whannah1 I see some recent commits, are you still working on this?

whannah1 · 2019-10-01T22:08:53Z

@oksanaguba, I noticed a couple minor things that don't seem to have any effect (when testing on Cori at least). I think this branch is still good to go, but I was gonna ask @AaronDonahue to give it a quick look over again. I just need another day or two to verify these fixes also work for the ECP fork, because there's a (perhaps different?) restart issue there as well that I'm struggling to understand.

modified: components/cam/src/dynamics/se/inidat.F90

whannah1 · 2019-10-03T22:38:06Z

Ok, so the problem that I thought I had appears to be a non-issue. @AaronDonahue, can you take a quick look at the recent changes? Afterwards this PR should be ready to go.

rljacob · 2019-10-10T17:48:56Z

@AaronDonahue this needs another review from you.

modified: components/cam/bld/namelist_files/namelist_defaults_cam.xml

mt5555 · 2019-10-31T13:53:21Z

2nding @rljacob request for another review from @AaronDonahue !

AaronDonahue · 2019-10-31T15:30:35Z

My apologies, I've been in and out of travel and missed this pinging of me. I'll get on this right away.

whannah1 · 2019-11-06T00:12:26Z

@oksanaguba , what's the run length of this test?
ERS.ne4_ne4.FC5AV1C-L.anvil-centos7_intel.cam-thetanh_ftype2

oksanaguba · 2019-11-06T02:57:15Z

memory output, master and then PR:

(base) [onguba@blueslogin4 run]$ gunzip cpl.log.88244.191105-170925.gz
(base) [onguba@blueslogin4 run]$ grep highwater cpl.log.88244.191105-170925
 memory_write: model date =   00010102       0 memory =  156081.60 MB (highwater)      58845.44 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010103       0 memory =  156081.92 MB (highwater)      59142.72 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010104       0 memory =  156081.92 MB (highwater)      59145.92 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010105       0 memory =  156081.92 MB (highwater)      59150.08 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010106       0 memory =  156081.92 MB (highwater)      59324.48 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
(seq_mct_drv): ===============  pes min memory highwater  (MB)    1120.967  ===============
(seq_mct_drv): ===============  pes max memory highwater  (MB)  465381.000  ===============
(base) [onguba@blueslogin4 run]$
(base) [onguba@blueslogin4 run]$ cd ../../SMS.ne4_ne4.FC5AV1C-L.anvil-centos7_intel.cam-thetanh_ftype2.C.20191105_172101_ewo6rl/run/
(base) [onguba@blueslogin4 run]$ gunzip cpl.log.88278.191105-174828.gz
(base) [onguba@blueslogin4 run]$ grep highwater cpl.log.88278.191105-174828
 memory_write: model date =   00010102       0 memory =  185810.67 MB (highwater)      71104.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010103       0 memory =  185811.05 MB (highwater)      71373.33 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010104       0 memory =  185811.05 MB (highwater)      71435.81 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010105       0 memory =  185811.05 MB (highwater)      71438.48 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010106       0 memory =  185811.05 MB (highwater)      71691.05 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
(seq_mct_drv): ===============  pes min memory highwater  (MB)    1109.342  ===============
(seq_mct_drv): ===============  pes max memory highwater  (MB)  248205.867  ===============

ambrad · 2019-11-06T05:25:48Z

Are the number of processes the same in master and PR runs? Maybe the PE XML changes are somehow (erroneously) changing that, giving fewer processes with consequently more memory per process.

oksanaguba · 2019-11-06T15:46:25Z

Some sanity checks: both clones are from yesterday's master.

Ranks: Both have global communicator with 144 ranks, both homme outputs have 96 ranks, 1 element per rank.

oksanaguba · 2019-11-06T15:50:06Z

@whannah1 tests ERS.ne4_ne4.FC5AV1C-L.anvil-centos7_intel.cam-thetanh_ftype2 ran for < 2 minutes.

whannah1 · 2019-11-06T15:50:50Z

@oksana I meant the run length, since it's not specified in the test name. What's the default run length for an ERS test?

oksanaguba · 2019-11-06T15:54:39Z

11 days for that ERS test.

whannah1 · 2019-11-06T15:59:20Z

This might be a dumb question, but could a change in the submodules explain the change in memory use? Maybe one of the clones didn't sync those properly?

oksanaguba · 2019-11-06T16:08:09Z

Both runs were done on anvil in the same terminal window. But I will resubmit PR run now. I also looked at some timers and all looks ok.

oksanaguba · 2019-11-06T16:09:32Z

I misread your question. submodules. I will resync clones.

oksanaguba · 2019-11-06T16:12:27Z

Baselines clone had older commits in submodules, tests are running now.

whannah1 · 2019-11-06T16:17:06Z

ok, let's hope that's the explanation.

oksanaguba · 2019-11-06T16:45:49Z

Here are two runs from master, (first) yesterday with older commits for mpp, mpas, and scorpio, and today's the same clone with synced modules (second):

(base) [onguba@blueslogin2 run]$ cd ../../SMS.ne4_ne4.FC5AV1C-L.anvil-centos7_intel.cam-thetanh_ftype2.G.20191105_164323_gq0oc1/run/
(base) [onguba@blueslogin2 run]$ grep highwater cpl.log.*
 memory_write: model date =   00010102       0 memory =  156081.60 MB (highwater)      58845.44 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010103       0 memory =  156081.92 MB (highwater)      59142.72 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010104       0 memory =  156081.92 MB (highwater)      59145.92 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010105       0 memory =  156081.92 MB (highwater)      59150.08 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010106       0 memory =  156081.92 MB (highwater)      59324.48 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
(seq_mct_drv): ===============  pes min memory highwater  (MB)    1120.967  ===============
(seq_mct_drv): ===============  pes max memory highwater  (MB)  465381.000  ===============
(base) [onguba@blueslogin2 run]$ cd ../../SMS.ne4_ne4.FC5AV1C-L.anvil-centos7_intel.cam-thetanh_ftype2.G.20191106_101124_ih5vi9/run/
(base) [onguba@blueslogin2 run]$ grep highwater cpl.log.*
 memory_write: model date =   00010102       0 memory =  975524.00 MB (highwater)     370904.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010103       0 memory =  975526.00 MB (highwater)     372420.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010104       0 memory =  975526.00 MB (highwater)     372646.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010105       0 memory =  975526.00 MB (highwater)     372672.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
 memory_write: model date =   00010106       0 memory =  975526.00 MB (highwater)     373930.00 MB (usage)  (pe=    0 comps= cpl ATM ICE GLC WAV IAC ESP)
(seq_mct_drv): ===============  pes min memory highwater  (MB)    1111.784  ===============
(seq_mct_drv): ===============  pes max memory highwater  (MB) 1249578.667  ===============

ambrad · 2019-11-06T17:54:47Z

If I'm reading these data correctly, (1) the second has >2x larger "max memory highwater" and >5x highwater on rank 0 but (2) the only difference is submodule SHA1s. Do others agree? If I have those two points right, then there seems to be either noise or a real issue due to a submodule change but likely nothing related to this PR.

oksanaguba · 2019-11-06T18:16:38Z

yes, has nothing to do with this pr. I will make an issue.

oksanaguba · 2019-11-06T18:43:01Z

Reran tests, e3sm_atm_integration, against master with updated clone. all baselines pass, memory issues are not from this pr.

whannah1 · 2019-11-06T18:46:12Z

that's good, thanks for all the extra effort!

… (PR #3204) This fixes an issues where aqua planet cases were crashing on restart due to not correctly setting the topography to zero. Instead the realistic topography was added on restart, which caused the run to crash. This PR resolves this issue by explicitly resetting the topography to zero when restarting a physgrid run. This also fixes an issue running certain physgrid configurations, such as ne4pg2, because the default NTASK was set higher than the number of physics columns. To get the physgrid specification to match correctly in config_pesall.xml an underscore was to be added to other grid query strings. This should not affect non-physgrid cases. The underscore is used because it always separates the atmosphere grid from the other grids. For example, A typical ne4 grid string is a%ne4np4_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240  But the string for ne4pg2 is a%ne4np4.pg2_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240  so in order to differentiate the grids we use "ne4np4_" and "ne4np4.pg2" to get different default PE layouts. Using "ne4np4" without the underscore leads to an error when running physgrid cases. After some discussion we decided to also include a change to the default physgrid mapping algorithm to use the high-order option. This will make all pg2 tests non-BFB. [BFB] except for SMS_Ln5.ne4pg2_ne4pg2.FC5AV1C-L

oksanaguba · 2019-11-07T18:21:37Z

merged to next

oksanaguba · 2019-11-08T17:30:30Z

melvin does not allow 96 cores for pg2 runs, limit is 60. a fix was pushed, I need to wait for another day.

… (PR #3204) This fixes an issues where aqua planet cases were crashing on restart due to not correctly setting the topography to zero. Instead the realistic topography was added on restart, which caused the run to crash. This PR resolves this issue by explicitly resetting the topography to zero when restarting a physgrid run. This also fixes an issue running certain physgrid configurations, such as ne4pg2, because the default NTASK was set higher than the number of physics columns. To get the physgrid specification to match correctly in config_pesall.xml an underscore was to be added to other grid query strings. This should not affect non-physgrid cases. The underscore is used because it always separates the atmosphere grid from the other grids. For example, A typical ne4 grid string is a%ne4np4_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240 But the string for ne4pg2 is a%ne4np4.pg2_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240 so in order to differentiate the grids we use "ne4np4_" and "ne4np4.pg2" to get different default PE layouts. Using "ne4np4" without the underscore leads to an error when running physgrid cases. After some discussion we decided to also include a change to the default physgrid mapping algorithm to use the high-order option. This will make all pg2 tests non-BFB. [BFB] except for SMS_Ln5.ne4pg2_ne4pg2.FC5AV1C-L tests

oksanaguba · 2019-11-08T18:57:45Z

remerged to next

…er (PR #3204) This fixes an issues where aqua planet cases were crashing on restart due to not correctly setting the topography to zero. Instead the realistic topography was added on restart, which caused the run to crash. This PR resolves this issue by explicitly resetting the topography to zero when restarting a physgrid run. This also fixes an issue running certain physgrid configurations, such as ne4pg2, because the default NTASK was set higher than the number of physics columns. To get the physgrid specification to match correctly in config_pesall.xml an underscore was to be added to other grid query strings. This should not affect non-physgrid cases. The underscore is used because it always separates the atmosphere grid from the other grids. For example, A typical ne4 grid string is a%ne4np4_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240 But the string for ne4pg2 is a%ne4np4.pg2_l%ne4np4_oi%ne4np4_r%null_g%null_w%null_z%null_m%oQU240 so in order to differentiate the grids we use "ne4np4_" and "ne4np4.pg2" to get different default PE layouts. Using "ne4np4" without the underscore leads to an error when running physgrid cases. After some discussion we decided to also include a change to the default physgrid mapping algorithm to use the high-order option. This will make all pg2 tests non-BFB. [BFB] except for SMS_Ln5.ne4pg2_ne4pg2.FC5AV1C-L tests

…id-new-topo-file-format Conflicts: components/cam/src/dynamics/se/inidat.F90 Merge master to get PR E3SM-Project#3204; resolve the conflict in inidat concerning reading PHIS(_d) from the topo file.

whannah1 added 7 commits September 23, 2019 13:55

Fix restart bug with physgrid on aquaplanet

720b327

modified: components/cam/src/dynamics/se/inidat.F90

Fix more stuff

d7af49d

modified: components/cam/src/dynamics/se/dyn_grid.F90 modified: components/cam/src/dynamics/se/fv_physics_coupling_mod.F90 modified: components/cam/src/dynamics/se/inidat.F90

change phys_tmp to phis_tmp

d31d268

modified: components/cam/src/dynamics/se/inidat.F90

add fv_physgrid

3d29819

modified: components/cam/src/dynamics/se/fv_physics_coupling_mod.F90

Add new pes defaults for ne4 + pg1,2,3

d592db3

modified: cime/config/e3sm/allactive/config_pesall.xml

simplify aqua topo bug fix

f8e1e2c

modified: components/cam/src/dynamics/se/inidat.F90

Fix PE-layout grid matching issue for physgrid

b504569

modified: cime/config/e3sm/allactive/config_pesall.xml

whannah1 requested review from ambrad and AaronDonahue September 25, 2019 17:15

whannah1 assigned rljacob Sep 25, 2019

ambrad reviewed Sep 25, 2019

View reviewed changes

Fix topo mapping

90d5a1b

modified: components/cam/src/dynamics/se/inidat.F90

ambrad approved these changes Sep 25, 2019

View reviewed changes

rljacob assigned oksanaguba and unassigned rljacob Sep 25, 2019

AaronDonahue approved these changes Sep 26, 2019

View reviewed changes

whannah1 added 2 commits September 30, 2019 08:26

Cosmetic update for consistency

4f39178

modified: components/cam/src/dynamics/se/inidat.F90

Add check to not set phis_tmp when not allocated

b3655b3

modified: components/cam/src/dynamics/se/inidat.F90

Another cosmetic update

66a5c3e

modified: components/cam/src/dynamics/se/inidat.F90

rljacob added the Atmosphere label Oct 14, 2019

Change se_fv_phys_remap_alg default to 1

2b22458

modified: components/cam/bld/namelist_files/namelist_defaults_cam.xml

whannah1 mentioned this pull request Oct 30, 2019

Physgrid: Introduce new topography file format. #3267

Merged

Add deallocate statement for phis_tmp

aab7b44

adding melving settings for ne4np4pg2

01ee915

oksanaguba merged commit 01ee915 into master Nov 11, 2019

oksanaguba deleted the whannah/atm/fix-physgrid-aqua-restart branch November 11, 2019 17:54

oksanaguba mentioned this pull request Dec 5, 2019

Homme DIRK arithmetic #3320

Merged

amametjanov mentioned this pull request Sep 29, 2021

Update V2 benchmarking grids and compsets #4551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix physgrid issues with PE layout and aqua restarts #3204

Fix physgrid issues with PE layout and aqua restarts #3204

whannah1 commented Sep 25, 2019 •

edited by oksanaguba

Loading

ambrad Sep 25, 2019

whannah1 Sep 25, 2019 •

edited

Loading

ambrad Sep 25, 2019

whannah1 Sep 25, 2019

ambrad left a comment

AaronDonahue left a comment

whannah1 commented Sep 26, 2019

oksanaguba commented Oct 1, 2019

whannah1 commented Oct 1, 2019

whannah1 commented Oct 3, 2019

rljacob commented Oct 10, 2019

mt5555 commented Oct 31, 2019

AaronDonahue commented Oct 31, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

ambrad commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019 •

edited

Loading

ambrad commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 7, 2019

oksanaguba commented Nov 8, 2019

oksanaguba commented Nov 8, 2019

Fix physgrid issues with PE layout and aqua restarts #3204

Fix physgrid issues with PE layout and aqua restarts #3204

Conversation

whannah1 commented Sep 25, 2019 • edited by oksanaguba Loading

ambrad Sep 25, 2019

Choose a reason for hiding this comment

whannah1 Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

ambrad Sep 25, 2019

Choose a reason for hiding this comment

whannah1 Sep 25, 2019

Choose a reason for hiding this comment

ambrad left a comment

Choose a reason for hiding this comment

AaronDonahue left a comment

Choose a reason for hiding this comment

whannah1 commented Sep 26, 2019

oksanaguba commented Oct 1, 2019

whannah1 commented Oct 1, 2019

whannah1 commented Oct 3, 2019

rljacob commented Oct 10, 2019

mt5555 commented Oct 31, 2019

AaronDonahue commented Oct 31, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

ambrad commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 6, 2019 • edited Loading

ambrad commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

oksanaguba commented Nov 6, 2019

whannah1 commented Nov 6, 2019

oksanaguba commented Nov 7, 2019

oksanaguba commented Nov 8, 2019

oksanaguba commented Nov 8, 2019

whannah1 commented Sep 25, 2019 •

edited by oksanaguba

Loading

whannah1 Sep 25, 2019 •

edited

Loading

oksanaguba commented Nov 6, 2019 •

edited

Loading