From 3f3396ed509a2c39ccf535140307d3a8bcc93cf1 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Thu, 3 Jun 2021 20:52:14 -0600 Subject: [PATCH 1/6] Give an error message when no default namelist value can be found The check here was not being triggered when it should, because None values were being converted to empty strings. This meant that, if no default value could be found, instead of aborting, the code was writing out 'variable = ' to the namelist file (rather than 'variable = value' for some value). The changes in this commit distinguish between two possible causes of None values coming from entry_id's get_value_match: 1. No match found: in this case, we want to keep the value as None so that it triggers a later error check (in contrast to what was being done before). 2. A match is found, but self.text(mnode) returns None: I'm not sure when (if ever) this is triggered, but I did not want to change the logic for this situation, so I have introduced an optional argument, replacement_for_none, which maintains the old logic (replacing None with an empty string) in this case. The changes in namelist_definition:get_default_value are somewhat unrelated: The original motivation was seeing that self.get_value_match can now return None; but as I looked closely, I saw that there was a double-call to self._split_defaults_text(value) that looks like a bug, so I have removed that double-call. I don't see any calls to this get_default_value routine from within cime, but it's possible that components use it in their buildnml. Resolves ESMCI/cime#3984 - see that issue for more details. --- scripts/lib/CIME/XML/entry_id.py | 47 +++++++++++++++------ scripts/lib/CIME/XML/namelist_definition.py | 9 ++-- 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/scripts/lib/CIME/XML/entry_id.py b/scripts/lib/CIME/XML/entry_id.py index d02e88680bb..2e16e339f38 100644 --- a/scripts/lib/CIME/XML/entry_id.py +++ b/scripts/lib/CIME/XML/entry_id.py @@ -43,29 +43,45 @@ def set_default_value(self, vid, val): return val - def get_value_match(self, vid, attributes=None, exact_match=False, entry_node=None): - # Handle this case: - # - # - # X - # Y - # Z - # - # + def get_value_match(self, vid, attributes=None, exact_match=False, entry_node=None, + replacement_for_none=None): + """Handle this case: + + + X + Y + Z + + + + If replacement_for_none is provided, then: if the found text value would give a + None value, instead replace it with the value given by the replacement_for_none + argument. (However, still return None if no match is found.) This may or may not + be needed, but is in place to maintain some old logic. + + """ if entry_node is not None: - value = self._get_value_match(entry_node, attributes, exact_match) + value = self._get_value_match(entry_node, attributes, exact_match, + replacement_for_none=replacement_for_none) else: node = self.get_optional_child("entry", {"id":vid}) value = None if node is not None: - value = self._get_value_match(node, attributes, exact_match) + value = self._get_value_match(node, attributes, exact_match, + replacement_for_none=replacement_for_none) logger.debug("(get_value_match) vid {} value {}".format(vid, value)) return value - def _get_value_match(self, node, attributes=None, exact_match=False): + def _get_value_match(self, node, attributes=None, exact_match=False, + replacement_for_none=None): ''' Note that the component class has a specific version of this function + + If replacement_for_none is provided, then: if the found text value would give a + None value, instead replace it with the value given by the replacement_for_none + argument. (However, still return None if no match is found.) This may or may not + be needed, but is in place to maintain some old logic. ''' # if there is a element - check to see if there is a match attribute # if there is NOT a match attribute, then set the default to "first" @@ -125,7 +141,12 @@ def _get_value_match(self, node, attributes=None, exact_match=False): expect(False, "match attribute can only have a value of 'last' or 'first', value is %s" %match_type) - return self.text(mnode) + text = self.text(mnode) + if text is None: + # NOTE(wjs, 2021-06-03) I'm not sure when (if ever) this can happen, but I'm + # putting this logic here to maintain some old logic, to be safe. + text = replacement_for_none + return text def get_node_element_info(self, vid, element_name): node = self.get_optional_child("entry", {"id":vid}) diff --git a/scripts/lib/CIME/XML/namelist_definition.py b/scripts/lib/CIME/XML/namelist_definition.py index 3a50d35dc25..865082bb2cc 100644 --- a/scripts/lib/CIME/XML/namelist_definition.py +++ b/scripts/lib/CIME/XML/namelist_definition.py @@ -191,10 +191,9 @@ def get_value_match(self, vid, attributes=None, exact_match=True, entry_node=Non if entry_node is None: entry_node = self._nodes[vid] value = super(NamelistDefinition, self).get_value_match(vid.lower(),attributes=all_attributes, exact_match=exact_match, - entry_node=entry_node) - if value is None: - value = '' - else: + entry_node=entry_node, + replacement_for_none='') + if value is not None: value = self._split_defaults_text(value) return value @@ -445,4 +444,4 @@ def get_default_value(self, item, attribute=None): all_attributes.update(attribute) value = self.get_value_match(item.lower(), all_attributes, True) - return self._split_defaults_text(value) + return value From 4ccd2a343dd9b278519952e32ed96da79c12a2ce Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Thu, 3 Jun 2021 21:14:56 -0600 Subject: [PATCH 2/6] Define pio_netcdf_format for esp component With the changes in the last commit to fix ESMCI/cime#3984, this missing default now led to an error. Previously, ESP's pio_netcdf_format was being set to an empty string. I'm not positive that the way I've set it is correct, but it seems more correct than before. --- src/drivers/mct/cime_config/namelist_definition_modelio.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/drivers/mct/cime_config/namelist_definition_modelio.xml b/src/drivers/mct/cime_config/namelist_definition_modelio.xml index 660bc93dee3..ce0275e59bf 100644 --- a/src/drivers/mct/cime_config/namelist_definition_modelio.xml +++ b/src/drivers/mct/cime_config/namelist_definition_modelio.xml @@ -172,6 +172,7 @@ $GLC_PIO_NETCDF_FORMAT $WAV_PIO_NETCDF_FORMAT $IAC_PIO_NETCDF_FORMAT + $ESP_PIO_NETCDF_FORMAT From cb55d7b5b5e6b124b632005ef53a78321870c4b3 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Fri, 4 Jun 2021 11:56:00 -0600 Subject: [PATCH 3/6] Fixes to allow a variable to be specified in user_nl but not in defaults (1) If a variable is specified in the user_nl file (the "infile" in nmlgen), then set have_value = True to avoid triggering an error: as long as a value is specified in the user_nl file, then it's okay if no default value exists. See https://github.com/ESMCI/cime/issues/3984#issuecomment-854409941 for detailed thoughts. (2) Change Namelist.get_variable_value to return a copy rather than a reference to the value list. See https://github.com/ESMCI/cime/issues/3984#issuecomment-854904669 for detailed thoughts. --- scripts/lib/CIME/namelist.py | 4 +++- scripts/lib/CIME/nmlgen.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/lib/CIME/namelist.py b/scripts/lib/CIME/namelist.py index cc76117da85..1b0716cfb25 100644 --- a/scripts/lib/CIME/namelist.py +++ b/scripts/lib/CIME/namelist.py @@ -953,7 +953,9 @@ def get_variable_value(self, group_name, variable_name): if gn: vn = string_in_list(variable_name,self._groups[gn]) if vn: - return self._groups[gn][vn] + # Make a copy of the list so that any modifications done by the caller + # don't modify the internal values. + return self._groups[gn][vn].copy() return [''] def get_value(self, variable_name): diff --git a/scripts/lib/CIME/nmlgen.py b/scripts/lib/CIME/nmlgen.py index 153a58c260c..0219fe0b658 100644 --- a/scripts/lib/CIME/nmlgen.py +++ b/scripts/lib/CIME/nmlgen.py @@ -629,6 +629,8 @@ def add_default(self, name, value=None, ignore_abs_path=None): have_value = False # Check for existing value. current_literals = self._namelist.get_variable_value(group, name) + if current_literals != [""]: + have_value = True # Check for input argument. if value is not None: From e0e553509083f22edfc7a402e32b6a40ddb35806 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Fri, 4 Jun 2021 12:33:30 -0600 Subject: [PATCH 4/6] Add comments and fix a pylint error --- scripts/lib/CIME/XML/namelist_definition.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/lib/CIME/XML/namelist_definition.py b/scripts/lib/CIME/XML/namelist_definition.py index 865082bb2cc..bc849c58689 100644 --- a/scripts/lib/CIME/XML/namelist_definition.py +++ b/scripts/lib/CIME/XML/namelist_definition.py @@ -174,6 +174,9 @@ def set_value(self, vid, value, subgroup=None, ignore_type=True): """This function is not implemented.""" raise TypeError("NamelistDefinition does not support `set_value`.") + # In contrast to the entry_id version of this method, this version doesn't support the + # replacement_for_none argument, because it is hard-coded to ''. + # pylint: disable=arguments-differ def get_value_match(self, vid, attributes=None, exact_match=True, entry_node=None): """Return the default value for the variable named `vid`. @@ -190,6 +193,8 @@ def get_value_match(self, vid, attributes=None, exact_match=True, entry_node=Non if entry_node is None: entry_node = self._nodes[vid] + # NOTE(wjs, 2021-06-04) In the following call, replacement_for_none='' may not + # actually be needed, but I'm setting it to maintain some old logic, to be safe. value = super(NamelistDefinition, self).get_value_match(vid.lower(),attributes=all_attributes, exact_match=exact_match, entry_node=entry_node, replacement_for_none='') From 3c1840cc96c2356a0aba34ba559b1e475d7131c0 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Fri, 4 Jun 2021 15:57:19 -0600 Subject: [PATCH 5/6] If no default value is found, print attributes used in search --- scripts/lib/CIME/XML/namelist_definition.py | 4 ++++ scripts/lib/CIME/nmlgen.py | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/lib/CIME/XML/namelist_definition.py b/scripts/lib/CIME/XML/namelist_definition.py index bc849c58689..39c2427eb7e 100644 --- a/scripts/lib/CIME/XML/namelist_definition.py +++ b/scripts/lib/CIME/XML/namelist_definition.py @@ -156,6 +156,10 @@ def get_group(self, name): def add_attributes(self, attributes): self._attributes = attributes + def get_attributes(self): + """Return this object's attributes dictionary""" + return self._attributes + def get_entry_nodes(self): return self._entry_nodes diff --git a/scripts/lib/CIME/nmlgen.py b/scripts/lib/CIME/nmlgen.py index 0219fe0b658..c823f92ff57 100644 --- a/scripts/lib/CIME/nmlgen.py +++ b/scripts/lib/CIME/nmlgen.py @@ -645,7 +645,8 @@ def add_default(self, name, value=None, ignore_abs_path=None): have_value = True default_literals = self._to_namelist_literals(name, default) current_literals = merge_literal_lists(default_literals, current_literals) - expect(have_value, "No default value found for {}.".format(name)) + expect(have_value, "No default value found for {} with attributes {}.".format( + name, self._definition.get_attributes())) # Go through file names and prepend input data root directory for # absolute pathnames. From 747fcd89de20ae934bddcababab30cee8e30d0e9 Mon Sep 17 00:00:00 2001 From: Bill Sacks Date: Fri, 4 Jun 2021 16:19:35 -0600 Subject: [PATCH 6/6] Use a list copy method that will work in python2 --- scripts/lib/CIME/namelist.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/lib/CIME/namelist.py b/scripts/lib/CIME/namelist.py index 1b0716cfb25..d6c7647ed66 100644 --- a/scripts/lib/CIME/namelist.py +++ b/scripts/lib/CIME/namelist.py @@ -955,7 +955,7 @@ def get_variable_value(self, group_name, variable_name): if vn: # Make a copy of the list so that any modifications done by the caller # don't modify the internal values. - return self._groups[gn][vn].copy() + return self._groups[gn][vn][:] return [''] def get_value(self, variable_name):