Parameterized task environment moved to suite environments #2308

challurip · 2017-05-26T01:48:58Z

Parameterized task environment "CYLC_TASK_PARAM_task" is now available in suite envionments.

hjoliver · 2017-05-26T02:26:49Z

lib/cylc/config.py

@@ -155,6 +155,7 @@ def __init__(self, suite, fpath, template_vars=None,
        self.actual_first_point = None
        self._start_point_for_actual_first_point = None

+        self.task_param = {}


Maybe a more descriptive name, e.g. task_param_vars.

hjoliver · 2017-05-26T02:31:12Z

lib/cylc/config.py

-                        new_environ[p_var_name] = p_val
+                        new_param = OrderedDictWithDefaults()
+                        new_param[p_var_name] = p_val
+                        self.task_param[name] = new_param


I noticed the branch doesn't work properly with multiple parameters, e.g. foo<m,n>.

new_param doesn't need to be ordered dict, a plain dict will do {}

new_param should be defined outside the loop, we need to store `task_param_vars[task-name] = {p_var_name_1: p_val_1, p_var_name_2:p_val_2, ...}

hjoliver · 2017-05-26T02:31:36Z

lib/cylc/config.py

@@ -2025,6 +2027,9 @@ def _get_taskdef(self, name):
        foo.reverse()
        taskd.namespace_hierarchy = foo

+        if name in self.task_param:
+            taskd.param_var = copy(self.task_param[name])


No need for copy() here.

hjoliver · 2017-05-26T02:33:05Z

lib/cylc/job_file.py

+        if job_conf['param_var']:
+                for var, val in job_conf['param_var'].items():
+                    value = str(val)  # (needed?)
+                    match = re.match(r"^(~[^/\s]*/)(.*)$", value)


Inside a loop - we should use compiled regexes here (use re.compile() at class or module level).

hjoliver · 2017-05-26T02:33:22Z

lib/cylc/job_file.py

+                        handle.write(
+                            '\n    export %s=%s"%s"' %
+                            (var, head, tail))
+                    elif re.match(r"^~[^\s]*$", value):


hjoliver · 2017-05-26T02:33:56Z

lib/cylc/taskdef.py

@@ -77,7 +77,7 @@ def __init__(self, name, rtcfg, run_mode, start_point, spawn_ahead):
        self.namespace_hierarchy = []
        self.dependencies = {}
        self.outputs = []
-
+        self.param_var = []


Is a dict, not a list.

hjoliver · 2017-05-29T21:24:21Z

lib/cylc/job_file.py

@@ -206,6 +209,28 @@ def _write_environment_1(self, handle, job_conf):
            ' '.join(job_conf['namespace_hierarchy']))
        handle.write(
            '\n    export CYLC_TASK_TRY_NUMBER=%s' % job_conf['try_num'])
+        if job_conf['param_var']:
+                for var, val in job_conf['param_var'].items():


Too much indent here.

hjoliver · 2017-05-29T21:25:24Z

lib/cylc/job_file.py

@@ -31,6 +31,9 @@ class JobFileWriter(object):

    """Write task job files."""

+    env_match1 = re.compile(r"^(~[^/\s]*/)(.*)$")
+    env_match2 = re.compile(r"^~[^\s]*$")


We capitalize constants like this, by convention - however, we don't need them at all in this case (see subsequent comments...)

hjoliver · 2017-05-29T21:42:16Z

lib/cylc/job_file.py

+                        # This gets values like "~one ~two" too, but these
+                        # (in variable values) aren't expanded by the shell
+                        # anyway so it doesn't matter.
+                        handle.write('\n    export %s="%s"' % (var, value))


@challurip - sorry, I didn't look closely enough first time through, but: (1) this is now the same code as in _write_environment_1() - where you cut-and-pasted it from - so it should be factored out into a function rather than repeated. However, don't bother doing that because: (2) in fact we don't most of this code at all for parameter variables - Unix file path ~ characters (as per the comments) can appear in general user-defined variables, but not in parameter values which are only simple strings or integers. i.e. we only need the final simple case - the content under else:.

hjoliver · 2017-05-29T21:56:21Z

tests/cylc-get-config/05-param-vars.t

+PARAM2 = \$CYLC_TASK_PARAM_u
+__END__
+
+purge_suite "${SUITE_NAME}"


It occurs to me this test correctly determines that the parameter values are no longer set in the user-defined task environment, but it doesn't test that they are correctly set in the suite-defined task environment. Can you try adding several more tests to this test script:

generate a job script via run_ok using cylc jobscript ${SUITE_NAME} foo_t1_right.1

then use grep_ok to check that $CYLC_TASK_PARAM_t and ..._u are correctly defined in it.

(An alternative would be to run a test suite in which the task foo_t1_right uses its parameter variables at run time - however, checking the job script will do in this case).

hjoliver · 2017-05-31T00:30:01Z

Looks good - @matthewrmshin please review or reassign.

matthewrmshin

Looks OK apart from 2 minor style issues.

matthewrmshin · 2017-05-31T08:52:34Z

lib/cylc/job_file.py

@@ -206,6 +206,10 @@ def _write_environment_1(self, handle, job_conf):
            ' '.join(job_conf['namespace_hierarchy']))
        handle.write(
            '\n    export CYLC_TASK_TRY_NUMBER=%s' % job_conf['try_num'])
+        if job_conf['param_var']:
+            for var, val in job_conf['param_var'].items():
+                value = str(val)  # (needed?)


This should not be needed. The template will do the essential string conversion.

matthewrmshin · 2017-05-31T08:54:30Z

lib/cylc/task_job_mgr.py

@@ -949,4 +948,5 @@ def _prep_submit_task_job_impl(self, suite, itask):
            'task_id': itask.identity,
            'try_num': itask.get_try_num(),
            'work_d': rtconfig['work sub-directory'],
+            'param_var': itask.tdef.param_var,


The keys were sorted alphabetically. Please insert param_var in the right place.

…t not in task environment

hjoliver self-requested a review May 26, 2017 01:57

challurip force-pushed the Parameter-environment branch from b5a9e4f to c9f5308 Compare May 26, 2017 02:29

hjoliver requested changes May 26, 2017

View reviewed changes

matthewrmshin assigned challurip May 26, 2017

matthewrmshin added this to the next release milestone May 26, 2017

challurip force-pushed the Parameter-environment branch from 18a43f7 to f30d341 Compare May 29, 2017 01:16

hjoliver requested changes May 29, 2017

View reviewed changes

hjoliver approved these changes May 31, 2017

View reviewed changes

hjoliver requested a review from matthewrmshin May 31, 2017 00:29

matthewrmshin reviewed May 31, 2017

View reviewed changes

challurip added 6 commits June 1, 2017 03:03

Modified code to view CYLC_TASK_PARAM variable in cylc environment bu…

9587cc0

…t not in task environment

Added tests

b7af2e4

Modified code to support multiple parameters

4b5c7d3

Added test to grep CYLC_TASK_PARAM and removed unnecessary match

012292f

added match for environments

48060f5

minor style changes

bd4934b

challurip force-pushed the Parameter-environment branch from d4f5fe9 to bd4934b Compare June 1, 2017 03:03

matthewrmshin approved these changes Jun 1, 2017

View reviewed changes

matthewrmshin merged commit f76e864 into cylc:master Jun 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parameterized task environment moved to suite environments #2308

Parameterized task environment moved to suite environments #2308

challurip commented May 26, 2017 •

edited by hjoliver

Loading

hjoliver May 26, 2017

hjoliver May 26, 2017

hjoliver May 26, 2017

hjoliver May 26, 2017

hjoliver May 26, 2017

hjoliver May 26, 2017

hjoliver May 29, 2017

hjoliver May 29, 2017

hjoliver May 29, 2017

hjoliver May 29, 2017

hjoliver commented May 31, 2017

matthewrmshin left a comment

matthewrmshin May 31, 2017

matthewrmshin May 31, 2017

Parameterized task environment moved to suite environments #2308

Parameterized task environment moved to suite environments #2308

Conversation

challurip commented May 26, 2017 • edited by hjoliver Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjoliver commented May 31, 2017

matthewrmshin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

challurip commented May 26, 2017 •

edited by hjoliver

Loading