Plumber.5.2

[wwieder]

Description of changes

Fixes usermods to point to 5.2 datasets

Specific notes

Still kind of hacky, ideally we can take this out of the custom shell commands for individual sites.
I think this can happen after @TeaganKing adds PLUMBER2SITE to cdeps?

CTSM Issues Fixed (include github issue #):
Fixes #2484

Any User Interface Changes (namelist or namelist defaults changes)?
updates for usermods

Testing performed, if any:
Manual test to make sure sites run as expected with 5.2 datasets

[wwieder]

@ekluzek I wasn't clear if you thought this should go on b4b-dev or master. If we merge it with other changes from @TeaganKing it may be easier? I just wanted to create this branch and PR so we can keep making progress on PLUMBER2 work.

[ekluzek]

I think this should go to b4b-dev, and I just rebased it to reflect that. Technically right now both are identical.

[TeaganKing]

This is the WIP PR where some of these changes are being made. Maybe I can just make a separate PR for the config_component.xml and cime_config details in order to contribute that to this PR? I think the rest of the linked PR will take a decent amount of additional effort to finalize.

[wwieder]

That could be nice to bring in the cime_config and config_component work here, as it will further clean up the usermods_dirs. I'll let you manage what makes the most sense, @TeaganKing. I mainly wanted to get something that you could start working with for the larger run_towers refactor you're doing.

[TeaganKing]

I actually realized there is also a relevant CDEPS PR that goes along with these changes. I'll add the CTSM changes here.

[TeaganKing]

I ran black on cime_config/buildnml and got a clean run with the second commit, so I'm not sure why that's still failing on black? Have you seen that before?

[TeaganKing]

In the ctsm_pylib environment, I ran black --version and saw: black, 22.3.0 (compiled: no). Black that's being run on GitHub is black[colorama]==22.3.0. I think this is the same version.

python/pyproject.toml contains the same information that is in my local copy-- could this be overwritten somewhere during the black check such that black fails on GitHub? This includes items such as line-length and target-version.

This seems like it should be a very simple fix...

[TeaganKing]

I still haven't resolved the black issue, but need to head out soon so am leaving this unresolved at the moment. Where I'm at right now is that I found the reverting the following line to the original passes black (and does not include PLUMBER functionality that we need):
if ("NEON" in clm_usrdat_name or "PLUMBER" in clm_usrdat_name) and clm_force_coldstart == "off":

If I run black locally, it makes a whole lot of other changes that apparently aren't needed to pass the black check on push.

Adding a 'format ignore' (see commit titled 'trying a workaround' does not seem to resolve the issue, either.

[TeaganKing]

Issues with black-check on push are finally resolved! I think that should be all of the changes for the cime_config and config_component part of this work.

[TeaganKing]

And moving forward, is the plan to pull in the CDEPS PR, add the CDEPS tag to this PR, and then bring this on to b4b? Or does this PR need additional work, @wwieder ?

[slevis-lmwg]

I understand this PR as waiting for ESCOMP/CDEPS#262 (correct me if I'm wrong).

[TeaganKing]

@wwieder I'm thinking this PR should also include a PLUMBER defaults usermod directory that also includes the xmlchange for CLM_USRDAT_NAME to be set to PLUMBER2 for all the plumber cases?

[wwieder]

@wwieder I'm thinking this PR should also include a PLUMBER defaults usermod directory that also includes the xmlchange for CLM_USRDAT_NAME to be set to PLUMBER for all the plumber cases?

I think this is marked with a TODO in #2485? But maybe a quick conversation would be helpful? It's been a while since I thought about this.

[TeaganKing]

@wwieder I'm thinking this PR should also include a PLUMBER defaults usermod directory that also includes the xmlchange for CLM_USRDAT_NAME to be set to PLUMBER for all the plumber cases?

I think this is marked with a TODO in #2485? But maybe a quick conversation would be helpful? It's been a while since I thought about this.

Did you intend to tag another issue? I'll be in the office tomorrow, and it looks like you're onsite too-- let's chat sometime then?

[wwieder]

Sorry yes, this one that was already merged?

[TeaganKing]

Sorry yes, this one that was already merged?

Oh, I see it now! Thanks!

[TeaganKing]

I also did uncomment the following line in the last commit given that this will come in after the CDEPS PR:
./xmlchange PLUMBER2SITE='+site + '\n'

This should now be ready for review after the CDEPS PR 262 is merged in.

[TeaganKing]

The merge conflicts are fixed now, too. Some variables had already been renamed in b4b-dev to conform to snake case, and the plumber2_usermods.py file had been moved to the python directory.

[TeaganKing]

Ready for review with the caveat of the dependency of CDEPS PR 262 (as the label on this PR suggests). I can also leave this as not ready for review if that's preferred given the dependency.

[wwieder]

Thanks @TeaganKing . @slevis-lmwg do you have time to review this? I'm hosting a workshop next week.

[TeaganKing]

Ok, thanks @ekluzek for your help! The cdeps tag has been updated and this is ready for review!

[wwieder]

Looks like this PR was to B4B-Dev, which means we may need to rebase this to the latest, but could it come in next week if things look OK @ekluzek?

Note, it would be nice to merge this before the 5.3 tag gets made, which means we'd need new PLUMBER2 surface datasets to be created before the 5.3 merge.

[TeaganKing]

Thanks Will. FYI I also rebased this on Friday and brought in updates from b4b-dev that were made before then.

[ekluzek]

This is great and a nice improvement that gets us so much further along with a clean implementation of PLUMBER that works with the newer datasets. Thanks for all the work here.

The bulk of this is straightforward work that's site specific so not something to review. There's a couple points where you catch some misspellings which is awesome.

The one thing I had was to simplify the buildnml code a bit to remove some duplication. This could also be done with an issue that's done later. But, I think it's straightforward enough to do now. We could go over this also if that's helpful.

[TeaganKing]

@ekluzek thanks for reviewing. Let me know what you think about the above conversation; otherwise, your suggestions have been addressed!

Also, my comment regarding RUN_REFTOD seems to have disappeared on my end... if you can't find it, the gist was that RUN_REFTOD is set to zero for ad/postad/transient cases for plumber and START_TOD is set to a nonzero (correct) value. We could update all of the shell commands to set RUN_REFTOD instead of setting START_TOD; I'm not sure if that would cause any issues elsewhere but imagine it would be fine (just need to check). Maybe let me know if you think this is worth doing?

[ekluzek]

Cool. Awesome. Let's bring this in.