[develop]: Use yaml format for specifying experiment parameters #778
Conversation
danielabdi-noaa
requested review from
mkavulich,
gsketefian,
JeffBeck-NOAA,
RatkoVasic-NOAA,
BenjaminBlake-NOAA,
ywangwof,
chan-hoo,
panll,
christinaholtNOAA,
christopherwharrop-noaa,
mark-a-potts,
jkbk2004,
willmayfield,
dmwright526,
gspetro-NOAA,
natalie-perlin and
EdwardSnyder-NOAA
as code owners
danielabdi-noaa
force-pushed
the
feature/yaml_configs
branch
4 times, most recently
from
80651a1 to
6dc72b0
Compare
danielabdi-noaa
force-pushed
the
feature/yaml_configs
branch
8 times, most recently
from
b487aed to
5cfeb47
Compare
danielabdi-noaa
force-pushed
the
feature/yaml_configs
branch
from
7a93524 to
b7dde4a
Compare
|
I used develop branch. The details can be found in the file /scratch2/BMC/det/lpan/review/09012022/test/ufs-srweather-app/Externals.cfg. diff test/expt_dirs/test_community/2019061518/dynf000.nc test1/expt_dirs/test_community/2019061518/dynf000.nc |
|
@panll The community test case also matches bit for bit whether i use shell or a yaml config using this PR. |
|
The weather model code used for test and test1 are same, which can be verified in the file Externals.cfg. |
|
@panll I am talking about my result for yaml not matching your result for yaml. I would expect that to match unless there is a difference in ufs-weather-model build or something of that sort. |
|
@panll I think I found what is causing the difference in develop branch. The bit-for-bit difference starts from the grid generation. The change came from the very latest commit in the yaml PR that rounds this variable ush/set_gridparams_ESGgrid.py I don't need to change it again in this PR because I believe the original shell script returns rounded values anyway. |
|
@danielabdi-noaa I ran 3 WE2E tests on Hera with a version of this PR from Wednesday. They all passed, but the jobs in my crontab weren't cleared afterwards. I will rerun them with the latest version, maybe I made a mistake. |
| # Now remove trailing whitespace. | ||
| # | ||
| test_desc="${test_desc%"${test_desc##*[![:space:]]}"}" | ||
| config_fn="config.${test_name}.yaml" |
There was a problem hiding this comment.
I generated a csv file containing test info and after opening it in google sheets, I found a couple of issues. At least in some places in the test descriptions, newlines are dropped and single quotes (apostrophes) are replaced with double quotes. For example, for the test custom_GFDLgrid__GFDLgrid_USE_NUM_CELLS_IN_FILENAMES_eq_TRUE, the description in the yaml file is:
metadata:
description: >-
This test checks the capability of the workflow to have the user
specify a new grid (as opposed to one of the predefined ones in the
workflow) of GFDLgrid type. Note that this test sets the workflow
variable
GFDLgrid_USE_NUM_CELLS_IN_FILENAMES
to "FALSE"; see the UFS SRW App's User's Guide for a description of
this variable.
but in the csv file, it becomes:
This test checks the capability of the workflow to have the user specify a new grid (as opposed to one of the predefined ones in the workflow) of GFDLgrid type. Note that this test sets the workflow variable GFDLgrid_USE_NUM_CELLS_IN_FILENAMES to "FALSE"; see the UFS SRW App"s User"s Guide for a description of this variable.
So the newline/indentation is ignored and the apostrophes in App's User's Guide are changed to double quotes. Any chance we can fix that up?
There was a problem hiding this comment.
Just randomly checking other descriptions in the google sheet, I see a similar issue for the test deactivate_tasks: extra spaces added and newlines ignored. Is there a way to have yaml treat the description verbatim?
There was a problem hiding this comment.
@gsketefian Yes, I replace single quotes with double quotes, not only in we2e test description but also in comments in crontab contents. Usually you get empty crontab content but Venita's automated testing setup had crontab contents with comments that had single quotes. Please take a look at PR #816 that describs the issue and how I solved it. I didn't find a better solution than that at the time ...
There was a problem hiding this comment.
@danielabdi-noaa In this case of the test descriptions, it doesn't seem to make sense to do that replacement. What you really want to do is take the description and put it into the csv file exactly as is. Replacing apostrophes with double quotes would be very confusing to someone reading the test description in the csv. Since this is separate from the treatment of crontab lines, is it possible to not do the replacement when dealing with these descriptions? There must be a way to just take a literal string from the yaml file (even one that includes single quotes and newlines) and place it exactly as-is into the csv.
There was a problem hiding this comment.
@gsketefian I have made the we2e test description specifically (not var_defns.sh or other places) to keep existing single quotes and new lines. Pyyamls folds newlines when >- is used for multiline string, so I had to replace that with |-. Even then when you write the multiline string back with yaml.dump it will not keep the format. I needed to use a hack to make pyyaml use block format when dumping multiline strings, but seems to work fine now.
For the record though, I liked the way it was where the test description is one long line, because, in the excel sheet it will be wrapped around cell width and looks natural. While with this new fix that keeps the format first used in the config file, widening the cell has no effect. Also now it is not consistent with the test description in var_defns.sh, that must replace single quotes and newlines to work properly. In any case, please test it and let me know what you think about it.
There was a problem hiding this comment.
@danielabdi-noaa Sorry I wasn't clear about the newlines (or suggested the wrong thing). Yes, I like it to be one long string too (that's how it is now) because that's what works best in a spreadsheet, except when there is a blank line, which signifies a new paragraph or newline. Currently, if you have this in the description in the test config file (in bash):
# Hello there. This is a description of
# this test. Define a couple of variables:
#
# MY_VAR1 = "abcd"
# MY_VAR2 = "efgh${MY_VAR1}"
# MY_VAR3 = 'klmn${MY_VAR2}'
#
# Done with description.
it will show up like this in a cell in the spreadsheet:
Hello there. This is a description of this test. Define a variable:
MY_VAR1 = "abcd"
MY_VAR2 = "efgh${MY_VAR1}"
MY_VAR3 = 'klmn${MY_VAR2}'
Done with description.
So the lines without a blank line in between get concatenated into one long line, but the blank lines cause there to be a newline to be inserted. Also, the indentations for the lines MY_VAR... = ... are kept, and no changes are made to single and double quotes. Maybe there is a way to specify a newline character, e.g. \n, so it can be used only where required (i.e. where we want a blank line). Anyway, I'll leave it up to you to decide what is best. Feel free to change the |- back to >- (I actually prefer that).
|
@panll I run the community test case with the develop branch, the yaml branch with both yaml and shell config formats and they are all identical after you "fix" develop branch with the following patch in diff --git a/ush/set_gridparams_ESGgrid.py b/ush/set_gridparams_ESGgrid.py
index 722c2f27..1831ae86 100644
--- a/ush/set_gridparams_ESGgrid.py
+++ b/ush/set_gridparams_ESGgrid.py
@@ -72,8 +72,8 @@ def set_gridparams_ESGgrid(lon_ctr,lat_ctr,nx,ny,halo_width,delx,dely,pazi):
#-----------------------------------------------------------------------
#
return (lon_ctr,lat_ctr,nx,ny,pazi,halo_width,stretch_factor,
- del_angle_x_sg,
- del_angle_y_sg,
+ round(del_angle_x_sg,10),
+ round(del_angle_y_sg,10),The results are here: The develop branch is half way through the forecast but all 6 netcdf files match bit-for-bit with the others. Edit Tests have finished and all netcdf files match. I have updated the location of the test directory for future reference. |
| # | ||
| test_descs_esc_sq=() | ||
| for (( i=0; i<=$((num_tests-1)); i++ )); do | ||
| test_descs_esc_sq[$i]=$( printf "%s" "${test_descs[$i]}" | \ |
There was a problem hiding this comment.
I guess removal of this substitution is what's causing the apostrophes (single quotes) to become double quotes.
There was a problem hiding this comment.
The replacement is done whenever you produce a shell string equivalent of other formats. Bash does not like single quotes since they have been causing me problems in unexpected places (two places I already mentioned but there were of others too), so I decided to get rid of them in the config parser:
# replace some problematic chars
v1 = v1.replace("'", '"')
v1 = v1.replace("\n", " ")
# end problematic
shell_str += f"{k}='{v1}'\n"Also I replace newlines with space for the same reason, without which I would not be able to store the test description in var_defns.sh. It is debatable whether that is needed there but new lines and single quotes in strings does cause lots of problems in bash scripts.
There was a problem hiding this comment.
Here is an example var_denfs.sh file with a test description
# [metadata]
description='This test checks the capability of the workflow to have the user specify a new grid (as opposed to one of the predefined ones in the workflow) of GFDLgrid type. Note that this test sets the workflow variable GFDLgrid_USE_NUM_CELLS_IN_FILENAMES to "TRUE" (which is its default value); see the UFS SRW User"s Guide for a description of this variable. The difference between this test and the one named custom_GFDLgrid__GFDLgrid_USE_NUM_CELLS_IN_FILENAMES_eq_TRUE is that this one uses almost no stretching by setting the workflow variable GFDLgrid_STRETCH_FAC very close to 1. Setting it exactly to 1 used to cause the workflow to fail because it caused the GFDL grid generator to assume a global grid. This bug should be rechecked, e.g. by setting GFDLgrid_STRETCH_FAC to exactly 1 below. If the grid generation succeeds, then this test can be removed.'
version='1.0'
# [user]
RUN_ENVIR='community'
MACHINE='HERA'
MACHINE_FILE='/scratch2/BMC/gsd-hpcs/Daniel.Abdi/ufs-srweather-app/regional_workflow/ush/machine/hera.sh'
ACCOUNT='zrtrr'
# [platform]
WORKFLOW_MANAGER='rocoto'
NCORES_PER_NODE='40'That multiline description would cause problems if it had "single quotes" or "new line" characters.
There was a problem hiding this comment.
Ok, I see. I didn't know the test description goes in var_defns.sh. It didn't used to and it's not necessary since it's not used by any tasks (as far as I know), but that's fine if you feel it should be there. And yes, it would be a problem to have single quotes and newlines in there. I wonder if you can escape them. But I'll leave it up to you how you want to handle it (also see my other comment above in this file).
It should clear them. I have run 68 we2e test cases simultaneously on Hera and my crontab was empty after all is done. Sorry I closed this PR by accident, pressed "closed with comment" instead of "comment" |
| - 0 | ||
| FCST_LEN_HRS: 6 | ||
| PREEXISTING_DIR_METHOD: rename | ||
| task_run_fcst: |
There was a problem hiding this comment.
It would be nice to list the different task dictionaries in the order in which they appear in the xml, both in these test config files and in config_defaults.yaml, i.e.
task_make_grid
task_make_orog
task_make_sfc_climo
task_get_extrn_ics
task_get_extrn_lbcs
task_make_ics
task_make_lbcs
task_run_fcst
task_run_post
task_get_obs_ccpp
task_get_obs_mrms
task_get_obs_ndas
task_run_vx_... (the vx tasks are still changing)
There was a problem hiding this comment.
@gsketefian I have made the change you suggested to both config_defaults.yaml and the we2e config files.
Will push later once I run some tests.
danielabdi-noaa
added
the
ci-hera-intel-WE
venitahagerty
removed
the
ci-hera-intel-WE
|
@danielabdi-noaa We discussed this PR in the DTC meeting just now and we are comfortable merging the PR today provided we can ensure bit-for-bit equivalence. Is it possible to modify your code to do this fairly easily? My understanding in the problem is simply in a truncation of the digits in the regional_grid.nml; can you modify your code to use the full, un-truncated precision as currently occurs in develop? We understand that this precision isn't scientifically relevant, but given the size of the PR if we can ensure there is bit-for-bit equivalence that would make us much more comfortable with the merge. |
|
@mkavulich Ok thanks, I have just pushed a commit that uses un-trunctated values for the grid parameters. This change in the yaml PR came in very recently so all the tests I did before used un-truncated values and were matching develop bit-for-bit. Here, you can find my test results with 9 test cases, and 5 different formats = 35 runs. The shell workflow is run with develop branch: The forecast output (netcdf files) match for all 5 formats per test. Yesterday, I have run the community test case, but with truncated versions for both There are three test cases in there run with The original shell script does truncation to 10 digits like this: The python version of develop keeps all digits because I thought it didn't hurt to be more accurate. I will run more tests if necessary. Just to make sure nothing is changed after the last commit i.e. with a non-truncated version, I run one more test on Hera, and it matches bit-for-bit |
| @@ -1,10 +1,13 @@ | |||
| #!/usr/bin/env python3 | |||
|
|
|||
| from textwrap import dedent | |||
|
|
|||
There was a problem hiding this comment.
@danielabdi-noaa Just wondering why you still need this script. The bash version of this script was something I had written to handle arguments for bash scripts/functions. Doesn't argparse take care of arguments now? (I would grep for this myself but Hera is down at the moment.)
| @@ -0,0 +1,2252 @@ | |||
| #---------------------------- | |||
There was a problem hiding this comment.
@danielabdi-noaa Can you describe a bit how/where the different sections (dictionaries) in the config file (is it called config.yaml?) are accessed? I think the idea behind dividing the variables into sections was to only use those sections that are necessary for a given rocoto task. Where are those determined for each task? Thx.
There was a problem hiding this comment.
@gsketefian There is this now closed PR #789 that did what you described. The variables needed only for the specific task being run are sourced. I may revive it once the dust is settled on this PR.
There was a problem hiding this comment.
I tested it on Jet. (Hera is under maintenance today). It's byte-for-byte match now. Thanks! @danielabdi-noaa @JeffBeck-NOAA @mkavulich
|
@panll Thanks for testing on Jet! I did one more test on Hera, now that it is up, including the last change. The results match bit-for-bit located here: Comparing checksums |
There was a problem hiding this comment.
Approving based on @linlin-pan's testing and @gsketefian's review.
DESCRIPTION OF CHANGES:
Use structured yaml configs for specifying experiment parameters. This helps in better management of the recent proliferation of tasks and their associated parameters. However, one can still use shell scripts for configuring experiments by changing config script extension to
shinconfig_defaults.yamlFurthermore, a utility for converting shell config scripts to yaml config is provided.
To convert a shell config script to yaml config script, for example
config.community.shTo check validity of existing yaml or shell config
TESTS CONDUCTED:
I have run tests successfully on HERA, JET, GAEA and CHEYNNE gnu+intel
GAEA and CHEYNNE tested through Jenkins shown in this page.
Out of 56 tests
HERA
0 failures
JET
5 failures
DEPENDENCIES:
None
DOCUMENTATION:
None
ISSUE:
To be opened later
CONTRIBUTORS:
@christinaholtNOAA