From 15ad245bb3110e6db47ff3fd0c38ddee1e22c0ab Mon Sep 17 00:00:00 2001 From: TomekTrzeciak Date: Fri, 6 Dec 2024 00:09:48 +0000 Subject: [PATCH 1/5] allow workflow config to unset global events config --- cylc/flow/workflow_events.py | 2 +- tests/unit/test_workflow_events.py | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/cylc/flow/workflow_events.py b/cylc/flow/workflow_events.py index 2b99ef3d2cf..704d604d2c4 100644 --- a/cylc/flow/workflow_events.py +++ b/cylc/flow/workflow_events.py @@ -235,7 +235,7 @@ def get_events_conf( glbl_cfg().get(['scheduler', 'mail']) ): value = getter.get(key) - if value is not None and value != []: + if value is not None: return value return default diff --git a/tests/unit/test_workflow_events.py b/tests/unit/test_workflow_events.py index 89449953f20..cea2c3f2b41 100644 --- a/tests/unit/test_workflow_events.py +++ b/tests/unit/test_workflow_events.py @@ -32,8 +32,11 @@ 'key, workflow_cfg, glbl_cfg, expected', [ ('handlers', True, True, ['stall']), - ('handlers', False, True, None), - ('handlers', False, False, None), + ('handlers', False, True, []), + ('handlers', False, False, []), + ('mail events', True, True, []), + ('mail events', False, True, ['abort']), + ('mail events', False, False, []), ('from', True, True, 'docklands@railway'), ('from', False, True, 'highway@mixture'), ('from', False, False, None), @@ -55,13 +58,14 @@ def test_get_events_handler( from = highway@mixture [[events]] abort on workflow timeout = True + mail events = abort ''' ) config = SimpleNamespace() config.cfg = { 'scheduler': { - 'events': {'handlers': ['stall']}, + 'events': {'handlers': ['stall'], 'mail events': []}, 'mail': {'from': 'docklands@railway'}, } if workflow_cfg else {'events': {}} } From 49499981fa613c367b50e850bd9ad32ec1096c37 Mon Sep 17 00:00:00 2001 From: TomekTrzeciak Date: Fri, 6 Dec 2024 17:05:13 +0000 Subject: [PATCH 2/5] catch more string template exceptions --- cylc/flow/workflow_events.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cylc/flow/workflow_events.py b/cylc/flow/workflow_events.py index 704d604d2c4..02d2d4166d3 100644 --- a/cylc/flow/workflow_events.py +++ b/cylc/flow/workflow_events.py @@ -198,7 +198,7 @@ def process_mail_footer( """ try: return (mail_footer_tmpl + '\n') % template_vars - except (KeyError, ValueError): + except (KeyError, TypeError, ValueError): LOG.warning( f'Ignoring bad mail footer template: {mail_footer_tmpl}' ) @@ -328,7 +328,7 @@ def _run_event_custom_handlers(self, schd, template_variables, event): cmd_key = ('%s-%02d' % (self.WORKFLOW_EVENT_HANDLER, i), event) try: cmd = handler % (template_variables) - except KeyError as exc: + except (KeyError, TypeError, ValueError) as exc: message = f'{cmd_key} bad template: {handler}\n{exc}' LOG.error(message) continue From 3b8a7df4a9af0fb45d1bb893839918de8f6f5e9c Mon Sep 17 00:00:00 2001 From: TomekTrzeciak Date: Fri, 6 Dec 2024 17:08:41 +0000 Subject: [PATCH 3/5] UserList to tell apart user set empty list from undefined value --- cylc/flow/parsec/util.py | 34 +++++++++++++----------------- cylc/flow/parsec/validate.py | 7 +++++- cylc/flow/workflow_events.py | 3 ++- tests/unit/test_workflow_events.py | 9 ++++---- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/cylc/flow/parsec/util.py b/cylc/flow/parsec/util.py index 785612639a3..e26134c62c0 100644 --- a/cylc/flow/parsec/util.py +++ b/cylc/flow/parsec/util.py @@ -212,7 +212,7 @@ def replicate(target, source): target[key].defaults_ = pdeepcopy(val.defaults_) replicate(target[key], val) elif isinstance(val, list): - target[key] = val[:] + target[key] = type(val)(val) else: target[key] = val @@ -247,7 +247,7 @@ def poverride(target, sparse, prepend=False): # Override in-place in the target ordered dict. setitem = target.__setitem__ if isinstance(val, list): - setitem(key, val[:]) + setitem(key, type(val)(val)) else: setitem(key, val) @@ -290,25 +290,21 @@ def m_override(target, sparse): stack.append( (val, dest[key], keylist + [key], child_many_defaults)) else: - if key not in dest: - if not ( - '__MANY__' in dest - or key in many_defaults - or '__MANY__' in many_defaults - ): - # TODO - validation prevents this, but handle properly - # for completeness. - raise Exception( - "parsec dict override: no __MANY__ placeholder" + - "%s" % (keylist + [key]) - ) - if isinstance(val, list): - dest[key] = val[:] - else: - dest[key] = val + if not ( + key in dest + or '__MANY__' in dest + or key in many_defaults + or '__MANY__' in many_defaults + ): + # TODO - validation prevents this, but handle properly + # for completeness. + raise Exception( + "parsec dict override: no __MANY__ placeholder" + + "%s" % (keylist + [key]) + ) if isinstance(val, list): - dest[key] = val[:] + dest[key] = type(val)(val) else: dest[key] = val for dest_dict, defaults in defaults_list: diff --git a/cylc/flow/parsec/validate.py b/cylc/flow/parsec/validate.py index 18c19596a63..eb2447d9f2b 100644 --- a/cylc/flow/parsec/validate.py +++ b/cylc/flow/parsec/validate.py @@ -597,7 +597,7 @@ def strip_and_unquote_list(cls, keys, value): # allow trailing commas if values[-1] == '': values = values[0:-1] - return values + return UserList(values) @classmethod def _unquoted_list_parse(cls, keys, value): @@ -647,6 +647,11 @@ def __str__(self): return f'{self[0]} .. {self[1]}' +class UserList(list): + # distinguish user defined, possibly empty list from automatic defaults + pass + + class CylcConfigValidator(ParsecValidator): """Type validator and coercer for Cylc configurations. diff --git a/cylc/flow/workflow_events.py b/cylc/flow/workflow_events.py index 02d2d4166d3..719b7d02f6f 100644 --- a/cylc/flow/workflow_events.py +++ b/cylc/flow/workflow_events.py @@ -24,6 +24,7 @@ from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.hostuserutil import get_host, get_user from cylc.flow.log_diagnosis import run_reftest +from cylc.flow.parsec.validate import UserList from cylc.flow.subprocctx import SubProcContext if TYPE_CHECKING: @@ -235,7 +236,7 @@ def get_events_conf( glbl_cfg().get(['scheduler', 'mail']) ): value = getter.get(key) - if value is not None: + if value not in (None, []) or isinstance(value, UserList): return value return default diff --git a/tests/unit/test_workflow_events.py b/tests/unit/test_workflow_events.py index cea2c3f2b41..47e366b38ba 100644 --- a/tests/unit/test_workflow_events.py +++ b/tests/unit/test_workflow_events.py @@ -21,6 +21,7 @@ from types import SimpleNamespace +from cylc.flow.parsec.validate import UserList from cylc.flow.workflow_events import ( WorkflowEventHandler, get_template_variables, @@ -32,11 +33,11 @@ 'key, workflow_cfg, glbl_cfg, expected', [ ('handlers', True, True, ['stall']), - ('handlers', False, True, []), - ('handlers', False, False, []), + ('handlers', False, True, None), + ('handlers', False, False, None), ('mail events', True, True, []), ('mail events', False, True, ['abort']), - ('mail events', False, False, []), + ('mail events', False, False, None), ('from', True, True, 'docklands@railway'), ('from', False, True, 'highway@mixture'), ('from', False, False, None), @@ -65,7 +66,7 @@ def test_get_events_handler( config = SimpleNamespace() config.cfg = { 'scheduler': { - 'events': {'handlers': ['stall'], 'mail events': []}, + 'events': {'handlers': ['stall'], 'mail events': UserList()}, 'mail': {'from': 'docklands@railway'}, } if workflow_cfg else {'events': {}} } From 0761f1ae3ffeab9ccef233048a89e65b8308664a Mon Sep 17 00:00:00 2001 From: TomekTrzeciak Date: Fri, 6 Dec 2024 22:27:59 +0000 Subject: [PATCH 4/5] list subclass to indicate unassigned list values in expanded config --- cylc/flow/parsec/config.py | 6 +++++- cylc/flow/parsec/util.py | 34 +++++++++++++++++------------- cylc/flow/parsec/validate.py | 7 +----- cylc/flow/workflow_events.py | 4 ++-- tests/unit/test_workflow_events.py | 3 +-- 5 files changed, 28 insertions(+), 26 deletions(-) diff --git a/cylc/flow/parsec/config.py b/cylc/flow/parsec/config.py index 19f937d8e5b..68c74271253 100644 --- a/cylc/flow/parsec/config.py +++ b/cylc/flow/parsec/config.py @@ -35,6 +35,10 @@ from optparse import Values +class DefaultList(list): + """List subclass to indicate unassigned list values in expanded config.""" + + class ParsecConfig: """Object wrapper for parsec functions.""" @@ -112,7 +116,7 @@ def expand(self) -> None: else: if node.default == ConfigNode.UNSET: if node.vdr and node.vdr.endswith('_LIST'): - defs[node.name] = [] + defs[node.name] = DefaultList() else: defs[node.name] = None else: diff --git a/cylc/flow/parsec/util.py b/cylc/flow/parsec/util.py index e26134c62c0..785612639a3 100644 --- a/cylc/flow/parsec/util.py +++ b/cylc/flow/parsec/util.py @@ -212,7 +212,7 @@ def replicate(target, source): target[key].defaults_ = pdeepcopy(val.defaults_) replicate(target[key], val) elif isinstance(val, list): - target[key] = type(val)(val) + target[key] = val[:] else: target[key] = val @@ -247,7 +247,7 @@ def poverride(target, sparse, prepend=False): # Override in-place in the target ordered dict. setitem = target.__setitem__ if isinstance(val, list): - setitem(key, type(val)(val)) + setitem(key, val[:]) else: setitem(key, val) @@ -290,21 +290,25 @@ def m_override(target, sparse): stack.append( (val, dest[key], keylist + [key], child_many_defaults)) else: - if not ( - key in dest - or '__MANY__' in dest - or key in many_defaults - or '__MANY__' in many_defaults - ): - # TODO - validation prevents this, but handle properly - # for completeness. - raise Exception( - "parsec dict override: no __MANY__ placeholder" + - "%s" % (keylist + [key]) - ) + if key not in dest: + if not ( + '__MANY__' in dest + or key in many_defaults + or '__MANY__' in many_defaults + ): + # TODO - validation prevents this, but handle properly + # for completeness. + raise Exception( + "parsec dict override: no __MANY__ placeholder" + + "%s" % (keylist + [key]) + ) + if isinstance(val, list): + dest[key] = val[:] + else: + dest[key] = val if isinstance(val, list): - dest[key] = type(val)(val) + dest[key] = val[:] else: dest[key] = val for dest_dict, defaults in defaults_list: diff --git a/cylc/flow/parsec/validate.py b/cylc/flow/parsec/validate.py index eb2447d9f2b..18c19596a63 100644 --- a/cylc/flow/parsec/validate.py +++ b/cylc/flow/parsec/validate.py @@ -597,7 +597,7 @@ def strip_and_unquote_list(cls, keys, value): # allow trailing commas if values[-1] == '': values = values[0:-1] - return UserList(values) + return values @classmethod def _unquoted_list_parse(cls, keys, value): @@ -647,11 +647,6 @@ def __str__(self): return f'{self[0]} .. {self[1]}' -class UserList(list): - # distinguish user defined, possibly empty list from automatic defaults - pass - - class CylcConfigValidator(ParsecValidator): """Type validator and coercer for Cylc configurations. diff --git a/cylc/flow/workflow_events.py b/cylc/flow/workflow_events.py index 719b7d02f6f..4e9f3155a2f 100644 --- a/cylc/flow/workflow_events.py +++ b/cylc/flow/workflow_events.py @@ -24,7 +24,7 @@ from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.hostuserutil import get_host, get_user from cylc.flow.log_diagnosis import run_reftest -from cylc.flow.parsec.validate import UserList +from cylc.flow.parsec.config import DefaultList from cylc.flow.subprocctx import SubProcContext if TYPE_CHECKING: @@ -236,7 +236,7 @@ def get_events_conf( glbl_cfg().get(['scheduler', 'mail']) ): value = getter.get(key) - if value not in (None, []) or isinstance(value, UserList): + if value is not None and not isinstance(value, DefaultList): return value return default diff --git a/tests/unit/test_workflow_events.py b/tests/unit/test_workflow_events.py index 47e366b38ba..3aabc6de067 100644 --- a/tests/unit/test_workflow_events.py +++ b/tests/unit/test_workflow_events.py @@ -21,7 +21,6 @@ from types import SimpleNamespace -from cylc.flow.parsec.validate import UserList from cylc.flow.workflow_events import ( WorkflowEventHandler, get_template_variables, @@ -66,7 +65,7 @@ def test_get_events_handler( config = SimpleNamespace() config.cfg = { 'scheduler': { - 'events': {'handlers': ['stall'], 'mail events': UserList()}, + 'events': {'handlers': ['stall'], 'mail events': []}, 'mail': {'from': 'docklands@railway'}, } if workflow_cfg else {'events': {}} } From 257eaa15c24416fca1f617fc168dd1d4aaeca233 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Fri, 14 Feb 2025 13:01:37 +0000 Subject: [PATCH 5/5] Changelog --- changes.d/6518.fix.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes.d/6518.fix.md diff --git a/changes.d/6518.fix.md b/changes.d/6518.fix.md new file mode 100644 index 00000000000..00f89c08774 --- /dev/null +++ b/changes.d/6518.fix.md @@ -0,0 +1 @@ +Allow setting empty values in `flow.cylc[scheduler][events]` to override the global configuration.