[develop] Use uwtools instead of set_namelist #1054
Conversation
christinaholtNOAA
requested review from
mkavulich,
gsketefian,
JeffBeck-NOAA,
RatkoVasic-NOAA,
BenjaminBlake-NOAA and
ywangwof
as code owners
parm/FV3.input.yml
Outdated
| @@ -83,7 +78,7 @@ FV3_HRRR: | |||
| <<: *RRFS_v1beta_phys | |||
| cdmbgwd: [3.5, 1.0] | |||
| do_mynnsfclay: True | |||
| do_sfcperts: !!python/none | |||
| do_sfcperts: | |||
There was a problem hiding this comment.
The latest version of pyyaml did not need/want this tag.
There was a problem hiding this comment.
Does this represent a potential back-compatibility issue for any yaml config files? If so it should probably be documented.
There was a problem hiding this comment.
@gspetro-NOAA do you have a suggestion where this kind of information can/should go? Perhaps release notes would be sufficient, but in the absence of a release, do we have an ongoing list of changes since the release?
There was a problem hiding this comment.
I also got a nice off-line suggestion to use null as the value here instead of leaving it blank. It will be more explicit and not make unfamiliar users feel like it might have been a mistake or oversight.
There was a problem hiding this comment.
@christinaholtNOAA For what it's worth, issue #981 contains all of the PRs and their commit messages since the SRW v2.2.0 release.
There was a problem hiding this comment.
@christinaholtNOAA There's also a "Known Issues" page for issues related to develop or general issues. Known issues related to a specific release can be added to the appropriate release page in the Known Issues section.
| 'lx': ${NEG_NX_OF_DOM_WITH_WIDE_HALO}, | ||
| 'ly': ${NEG_NY_OF_DOM_WITH_WIDE_HALO}, | ||
| 'pazi': ${PAZI}, | ||
| } |
There was a problem hiding this comment.
Several of these blocks show up, and this is to ensure that the resulting input file for the tool is proper YAML.
| for k, v in values.copy().items(): | ||
| if v is None: | ||
| del base_namelist[sect][k] | ||
| base_namelist.dump(FV3_NML_FP) |
There was a problem hiding this comment.
The set_namelist tool handled all of the steps now included here with the help of the uwtools API.
| @@ -1,32 +1,28 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
This file was linted to PEP8 standards adopted by SRW. That includes its new lower case naming convention required of Python modules.
| "-c", "--cdate", | ||
| dest="cdate", | ||
| required=True, | ||
| type=lambda d: dt.datetime.strptime(d, '%Y%m%d%H'), |
There was a problem hiding this comment.
This line does what str_to_type handles, but with less ambiguity.
| "FV3_NML_FP", | ||
| "PARMdir", | ||
| "RUN_ENVIR", | ||
| ] |
There was a problem hiding this comment.
Here, I'm limiting the namespace of global variables this script requires from all the ones in the general sections of the experiment config file.
| fixed_cfg = load_config_file(os.path.join(PARMdir, "fixed_files_mapping.yaml")) | ||
| IMPORTS = ["SFC_CLIMO_FIELDS", "FV3_NML_VARNAME_TO_SFC_CLIMO_FIELD_MAPPING"] | ||
| import_vars(dictionary=flatten_dict(fixed_cfg), env_vars=IMPORTS) | ||
| fixed_cfg = get_yaml_config(os.path.join(PARMdir, "fixed_files_mapping.yaml"))["fixed_files"] |
There was a problem hiding this comment.
Use uwtools to get the YAML config, and appropriate sub-section.
| nml_var_name = tup[0] | ||
| sfc_climo_field_name = tup[1] | ||
| for mapping in fixed_cfg["FV3_NML_VARNAME_TO_SFC_CLIMO_FIELD_MAPPING"]: | ||
| nml_var_name, sfc_climo_field_name = re.search(regex_search, mapping).groups() |
There was a problem hiding this comment.
Removing some magic numbers.
| settings = | ||
|
|
||
| {settings_str} | ||
| """ |
There was a problem hiding this comment.
It was already an f-string. Treat it as such.
parm/FV3.input.yml
Outdated
| @@ -83,7 +78,7 @@ FV3_HRRR: | |||
| <<: *RRFS_v1beta_phys | |||
| cdmbgwd: [3.5, 1.0] | |||
| do_mynnsfclay: True | |||
| do_sfcperts: !!python/none | |||
| do_sfcperts: | |||
There was a problem hiding this comment.
Does this represent a potential back-compatibility issue for any yaml config files? If so it should probably be documented.
|
|
||
| uw config realize \ | ||
| -i ${tmpfile} \ | ||
| ) | uw config realize \ |
There was a problem hiding this comment.
Just FYI, since a settings variable is defined (unlike in my previous example, which had no intermediate variable), I believe you can also just:
echo "$settings" | uw config realize ...
without the ( cat ... ) subshell command, e.g.
$ export VAR1=foo
$ export VAR2=bar
$ settings="
config:
foo: $VAR1
bar: $VAR2
"
$ echo "$settings" | uw config realize --input-format yaml --output-format nml
&config
foo = 'foo'
bar = 'bar'
/
There was a problem hiding this comment.
These changes overall look good to me! Thank you for updating the version of uwtools and replacing set_namelists with the uw config realize option! I have successfully ran the fundamental tests on Hercules:
----------------------------------------------------------------------------------------------------
Experiment name | Status | Core hours used
----------------------------------------------------------------------------------------------------
grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta_2 COMPLETE 14.32
nco_grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_timeoffset_suite_ COMPLETE 17.86
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v15p2_20240 COMPLETE 9.64
grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v17_p8_plot COMPLETE 20.06
grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_HRRR_suite_HRRR_2024031 COMPLETE 28.27
grid_SUBCONUS_Ind_3km_ics_HRRR_lbcs_RAP_suite_WoFS_v0_20240315095 COMPLETE 22.15
grid_RRFS_CONUS_25km_ics_NAM_lbcs_NAM_suite_GFS_v16_2024031509565 COMPLETE 34.74
----------------------------------------------------------------------------------------------------
Total COMPLETE 147.04
and ran all of the verification tests on Hera:
----------------------------------------------------------------------------------------------------
Experiment name | Status | Core hours used
----------------------------------------------------------------------------------------------------
MET_ensemble_verification_only_vx_time_lag_20240315142246 COMPLETE 3.65
MET_ensemble_verification_only_vx_20240315142250 COMPLETE 1.00
MET_ensemble_verification_winter_wx_20240315142252 COMPLETE 111.73
MET_ensemble_verification_20240315142254 COMPLETE 20.18
MET_verification_only_vx_20240315142257 COMPLETE 0.23
MET_verification_winter_wx_20240315142258 COMPLETE 12.20
MET_verification_20240315142300 COMPLETE 10.38
----------------------------------------------------------------------------------------------------
Total COMPLETE 159.37
An AQM test is also underway on Hera.
Is there a reason that the header for exregional_run_met_pb2nc_obs.sh was changed from /usr/bin/env bash to /bin/bash? The /usr/bin/env bash method should be more robust, so isn't clear why this modification was made (and why only for this one script, the rest of the scripts still use /usr/bin/env bash). Any clarification would be greatly appreciated.
There was a problem hiding this comment.
Thank you very much for putting back the /usr/bin/env bash in scripts/exregional_run_met_pb2nc_obs.sh!
The AQM test successfully passed:
----------------------------------------------------------------------------------------------------
Experiment name | Status | Core hours used
----------------------------------------------------------------------------------------------------
aqm_grid_AQM_NA13km_suite_GFS_v16_20240315173059 COMPLETE 2787.88
----------------------------------------------------------------------------------------------------
Total COMPLETE 2787.88
Merging in PR #1055 has caused a conflict in parm/FV3.input.yml. Please merge the latest HEAD in develop to your branch, then I will launch the automated Jenkins tests for this PR.
Thanks!
MichaelLueken
added
the
run_we2e_coverage_tests
|
Manual testing of the WE2E coverage tests on Hera Intel have successfully completed: |
|
The manual Hera GNU WE2E coverage tests successfully passed on Rocky8: The manual launched fundamental tests successfully passed on Orion: The automated Gaea and Hercules tests successfully passed. On Jet, the On Derecho, the I have also opened PR #3 in your fork. This PR will change the Hera spack-stack from CentOS to Rocky8. Once PR #3 has been merged and the issue with |
[replace_set_namelist] Update Hera spack-stack locations to use Rocky8
|
The Derecho WE2E coverage tests are now passing: The Gaea WE2E coverage tests were also ran to ensure that the deterministic stochastic test still functions. All tests successfully passed: Moving forward with merging this PR now. |
DESCRIPTION OF CHANGES:
Continues the integration of the uwtools package. In this PR, I've done the following:
Type of change
TESTS CONDUCTED:
The final test is still running (slowly) after updating to the latest version of uwtools. I don't expect trouble out of it give our changes between uwtools v2.0.1 and v2.1.0 were backwards compatible and this PR worked as expected on Hera with uwtools v.2.0.1.
DEPENDENCIES:
n/a
DOCUMENTATION:
I don't think so, but could have missed something.
ISSUE:
n/a
CHECKLIST