Bugfix addresses issue with mulitple mpi comm groups when dyn_npes<npes #1604

AaronDonahue · 2017-06-27T22:23:48Z

Addresses issue #1593

Addresses an issue in HOMME where multiple communication groups are
generated in cases where dyn_npes < npes for the ATM model. This
causes the code to crash for certain choices of dyn_npes with respects
to npes.
* Change to initmp subroutine for the case of CAM defined
* Change to namelist read for the case of CAM defined
* Change to dynamics initialization (dyn_init1) to be
compatible with changes to HOMME

[BFB] - Bit-For-Bit

Addresses an issue in HOMME where multiple communication groups are generated in cases where dyn_npes < npes for the ATM model. This causes the code to crash for certain choices of dyn_npes with respects to npes. * Change to initmp subroutine for the case of CAM defined * Change to namelist read for the case of CAM defined * Change to dynamics initialization (dyn_init1) to be compatible with changes to HOMME [BFB] - Bit-For-Bit

rljacob · 2017-06-28T02:18:03Z

@lazarusM3B can you add a real name to your github profile? All the usernames are hard to keep track of.

gold2718 · 2017-06-28T02:34:05Z

@lazarusM3B (aka Aaron Donahue as indicated in the branch name),
You could easily try out @mt5555's preferred method by gong back to just using par and its communicator so that line 135 of dyn_comp.F90 becomes:

if (iam < par%nprocs) then
    call readnl(par, NLFileName)
end if

and make the assignments to masterproc etc. inside of readnl.
Testing should reveal any issues with physics tasks not having reasonable namelist values (I don't expect any).

AaronDonahue · 2017-06-28T14:52:35Z

@rljacob , Absolutely, I've been meaning to do that for this reason.

@gold2718 @mt5555 , I'll go ahead and try this fix and see what happens. I'll let you know what I get.

AaronDonahue · 2017-06-28T15:26:42Z

@gold2718 @mt5555 , It looks like this fix works and is less intrusive then the previous approach. I'm going to run a quick B4B test to make sure that it isn't somehow running but has messed with the solution. Then push a new commit.

This commit is less invasive by leaving the readnl subroutine alone and just having the ATM model skip calling the subroutine for processors not assigned to the dynamics solve. * Reverted the homme file namelist_mod.F90 to original version * Changed dyn_init1 in dyn_comp.F90 to skip the namelist read for cores not assigned to solve dynamics. [BFB] - Bit-For-Bit

AaronDonahue · 2017-06-28T18:00:31Z

I pushed a new commit to the branch with the above changes made.

I am about to leave on vacation for a few weeks so I may not be able to respond to any new comments on this PR until I get back.

mt5555 · 2017-06-28T19:00:48Z

does this work?
I worry about statements like this:

 tstep = dtime/real(se_nsplit*qsplit,r8)

all processor will compute tstep, but in this case the wont all have the same value of se_nsplit, since it's in the namelist. In this case it is harmless, becuase non-dycore pes would never use 'tstep'.

Sorry to waffle on this, but your other solution might be better.

Or, how about making a fake "par",

type (parallel_t) :: par _all

par=initmp(npes_se)
par_all%comm = mpicom
par_all%root = 0
par_all%masterproc = (iam==0)
etc...

call readnl(par_all, NLFileName)

mt5555 · 2017-06-28T22:50:44Z

old comment: (moved from issue #1593 ) adding here for reference:

So are you saying that there are dycore namelist variables that are needed by the physics procs? I would have hoped that anything in the namelist read by the dycore is not needed by the physics, but that might be wishfull thinking.

An alternative would be to call readnl from the dp coupling layer (since it wont be a big deal to call it twice).

I can see your solution may be the best way - but my first reaction is I dont like dycore code sometimes working with one communicator, and other times working with a different communicator.

Fixed issue with GET_REFCASE not being set correctly that was introduced with PR#1561 and an issue with X compset mapping files that was introduced in #1586 Test suite: scripts_regression_tests.py Test baseline: Test namelist changes: Test status: bit for bit Fixes User interface changes?: Code review:

rljacob · 2017-07-24T17:10:00Z

Where are we with this PR?

rljacob · 2017-07-24T17:10:27Z

@mt5555 or @golaz you need to approve this.

AaronDonahue · 2017-07-24T17:18:34Z

@rljacob @mt5555 @golaz , I've been working on this this morning. Sorry it took so long to get back but I wasn't able to work on this until I got up and running on LC machines, which we just got working last week.

But to summarize what I've got so far. I took a look at implementing the suggestions made by @mt5555 up above but this lead to another set of errors further down the pipe in the code. I was trying to hunt down exactly why. As it stands the fix that is in this commit appears to work, but as @mt5555 pointed out it means that non-dynamics processors won't read in the dynamics namelist. So far this doesn't look like a problem, but could be hard to test for. If the reviewers are ok with this then we can probably call it finished, but if there are still concerns I am happy to continue working on this issue.

singhbalwinder · 2017-08-17T18:37:53Z

@mt5555 and @gold2718 : Is it okay to move forward with this PR?

mt5555 · 2017-08-17T23:06:56Z

@AaronDonahue : looks good - your knowledge of this is deeper than mine, so I defer to your judgement as to the best approach.

AaronDonahue · 2017-08-21T19:54:03Z

@mt5555 , Excellent, thank you. @singhbalwinder, I would like to take one more look at this code before we go ahead an integrate it. It has been over a month since I started this PR and I want to make sure that its ready to go.

rljacob · 2017-08-24T15:02:21Z

@AaronDonahue please merge this to next today.

singhbalwinder · 2017-08-24T15:58:51Z

@rljacob : Is your comment above for this PR? This PR was created by @AaronDonahue and I am the integrator. @AaronDonahue indicated in his comment above that he would like to work on the suggestions made by reviewers before merging it. Is that still the case @AaronDonahue ?

AaronDonahue · 2017-08-24T16:00:26Z

@singhbalwinder , yes, I was planning on making my final check this morning before giving the green light.

singhbalwinder · 2017-08-24T16:03:25Z

Sounds good. Let me know when you think you are okay with this PR and I will merge it to next.

rljacob · 2017-08-24T16:13:54Z

Sorry, I mean you should integrate it @singhbalwinder.

AaronDonahue · 2017-08-24T22:06:07Z

@singhbalwinder @rljacob , I've dug deeper into potential issues with handling communication groups in this way and found that the code can run into problems with a history write. Based on this I will need some more time to work on the issue.

rljacob · 2017-08-24T22:11:03Z

@AaronDonahue in that case, please close the PR and you can reopen it when its ready.

AaronDonahue · 2017-08-24T22:18:59Z

@rljacob , absolutely, will do

AaronDonahue added Atmosphere bug fix PR HOMME labels Jun 27, 2017

AaronDonahue mentioned this pull request Jun 27, 2017

Issue with interaction between Dynamics Init and HOMME #1593

Closed

AaronDonahue added the BFB PR leaves answers BFB label Jun 27, 2017

gold2718 requested review from gold2718 and singhbalwinder June 28, 2017 02:09

AaronDonahue assigned singhbalwinder Jun 28, 2017

rljacob requested review from amametjanov and removed request for singhbalwinder July 24, 2017 17:09

rljacob requested review from mt5555 and removed request for amametjanov July 24, 2017 17:10

mt5555 approved these changes Aug 17, 2017

View reviewed changes

AaronDonahue closed this Aug 21, 2017

AaronDonahue reopened this Aug 21, 2017

AaronDonahue closed this Aug 24, 2017

rljacob deleted the aarondonahue/cam/nprocs_fix branch February 15, 2018 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix addresses issue with mulitple mpi comm groups when dyn_npes<npes #1604

Bugfix addresses issue with mulitple mpi comm groups when dyn_npes<npes #1604

AaronDonahue commented Jun 27, 2017

rljacob commented Jun 28, 2017

gold2718 commented Jun 28, 2017

AaronDonahue commented Jun 28, 2017

AaronDonahue commented Jun 28, 2017 •

edited

Loading

AaronDonahue commented Jun 28, 2017

mt5555 commented Jun 28, 2017 •

edited

Loading

mt5555 commented Jun 28, 2017

rljacob commented Jul 24, 2017

rljacob commented Jul 24, 2017

AaronDonahue commented Jul 24, 2017

singhbalwinder commented Aug 17, 2017

mt5555 commented Aug 17, 2017

AaronDonahue commented Aug 21, 2017 •

edited

Loading

rljacob commented Aug 24, 2017

singhbalwinder commented Aug 24, 2017

AaronDonahue commented Aug 24, 2017

singhbalwinder commented Aug 24, 2017

rljacob commented Aug 24, 2017 •

edited

Loading

AaronDonahue commented Aug 24, 2017

rljacob commented Aug 24, 2017

AaronDonahue commented Aug 24, 2017

Bugfix addresses issue with mulitple mpi comm groups when dyn_npes<npes #1604

Bugfix addresses issue with mulitple mpi comm groups when dyn_npes<npes #1604

Conversation

AaronDonahue commented Jun 27, 2017

rljacob commented Jun 28, 2017

gold2718 commented Jun 28, 2017

AaronDonahue commented Jun 28, 2017

AaronDonahue commented Jun 28, 2017 • edited Loading

AaronDonahue commented Jun 28, 2017

mt5555 commented Jun 28, 2017 • edited Loading

mt5555 commented Jun 28, 2017

rljacob commented Jul 24, 2017

rljacob commented Jul 24, 2017

AaronDonahue commented Jul 24, 2017

singhbalwinder commented Aug 17, 2017

mt5555 commented Aug 17, 2017

AaronDonahue commented Aug 21, 2017 • edited Loading

rljacob commented Aug 24, 2017

singhbalwinder commented Aug 24, 2017

AaronDonahue commented Aug 24, 2017

singhbalwinder commented Aug 24, 2017

rljacob commented Aug 24, 2017 • edited Loading

AaronDonahue commented Aug 24, 2017

rljacob commented Aug 24, 2017

AaronDonahue commented Aug 24, 2017

AaronDonahue commented Jun 28, 2017 •

edited

Loading

mt5555 commented Jun 28, 2017 •

edited

Loading

AaronDonahue commented Aug 21, 2017 •

edited

Loading

rljacob commented Aug 24, 2017 •

edited

Loading