Enhance ability to use template variables by gsketefian · Pull Request #650 · ufs-community/regional_workflow

gsketefian · 2021-12-13T19:19:46Z

DESCRIPTION OF CHANGES:

Enhance ability to use template variables in the experiment configuration file (either in the default configuration file config_defaults.sh or the user configuration file config.sh).
Modify WE2E test system to include test of template variable use.
Fix bugs.

Notes on template variables:

A template variable (or simply a template) is an experiment variable that contains in its definition a reference to another variable(s). The referenced variable can be another experiment variable (i.e. one that is defined in var_defns.sh), or it can be a local variable (i.e. one that is not defined in var_defns.sh but in the script or function that sources var_defns.sh and uses the template). For example, a template named TEMPL_VAR my be defined in config_defaults.sh or config.sh as

TEMPL_VAR='cd ${some_dir}'

where some_dir may be an experiment variable or a local variable. TEMPL_VAR can then be evaluated using bash's eval built-in command in a script or function that first sources var_defns.sh and, if necessary, defines some_dir. Note that single quotes must be used on the right-hand side to avoid expansion of ${some_dir} before run time (i.e. when eval is called on TEMPL_VAR). For details, see the documentation added in PR #198.

Changes to WE2E tests:

Modify the WE2E test configuration file config.deactivate_tasks.sh to include template variables. deactivate_tasks now serves as a test of both deactivating tasks and of using template variables.
Add template_vars as an alternate test name for deactivate_tasks (by creating a symlink named config.template_vars.sh that points to config.deactivate_tasks.sh).

Bug fixes:

In get_WE2Etest_names_subdirs_descs.sh, change the variable alt_test_subdirs to alt_test_names at a single location.
In setup.sh, set BUILD_ENV_FN and WFLOW_ENV_FN (instead of in load_modules_run_task.sh and launch_FV3LAM_wflow.sh, respectively). This way, these variables will have the correct values in var_defns.sh.
In get_expts_status.sh, fix the way homerrfs is calculated.

TESTS CONDUCTED:

The WE2E tests grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v15p2 and template_vars were run on Hera. Both completed successfully.

DOCUMENTATION:

Documentation is added to the User's Guide via PR #198 into the ufs-srweather-app repo.

Dependencies:

PR #198 for the documentation.

CONTRIBUTORS:

@christinaholtNOAA and @mkavulich brought up the issue of templates as part of PR #617.

…e new experiment variable DEBUG is set to TRUE (as opposed to VERBOSE being set to TRUE).

…nt variable DEBUG is set to TRUE (as opposed to VERBOSE being set to TRUE). Also, for brevity, rename variable file_full_path to file_fp.

…stead of VERBOSE to reduce clutter in output.

…d/or config.sh, i.e. variables whose definitions contain other variables, e.g. MY_CMD="ls \${SOME_DIR}".

Conflicts: ush/setup.sh

…of launch_FV3LAM_wflow.sh and load_modules_run_task.sh) so that the correct values get recorded to the experiment's var_defns.sh file.

…ed to allow for template variables, i.e. variables that are defined in terms of other variables in config_defaults.sh and config.sh, e.g. MY_VAR="${ANOTHER_VAR}".

…rs' into feature/template_vars

…acter that indicates a reference to a bash variable in a string (this character is the dollar sign). Then, in check_var_valid_value.sh, use this new variable to skip checking for valid values if a variable's value contains this character.

1) Introduce new utility function get_bash_file_contents() to get the contents of a bash script/function with comment lines, empty lines, and leading and trailing whitespace removed. This will be used to extract the contents of the default and user-specified experiment configuration files. 2) In compare_config_scripts.sh: a) Replace code with the new function get_bash_file_contents(). b) Clean up comments and output messages. c) Use input arguments to specify the paths/names to the default and user-specified experiment configuration files. 3) Rename file compare_config_scripts.sh (and its function within) to check_expt_config_vars(.sh). 4) Replace code in setup.sh with the new function get_bash_file_contents().

…test use of template variables in the experiment configuration file. Include detailed comments on how template variables can be used. Add a alternate test (symlink) named template_vars that points to the deactivate_tasks test.

…HAR.

christinaholtNOAA

Gerard, I've left several additional comments and questions. I am also now considering that it could be more useful to hold off on introducing support for templating all config variables. The implementation of this feature seems to add a level of complexity that may hinder the configuration of the system more than help us. When I'd mentioned needing to use a few templated configuration variables in a previous PR, I was already working with a solution that had MUCH less overhead for the problem at hand when separating out machine-dependent files. In general, we do not need all config variables to be templated, but it could be helpful for a select few to work that way so that we can, for instance, supply the templated name of a cycle-dependent input file.

I think it could be helpful to consider a transition to python and YAML sooner rather than later for managing configuration, and we could mitigate the need for jumping through so many hoops to make bash work in a way python is already great with.

christinaholtNOAA · 2021-12-16T18:55:59Z

+#
+RUN_CMD_UTILS='cd \$yyyymmdd'
+RUN_CMD_FCST='mpirun -np \${PE_MEMBER01}'
+RUN_CMD_POST='\$( echo hello \$yyyymmdd )'


It's unclear to me how this test will pass/fail. It certainly wouldn't run for a WE2E test where commands are actually submitted. Is it a smoke test to make sure a var_defns.sh file comes out? Are the contents of that file checked for the right answer?

Yes, it is a test to make sure the templated variables are placed towards the end of var_defns.sh. I actually wrote some other code to make sure that the commands in these variables can be evaluated and executed properly (in as much as it makes sense to evaluate them), and they do. I did not have a good case of how these would be used in an actual application, so I left it at that. If you have a use case we can include as a WE2E test, that would be great.

How is this test run?

christinaholtNOAA · 2021-12-17T18:39:41Z

+  contents=""
+  while read crnt_line; do
+    contents="${contents}${crnt_line}
+"


This line seems like it needs a line continuation, maybe?

It is actually correct; I tested it. It is just adding a newline after appending ${crnt_line} to contents. Adding a line continuation will cause the newline to be skipped, and all newlines will be missing from contents (making it one super long line, which isn't what we want). An alternate and maybe clearer way to append a new line to contents is

printf -v "contents" "${contents}${crnt_line}\n"

This is a bit slower since it calls printf for every line that's read in. Either way is ok with me.

christinaholtNOAA · 2021-12-17T18:42:00Z

+#
+#-----------------------------------------------------------------------
+#
+VARVALUE_REF_CHAR="\$"


I think it's much clearer just to use the standard bash syntax where it's needed, instead of building this level of re-direction around something that most folks understand in bash.

christinaholtNOAA · 2021-12-17T18:44:07Z

-machine=$(echo_lowercase $MACHINE)
-env_fn=${WFLOW_ENV_FN:-"wflow_${machine}.env"}
-env_fp="${SR_WX_APP_TOP_DIR}/env/${env_fn}"
+env_fp="${SR_WX_APP_TOP_DIR}/env/${WFLOW_ENV_FN}"


Ahhh. I couldn't find where it was set in my last pass. I needed to expand the diffs in setup.sh to find it. Thanks for the clarification! :)

christinaholtNOAA · 2021-12-17T18:49:05Z

+# (i.e. dummy) values for various parameters.  The experiment generation 
+# scripts copy these templates to appropriate locations in the experiment 
+# directory (either the top of the experiment directory or one of the 
+# cycle subdirectories) and replace the placeholders in these copies by 


Experiment generation no longer creates cycle subdirectories, if I remember correctly. This comment should be updated to reflect that.

christinaholtNOAA · 2021-12-17T18:50:19Z

+# cycle subdirectories) and replace the placeholders in these copies by 
+# actual values specified in the experiment configuration file (or derived 
+# from such values).  The scripts then use the resulting "actual" files 
+# as inputs to the forecast model.


All of them are "actual" files -- templates and run-time ready ones. Maybe "The scripts move the configured files from the experiment directory to the run directory at run time."

christinaholtNOAA · 2021-12-17T18:50:58Z

+# that need to be replaced according to the experiment configuration.  
+# If using CCPP, this file simply needs to be copied over from its 
+# location in the forecast model's directory structure to the experiment 
+# directory.


There's no longer an option NOT to use CCPP, so this logic comment is no longer clear.

Modified this paragraph. Hopefully addresses all three concerns here.

christinaholtNOAA · 2021-12-17T18:57:29Z

+# be expanded/evaluated when the variable definitions file is sourced.
+# In this case, unexpected behavior or failure may be encountered if the
+# referenced variables are not already defined.  To cover this scenario,
+# we place the templates all at the end of the variable definitions file.


This seems like too much logic for what functionality we need. Currently there are very few variable paths that need to be expanded at run time instead of at configuration time. By that, I mean that to date, we (rightly) don't propagate variable references that should be expanded to the var_defns.sh file.

If we limit users to only variables we expect to contain templates (run-time specific ones), then we don't have to handle the situation where ALL variables could potentially contain a template. This logic becomes unnecessary and a simplified understanding and implementation.

For example, we document that templates are only supported for RUN_COMMAND_* and EXTRN_MODEL_PATH (or whatever) directory variables. Then, we know that we only have the expectations to expand THOSE variables at run-time. Nothing else needs to change.

The reason for writing templated variables at the end of the var_defns.sh file is to make sure that any variables that they reference are already defined above in that file. This was the problem @mkavulich was running into when templating RUN_CMD_FCST, so he had to put in code to specifically remove the line originally defining RUN_CMD_FCST (at which point PE_MEMBER01 was not yet defined) and then insert it later in var_defns.sh.

This code is just identifying which variables are templated. It would be pretty straightforward to add an if-statement in the loop below to allow only a select few variables to be templated.

The templated variables that we need may not have their values defined in the var_defns.sh file, though. They may need to be expanded at run time, another reason that it's important to limit specific ones to be templated, and handle those specifically in the run scripts. Its irrelevant to the generation of the var_defns.sh file whether the variable is a template or not when we can assume that only specific variables are templates, and those are handled appropriately at run time.

The ones I am thinking of currently are templates for location of cycle-specific files on disk. Also, run commands for the platforms that use "nprocs" for their run command. In those cases, nprocs is computed in the run script, and could be different for each task, so should only be filled in there. PE_MEMBER01 is another one that could be filled in in var_defns.sh for the RUN_CMD, but wouldn't necessarily need to be if we expand all RUN_CMDS at run time (because of the aforementioned treatment of nprocs).

Are there others that need to be templated that you can think of given that the current system strictly does not allow template variables?

christinaholtNOAA · 2021-12-28T14:35:59Z

-NDIGITS_ENSMEM_NAMES="${NDIGITS_ENSMEM_NAMES}"
-ENSMEM_NAMES=( $( printf "\"%s\" " "${ENSMEM_NAMES[@]}" ))
-FV3_NML_ENSMEM_FPS=( $( printf "\"%s\" " "${FV3_NML_ENSMEM_FPS[@]}" ))
+SR_WX_APP_TOP_DIR=\"${SR_WX_APP_TOP_DIR}\"


Do we need the quotes to show up in config file here? All of these variables would be expanded to include their final results in at this point, if we consider limiting the ability to use templates to a handful of variables where they are needed.

I generally just double-quote the value (note: double-quoting doesn't prevent expansion of the value; single-quotes do) since it seems to be the recommended approach in online forums. If a variable contains whitespace (I know most of these don't), the double quotes will keep the whitespace (and anything following) as part of the value (otherwise, will likely get an error). Also, it looks nicer with syntax coloring, at least in vi/vim.

By doing this explicitly, none of these values CAN be templates. They WILL be expanded once this file is sourced, even if they aren't expanded when the file is written, and we definitely want to preserve templates until run time when they should be filled in.

For example, if you wanted log files to be written to a date-specific location, which is something that is useful for real-time runs, we would want to preserve that template and would need to track down where to remove double quotes for expansion.

Instead, we probably should remove these double quotes completely unless a given argument is allowed to contain white space. It looks like the case where there are multiple items with white space already DO NOT have double quotes and are handled appropriately as strings aside from double quotes.

Please consider removing all the double quotes here for clarity in how we are able to introduce templated variable values in the future.

gsketefian · 2022-01-06T22:47:22Z

Gerard, I've left several additional comments and questions. I am also now considering that it could be more useful to hold off on introducing support for templating all config variables. The implementation of this feature seems to add a level of complexity that may hinder the configuration of the system more than help us. When I'd mentioned needing to use a few templated configuration variables in a previous PR, I was already working with a solution that had MUCH less overhead for the problem at hand when separating out machine-dependent files. In general, we do not need all config variables to be templated, but it could be helpful for a select few to work that way so that we can, for instance, supply the templated name of a cycle-dependent input file.

I think it could be helpful to consider a transition to python and YAML sooner rather than later for managing configuration, and we could mitigate the need for jumping through so many hoops to make bash work in a way python is already great with.

As I mentioned in one of my replies, it would at this point be quite straightforward to add an if-statement in the loop in setup.sh where templated variables are identified to allow only a certain subset of variables to be templated. And yes, I'm ok with using a python solution for configuration, although it would be good to talk about that with the rest of the DTC team so they're all on-board. But if we could get this PR in, that would be great.

JeffBeck-NOAA · 2022-01-07T00:08:51Z

Gerard, I've left several additional comments and questions. I am also now considering that it could be more useful to hold off on introducing support for templating all config variables. The implementation of this feature seems to add a level of complexity that may hinder the configuration of the system more than help us. When I'd mentioned needing to use a few templated configuration variables in a previous PR, I was already working with a solution that had MUCH less overhead for the problem at hand when separating out machine-dependent files. In general, we do not need all config variables to be templated, but it could be helpful for a select few to work that way so that we can, for instance, supply the templated name of a cycle-dependent input file.
I think it could be helpful to consider a transition to python and YAML sooner rather than later for managing configuration, and we could mitigate the need for jumping through so many hoops to make bash work in a way python is already great with.

As I mentioned in one of my replies, it would at this point be quite straightforward to add an if-statement in the loop in setup.sh where templated variables are identified to allow only a certain subset of variables to be templated. And yes, I'm ok with using a python solution for configuration, although it would be good to talk about that with the rest of the DTC team so they're all on-board. But if we could get this PR in, that would be great.

@gsketefian @christinaholtNOAA, I reviewed and approved this PR, since it looks like Christina's concerns have been resolved. Transitioning to a purely Python and YAML-based configuration system would make sense, since we've already set that up for other components of the workflow. However, given the rather large footprint of that work, a future PR would be more appropriate. Since variable templating is desired soon, I think we should get this PR (and Christina's related PR) merged and then work on a larger migration to Python afterward.

christinaholtNOAA

Gerard, I feel as though this solution is still not quite capturing what we need for being able to use a template as a value in a variable.

It will be helpful if we can specify a variable that contains more generic templated information that is not expanded until run time, and to limit those variables to a small handful that we know might need to contain processor counts or date-time information, e.g.. This can be as simple as not using double quotes to expand variables by default, and to just have them show up in var_defns.sh as they are set by the scripts and configure files.

It seems that the solution here is attempting to engineer the capability to specify a variable that reference any other known variable and expand it in the var_defns.sh file. Bash already does this for us by default if we are careful about the order in which variables are specified, and we are. If we aren't careful, we have the set -u feature of bash that will readily call us out to fix our mistake.

To date, the setup.sh file expands everything before writing any of it to the var_defns.sh file, leaving no references to other variables, which is great and as it should be. I was proposing in a comment of a previous PR that If we assume that everything will be expanded before writing the variables to var_defns.sh file, then we can write those values with single quotes and nothing changes, except our ability to write a template value to that file and preserve it until run time when we want to fill it in with task- and configuration-specific information.

Here's an example of what I mean:

HOMErrfs="/some/path/on/disk"
EXTRN_MDL_SYSBASEDIR_ICS="/some/path/to/${YYYYMMDD}/${HH}"

Gets expanded to an unuseful path when sourced:

HOMErrfs="/some/path/on/disk"
EXTRN_MDL_SYSBASEDIR_ICS="/some/path/to//"

However, if we use single quotes for everything, we lose nothing from how we were already doing things and we gain the ability to template some fields

HOMErrfs='/some/path/on/disk'
EXTRN_MDL_SYSBASEDIR_ICS='/some/path/to/${YYYYMMDD}/${HH}'

We can then use $(eval echo $EXTRN_MDL_SYSBASEDIR_ICS) once we've pinned down what YYYYMMDD and HH should be and have a nice path like /some/path/to/20210103/12

christinaholtNOAA · 2022-01-06T23:27:56Z

+#
+RUN_CMD_UTILS='cd \$yyyymmdd'
+RUN_CMD_FCST='mpirun -np \${PE_MEMBER01}'
+RUN_CMD_POST='\$( echo hello \$yyyymmdd )'


How is this test run?

christinaholtNOAA · 2022-01-06T23:45:31Z

+# be expanded/evaluated when the variable definitions file is sourced.
+# In this case, unexpected behavior or failure may be encountered if the
+# referenced variables are not already defined.  To cover this scenario,
+# we place the templates all at the end of the variable definitions file.


The templated variables that we need may not have their values defined in the var_defns.sh file, though. They may need to be expanded at run time, another reason that it's important to limit specific ones to be templated, and handle those specifically in the run scripts. Its irrelevant to the generation of the var_defns.sh file whether the variable is a template or not when we can assume that only specific variables are templates, and those are handled appropriately at run time.

The ones I am thinking of currently are templates for location of cycle-specific files on disk. Also, run commands for the platforms that use "nprocs" for their run command. In those cases, nprocs is computed in the run script, and could be different for each task, so should only be filled in there. PE_MEMBER01 is another one that could be filled in in var_defns.sh for the RUN_CMD, but wouldn't necessarily need to be if we expand all RUN_CMDS at run time (because of the aforementioned treatment of nprocs).

Are there others that need to be templated that you can think of given that the current system strictly does not allow template variables?

christinaholtNOAA · 2022-01-06T23:47:47Z

+      printf -v "var_defns_notempl" "${var_defns_notempl}${var_defn}\n"
+    else
+      printf -v "var_defns_templ" "${var_defns_templ}${var_defn}\n"
+    fi


This is not necessary because the whole point is that we don't want to fill in templates in this file!

christinaholtNOAA · 2022-01-06T23:56:24Z

-NDIGITS_ENSMEM_NAMES="${NDIGITS_ENSMEM_NAMES}"
-ENSMEM_NAMES=( $( printf "\"%s\" " "${ENSMEM_NAMES[@]}" ))
-FV3_NML_ENSMEM_FPS=( $( printf "\"%s\" " "${FV3_NML_ENSMEM_FPS[@]}" ))
+SR_WX_APP_TOP_DIR=\"${SR_WX_APP_TOP_DIR}\"


By doing this explicitly, none of these values CAN be templates. They WILL be expanded once this file is sourced, even if they aren't expanded when the file is written, and we definitely want to preserve templates until run time when they should be filled in.

For example, if you wanted log files to be written to a date-specific location, which is something that is useful for real-time runs, we would want to preserve that template and would need to track down where to remove double quotes for expansion.

Instead, we probably should remove these double quotes completely unless a given argument is allowed to contain white space. It looks like the case where there are multiple items with white space already DO NOT have double quotes and are handled appropriately as strings aside from double quotes.

Please consider removing all the double quotes here for clarity in how we are able to introduce templated variable values in the future.

gsketefian · 2022-01-07T15:30:38Z

@christinaholtNOAA After skimming through your comments, I think we're close to a solution. The single vs. double quotes was something I was grappling with as well when making this PR. Let me read through them more carefully and get back to you. Thanks.

christinaholtNOAA · 2022-01-07T16:07:57Z

@JeffBeck-NOAA We are working at NOAA GSL toward a plan for merging capabilities for RRFS DA into develop. That will inevitably include a transition to Python + YAML and we need to be in regular coordination about that effort. Please do not move forward on that without first discussing details with our group.

JeffBeck-NOAA · 2022-01-07T16:55:04Z

@JeffBeck-NOAA We are working at NOAA GSL toward a plan for merging capabilities for RRFS DA into develop. That will inevitably include a transition to Python + YAML and we need to be in regular coordination about that effort. Please do not move forward on that without first discussing details with our group.

@christinaholtNOAA, since you've contributed similar features in the past, I was going to refer to you for that development, so no worries.

gsketefian · 2022-01-07T23:39:21Z

@christinaholtNOAA Since the comments/replies above are getting convoluted, I'll reply to you here. Let me know if I miss anything.

It seems that the solution here is attempting to engineer the capability to specify a variable that reference any other known variable and expand it in the var_defns.sh file.

Yes, that was my intent. I was trying to allow for the case @mkavulich was struggling with that used double-quotes, i.e.

RUN_CMD_FCST="mpirun -n \${PE_MEMBER01}"

and also to allow the referenced variable (PE_MEMBER01 here) to be expanded either in var_defns.sh or at run time. That's just to cover as many use cases as possible. But I'm ok with narrowing things down so that variable expansion happens only at runtime (by requiring that templates always use single quotes). Then it would be unnecessary to move the template variables to the end of var_defns.sh. It also makes it unnecessary to escape the $, which is good because it makes it simpler for the user.

Are there others that need to be templated that you can think of given that the current system strictly does not allow template variables?

Nope!

Finally, it would be good to have a WE2E test that properly tests the templating capability, but I cannot think of one without changing one of the J-jobs or ex-scripts to include a call like eval ${RUN_CMD_...} that is only for testing purposes. If you have a simple case we can use, that would be great. Otherwise, we can leave out the WE2E test for now and maybe you can add one when you're modifying the scripts in a future PR such that they use template variables.

I do have a couple of additional questions:

How do you propose to determine whether a templated variable is one of the ones allowed to be a template? Have an "allowed" list to compare against?
Should we be concerned that a (malicious) 3rd party may give a user a config.sh file to run that contains something like RUN_CMD_FCST=' rm -rf *? Or do we just leave that up to the user to catch?

I'll work on the code adjustments to this PR early next week. Please let me know if I missed anything. Thanks.

… variables will be expanded only at runtime (not within var_defns.sh). Details: * Removed code in setup.sh that places template variables at the end of the variable definitions file (var_defns.sh). This is no longer needed because templates will not be expanded within var_defns.sh. * Quoting of variable values in var_defns.sh: * Place all primary variable values in var_defns.sh in single quotes. If this is not done, bash will try to expand template variables, usually resulting in a "variable undefined" error. * For consistency with the use of single quotes for the primary variables in var_defns.sh, also place values of all secondary/derived variables in that file in single quotes. If this is not done, at least one variable (CRONTAB_LINE) that contains whitespace will cause an error. * Remove unnecessary presence of backslash escape characters in the definitions of the template variables in the WE2E test config.template_vars.sh. * Clean up code comments.

gsketefian · 2022-01-10T17:22:44Z

@christinaholtNOAA Just pushed a commit to simplify the logic in setup.sh.

Note that I ended up using single quotes when writing all variables to var_defns.sh. If this is not done, template variables cause an error because bash tries to expand their values, usually with "undefined variable" errors. If we want to use single quotes only around templates, then there needs to be an if-statement in the loop over the variables in setup.sh that picks out the template variables. That was more complicated, so I went with the simpler solution. Also, one of the secondary/derived variables (CRONTAB_LINE) must be placed in quotes since it contains whitespace (otherwise there's an error). For consistency throughout var_defns.sh, I put all secondary variables in single quotes as well (and the syntax coloring of var_defns.sh in vi/vim is much easier to read this way).

Please let me know what you think. Thanks.

christinaholtNOAA

@gsketefian Thanks for making those changes. I think this is a super useful feature that will help out with how we can achieve machine independence. I appreciate the willingness to help and make it what we need for this project.

I wanted to respond to your earlier questions.

As for tests with this, I think they can be covered by the existing tests in the next couple of iterations of PRs since we haven't actually implemented a use case for one of these templates just yet.

How do you propose to determine whether a templated variable is one of the ones allowed to be a template? Have an "allowed" list to compare against?

I think it would be acceptable to just document them for now. It's a relatively obscure change that was made to existing behavior and since we're treating all variables the same, I don't foresee any issues that might arise from compatibility with older config files and new scripts. If it's a feature that starts causing us trouble in the future, we could build a check to make sure that templates aren't specified where we don't expect them.

Should we be concerned that a (malicious) 3rd party may give a user a config.sh file to run that contains something like RUN_CMD_FCST=' rm -rf *? Or do we just leave that up to the user to catch?

We should always be concerned with malicious settings for config items, but as it is, whether we set a template and use "eval" to run it, or if we just run it as a command, we will have the same result. We do not currently have check in place for run commands that could contain malicious attacks, so I can't see how we'd need to require that for this particular feature.

christinaholtNOAA · 2022-01-10T23:48:47Z

-  is_element_of "valid_var_values" "${var_value}" || { \
-    valid_var_values_str=$(printf "\"%s\" " "${valid_var_values[@]}");
-    print_err_msg_exit "\
+  if [[ "${var_value}" != *"\$"* ]]; then


Would the \ be required in a template variable? I don't think you need it when using single quotes.

I want to test this but Hera is down today, and I'm reluctant to rebuilt on another platform... I will if I have time, otherwise tomorrow.

Per your suggestion, we're not using the \ before the $ in the templates any more (and requiring that templates always be in single quotes). Then the question is what needs to change here. Turns out all of the following work to detect a $ in a single-quoted template:

if [[ "${var_value}" != *"\$"* ]]; then ... if [[ "${var_value}" != *"$"* ]]; then ... if [[ "${var_value}" != *'$'* ]]; then ...

I think the last one is the clearest, so I'm going with that.

…detect template variables).

gsketefian · 2022-01-12T23:08:24Z

I am ready to merge this one (along with PR #198) if there are no more comments.

christinaholtNOAA · 2022-01-12T23:37:18Z

I can take another look.

christinaholtNOAA · 2022-01-12T23:43:44Z

LGTM!

## DESCRIPTION OF CHANGES: A couple of fixes to get the workflow running on Cheyenne. - Remove `module purge` from load_modules_run_task.sh. This no longer causes failures on Cheyenne due to intervening PR #650, but it should be removed anyway as it can cause future issues - Fixing the number of processors used in the mpirun command for the weather model on Cheyenne. I am honestly not sure how this was ever working, but this change fixes nearly all of the runtime failures currently seen on Cheyenne. ## TESTS CONDUCTED: ### Cheyenne Ran a set of WE2E tests on Cheyenne, chosen mostly at random to save core hours (I did ensure that a variety of domains were run so that several different MPI layouts were tested). Most tasks succeed, and all failures (aside from one walltime issue) are also tests that fail on Hera with the current develop branch. See issue #673 for more details. **Successful tests:** - grid_CONUS_25km_GFDLgrid_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v16 - grid_GSD_HRRR_AK_50km_ics_RAP_lbcs_RAP_suite_GSD_SAR - grid_RRFS_CONUS_13km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta - grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v15p2 - grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v16 - grid_RRFS_CONUS_25km_ics_HRRR_lbcs_HRRR_suite_HRRR - grid_RRFS_CONUS_25km_ics_HRRR_lbcs_HRRR_suite_RRFS_v1beta - grid_RRFS_CONUS_25km_ics_HRRR_lbcs_RAP_suite_HRRR - grid_RRFS_CONUS_25km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta - grid_RRFS_CONUS_3km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta **Unsuccessful tests:** - All gfdlmp tests (grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_2017_gfdlmp, grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_2017_gfdlmp_regional, grid_RRFS_CONUS_25km_ics_GSMGFS_lbcs_GSMGFS_suite_GFS_2017_gfdlmp) - grid_RRFS_CONUS_25km_ics_GSMGFS_lbcs_GSMGFS_suite_GFS_v16 - GST_release_public_v1 - Hit walltime limit ### Hera, Jet, and Orion Ran the same set of tests on Hera, Jet, and Orion, with similar results. On Hera the GST successfully completed (though was close to reaching the walltime limit). On Jet, a few tests (grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_RAP_suite_HRRR, grid_RRFS_CONUS_25km_ics_HRRR_lbcs_HRRR_suite_HRRR, grid_RRFS_CONUS_25km_ics_HRRR_lbcs_HRRR_suite_RRFS_v1beta) failed due to missing initial and/or lateral boundary conditions. On Orion, even more tests failed due to missing ICs and LBCs (grid_GSD_HRRR_AK_50km_ics_RAP_lbcs_RAP_suite_GSD_SAR, grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_RAP_suite_HRRR, grid_RRFS_CONUS_25km_ics_GSMGFS_lbcs_GSMGFS_suite_GFS_2017_gfdlmp, grid_RRFS_CONUS_25km_ics_GSMGFS_lbcs_GSMGFS_suite_GFS_v16). **To summarize, the only test failures were those that were also seen in develop, and mostly due to missing input files on those platforms.** ## DEPENDENCIES: This will need to be merged prior to ufs-community/ufs-srweather-app#206 ## ISSUE: #663 has technically already been resolved, but this will fully address that specific issue.

gsketefian added 28 commits November 19, 2021 04:32

Change "set -x" to "set -x" to reduce clutter in output.

5392d36

Change printing of input arguments to a function or script only if th…

10b6e3a

…e new experiment variable DEBUG is set to TRUE (as opposed to VERBOSE being set to TRUE).

Change printing out of informational message only if the new experime…

666cdea

…nt variable DEBUG is set to TRUE (as opposed to VERBOSE being set to TRUE). Also, for brevity, rename variable file_full_path to file_fp.

Add new experiment variable DEBUG to config_defaults.sh and use it in…

24b49bf

…stead of VERBOSE to reduce clutter in output.

Changes to allow use of "template" variables in config_defaults.sh an…

b275988

…d/or config.sh, i.e. variables whose definitions contain other variables, e.g. MY_CMD="ls \${SOME_DIR}".

Modifications to when messages are printed to screen (VERBOSE, DEBUG).

fc59ea0

Merge branch 'develop' into feature/debug_mode

aa23ce3

Change "set -x" to "set +x" in j-jobs to make output less compact.

45c6ffb

Merge branch 'feature/debug_mode' into feature/template_vars

b003c52

Fix typo.

a966685

Merge branch 'develop' into feature/template_vars

754e2da

Merge branch 'develop' into feature/template_vars

d0eb0db

Merge branch 'develop' into feature/template_vars

7424e13

Conflicts: ush/setup.sh

Set the variables BUILD_ENV_FN and WFLOW_ENV_FN in setup.sh (instead …

52a1c9e

…of launch_FV3LAM_wflow.sh and load_modules_run_task.sh) so that the correct values get recorded to the experiment's var_defns.sh file.

Rewrite the way the variable definitions file (var_defns.sh) is creat…

4beda25

…ed to allow for template variables, i.e. variables that are defined in terms of other variables in config_defaults.sh and config.sh, e.g. MY_VAR="${ANOTHER_VAR}".

Merge remote-tracking branch 'refs/remotes/origin/feature/template_va…

6845ce3

…rs' into feature/template_vars

Minor edits.

880bebe

Minor edits.

2ff052d

Minor changes.

0c2e5c4

Bug fix.

11fbbd4

Fix typo in message.

06a3d2a

Typo fixes to comments and output messages.

dc69b5f

Add forgotten file.

40588ef

Merge branch 'develop' into feature/template_vars

6d1ad63

For clarity, change variable name from VAR_REF_CHAR to VARVALUE_REF_C…

f37de7e

…HAR.

gsketefian mentioned this pull request Dec 13, 2021

Update rst documentation for using template variables #651

Closed

gsketefian changed the title ~~Feature/template vars~~ Add capability to use template variables Dec 13, 2021

Fixes to comments and output messages (avoid hyphenation).

d1bd7b5

christinaholtNOAA suggested changes Dec 28, 2021

View reviewed changes

gsketefian added 3 commits December 29, 2021 01:22

Change local variable name to lowercase and rename for clarity.

7f11b80

Remove variable VARVALUE_REF_CHAR.

f37bfb6

Fix comments; remove duplicate recording of NCORES_PER_NODE.

a41a5a3

JeffBeck-NOAA mentioned this pull request Jan 6, 2022

Refactor supported input path handling. #653

Merged

JeffBeck-NOAA approved these changes Jan 6, 2022

View reviewed changes

christinaholtNOAA suggested changes Jan 7, 2022

View reviewed changes

christinaholtNOAA approved these changes Jan 11, 2022

View reviewed changes

gsketefian added 2 commits January 12, 2022 09:19

Bug fix.

6cd1786

Simplify syntax of if-statement test for detecting a dollar sign (to …

d6847dc

…detect template variables).

Merge branch 'develop' into feature/template_vars

7481e6d

gsketefian merged commit 448a421 into ufs-community:develop Jan 13, 2022

gsketefian mentioned this pull request Jan 14, 2022

Make platform information configurable. #666

Merged

This was referenced Feb 1, 2022

Various fixes for Cheyenne platform #670

Closed

Fix workflow on Cheyenne #672

Merged

jwolff-ncar mentioned this pull request Mar 24, 2022

Update rst documentation for using template variables ufs-community/ufs-srweather-app#230

Closed

mkavulich mentioned this pull request Oct 3, 2022

Update documentation for config files ufs-community/ufs-srweather-app#392

Closed

3 tasks

Conversation

gsketefian commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DESCRIPTION OF CHANGES:

Notes on template variables:

Changes to WE2E tests:

Bug fixes:

TESTS CONDUCTED:

DOCUMENTATION:

Dependencies:

CONTRIBUTORS:

Uh oh!

christinaholtNOAA left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsketefian commented Jan 6, 2022

Uh oh!

JeffBeck-NOAA commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christinaholtNOAA left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gsketefian commented Jan 7, 2022

Uh oh!

christinaholtNOAA commented Jan 7, 2022

Uh oh!

JeffBeck-NOAA commented Jan 7, 2022

Uh oh!

gsketefian commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsketefian commented Jan 10, 2022

Uh oh!

christinaholtNOAA left a comment

Choose a reason for hiding this comment

Uh oh!

gsketefian commented Dec 13, 2021 •

edited

Loading

JeffBeck-NOAA commented Jan 7, 2022 •

edited

Loading

christinaholtNOAA left a comment •

edited

Loading

gsketefian commented Jan 7, 2022 •

edited

Loading