Fix param expansion in inherited environment. (#4248)

* Fix param expansion in inherited environment.

* Update change log.

* Extend functional test.

* Code tidy.

* Make prev fix work regardless of namespace order.

* Extend param test again.

CHANGES.md

@@ -82,6 +82,9 @@ workflow's public database file was not closed properly.
 
 ### Fixes
 
+[#4248](https://github.com/cylc/cylc-flow/pull/4248)
+ - Fix parameter expansion in inherited task environments.
+
 [#4227](https://github.com/cylc/cylc-flow/pull/4227) - Better error messages
 when initial cycle point is not valid for the cycling type.
 

cylc/flow/config.py

@@ -946,6 +946,17 @@ def _expand_name_list(self, orig_names):
             exp_names += [name for name, _ in name_expander.expand(orig_name)]
         return exp_names
 
+    def _update_task_params(self, task_name, params):
+        """Update the dict of parameters used in a task definition.
+
+        # Used to expand parameter values in task environments.
+        """
+        self.task_param_vars.setdefault(
+            task_name, {}
+        ).update(
+            params
+        )
+
     def _expand_runtime(self):
         """Expand [runtime] name lists or parameterized names.
 
@@ -973,8 +984,7 @@ def _expand_runtime(self):
                     newruntime[name] = OrderedDictWithDefaults()
                 replicate(newruntime[name], namespace_dict)
                 if indices:
-                    # Put parameter values in task environments.
-                    self.task_param_vars[name] = indices
+                    self._update_task_params(name, indices)
                     new_environ = OrderedDictWithDefaults()
                     if 'environment' in newruntime[name]:
                         new_environ = newruntime[name]['environment'].copy()
@@ -985,12 +995,15 @@ def _expand_runtime(self):
                     origin = 'inherit = %s' % ', '.join(parents)
                     repl_parents = []
                     for parent in parents:
-                        repl_parents.append(
+                        used_indices, expanded = (
                             name_expander.expand_parent_params(
-                                parent, indices, origin))
+                                parent, indices, origin)
+                        )
+                        repl_parents.append(expanded)
+                        if used_indices:
+                            self._update_task_params(name, used_indices)
                     newruntime[name]['inherit'] = repl_parents
         self.cfg['runtime'] = newruntime
-
         # Parameter expansion of visualization node attributes. TODO - do vis
         # 'node groups' too, or deprecate them (use families in 'node attrs').
         name_expander = NameExpander(self.parameters)
@@ -1058,7 +1071,7 @@ def check_param_env_tmpls(self):
         if bads:
             LOG.warning(
                 'bad parameter environment template:\n    '
-                '\n    '.join(
+                + '\n    '.join(
                     '[runtime][%s][environment]%s = %s  # %s' %
                     bad for bad in sorted(bads)
                 )

cylc/flow/param_expand.py

@@ -215,7 +215,7 @@ def expand_parent_params(self, parent, param_values, origin):
         """
         head, p_list_str, tail = REC_P_ALL.match(parent).groups()
         if not p_list_str:
-            return head
+            return (None, head)
         used = {}
         for item in (i.strip() for i in p_list_str.split(',')):
             if '-' in item or '+' in item:
@@ -253,7 +253,7 @@ def expand_parent_params(self, parent, param_values, origin):
             tmpl += self.param_tmpl_cfg[pname]
         if tail:
             tmpl += tail
-        return tmpl % used
+        return (used, tmpl % used)
 
 
 class GraphExpander:

tests/functional/param_expand/03-env-tmpl.t

@@ -16,6 +16,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #-------------------------------------------------------------------------------
 # Test parameter environment templates
+# Extended to include inheritance from parameterized parents (GitHub #4248)
 . "$(dirname "$0")/test_header"
 set_test_number 2
 reftest

tests/functional/param_expand/03-env-tmpl/flow.cylc

@@ -1,22 +1,40 @@
 [task parameters]
     num = 99..101..2
     stuff = this, that
+    state = open, closed
 [scheduling]
     [[graph]]
-        R1 = "t1<num,stuff> => t2<num,stuff>"
+        R1 = """
+           t1<num,stuff> => t2<num,stuff>
+           x<stuff>
+        """
 [runtime]
     [[T]]
         [[[environment]]]
             MYNUM = %(num)d
             MYSTUFF = stuff %(stuff)s
             MY_FILE = %(num)04d-%(stuff)s
+    [[U<state>]]
+        [[[environment]]]
+            STATUS = %(state)s
     [[t1<num,stuff>]]
-        inherit = T
+        inherit = T, U<state=open>
         script = """
-echo "${MYNUM} and ${MYSTUFF}" >"${CYLC_WORKFLOW_RUN_DIR}/${MY_FILE}"
-"""
+            FILE="${CYLC_WORKFLOW_RUN_DIR}/t1-${MY_FILE}"
+            echo "${MYNUM} ${MYSTUFF} ${STATUS}" >"${FILE}"
+            diff ${FILE} ${FILE}.ref
+        """
     [[t2<num,stuff>]]
-        inherit = T
+        inherit = T, U<state=closed>
         script = """
-test "${MYNUM} and ${MYSTUFF}" = "$(<"${CYLC_WORKFLOW_RUN_DIR}/${MY_FILE}")"
-"""
+            FILE="${CYLC_WORKFLOW_RUN_DIR}/t2-${MY_FILE}"
+            echo "${MYNUM} ${MYSTUFF} ${STATUS}" >"${FILE}"
+            diff ${FILE} ${FILE}.ref
+        """
+    # The following tests the example of GH #4248. We had wrongly assumed that
+    # general comes before specific, for parameters in inherited environments.
+    [[x<stuff=that>]]
+        inherit = U<state=closed>
+        script = test $STATUS == closed
+    [[x<stuff>]]
+        pre-script = "echo pre"

tests/functional/param_expand/03-env-tmpl/reference.log

@@ -1,10 +1,13 @@
-2017-01-12T14:46:57Z INFO - Initial point: 1
-2017-01-12T14:46:57Z INFO - Final point: 1
-2017-01-12T14:46:57Z INFO - [t1_num099_this.1] -triggered off []
-2017-01-12T14:47:00Z INFO - [t2_num099_this.1] -triggered off ['t1_num099_this.1']
-2017-01-12T14:46:57Z INFO - [t1_num099_that.1] -triggered off []
-2017-01-12T14:47:00Z INFO - [t2_num099_that.1] -triggered off ['t1_num099_that.1']
-2017-01-12T14:46:57Z INFO - [t1_num101_this.1] -triggered off []
-2017-01-12T14:47:00Z INFO - [t2_num101_this.1] -triggered off ['t1_num101_this.1']
-2017-01-12T14:46:57Z INFO - [t1_num101_that.1] -triggered off []
-2017-01-12T14:47:00Z INFO - [t2_num101_that.1] -triggered off ['t1_num101_that.1']
+Initial point: 1
+Final point: 1
+[t1_num099_this.1] -triggered off []
+[t2_num099_this.1] -triggered off ['t1_num099_this.1']
+[t1_num099_that.1] -triggered off []
+[t2_num099_that.1] -triggered off ['t1_num099_that.1']
+[t1_num101_this.1] -triggered off []
+[t2_num101_this.1] -triggered off ['t1_num101_this.1']
+[t1_num101_that.1] -triggered off []
+[t2_num101_that.1] -triggered off ['t1_num101_that.1']
+[x_that.1] -triggered off []
+[x_this.1] -triggered off []
+

tests/functional/param_expand/03-env-tmpl/t1-0099-that.ref

@@ -0,0 +1 @@
+99 stuff that open

tests/functional/param_expand/03-env-tmpl/t1-0099-this.ref

@@ -0,0 +1 @@
+99 stuff this open

tests/functional/param_expand/03-env-tmpl/t1-0101-that.ref

@@ -0,0 +1 @@
+101 stuff that open

tests/functional/param_expand/03-env-tmpl/t1-0101-this.ref

@@ -0,0 +1 @@
+101 stuff this open

tests/functional/param_expand/03-env-tmpl/t2-0099-that.ref

@@ -0,0 +1 @@
+99 stuff that closed

tests/functional/param_expand/03-env-tmpl/t2-0099-this.ref

@@ -0,0 +1 @@
+99 stuff this closed

tests/functional/param_expand/03-env-tmpl/t2-0101-that.ref

@@ -0,0 +1 @@
+101 stuff that closed

tests/functional/param_expand/03-env-tmpl/t2-0101-this.ref

@@ -0,0 +1 @@
+101 stuff this closed