From d4edafaf03663d81feec862653e3acc5235b5297 Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Wed, 18 Dec 2024 02:19:24 +1300 Subject: [PATCH] Fix output consistency check when there are offsets. (#6526) * Fix output consistency check when there are offsets. * Update change log. * Add graph parser unit tests. Co-authored-by: Tim Pillinger * Deflake a test. * Sort pairs to make errors deterministic. * dict(was hiding stuff) * tweak variable name (review suggestion) --------- Co-authored-by: Tim Pillinger Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com> --- changes.d/6526.fix.md | 1 + cylc/flow/graph_parser.py | 35 +++++++++---------- tests/flakyfunctional/cylc-show/04-multi.t | 15 ++++++-- tests/functional/graphing/06-family-or.t | 6 ++-- .../integration/reftests/test_pre_initial.py | 4 +-- tests/unit/test_graph_parser.py | 29 +++++++++++++-- 6 files changed, 62 insertions(+), 28 deletions(-) create mode 100644 changes.d/6526.fix.md diff --git a/changes.d/6526.fix.md b/changes.d/6526.fix.md new file mode 100644 index 00000000000..d6874a7e679 --- /dev/null +++ b/changes.d/6526.fix.md @@ -0,0 +1 @@ +Output optionality validation now checks tasks with cycle offsets. diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index c751e6196b8..2804f9a67cc 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -251,6 +251,7 @@ class GraphParser: rf""" (!)? # suicide mark ({_RE_NODE}) # node name + ({_RE_OFFSET})? # cycle point offset ({_RE_QUAL})? # trigger qualifier ({_RE_OPT})? # optional output indicator """, @@ -469,10 +470,9 @@ def parse_graph(self, graph_string: str) -> None: pairs.add((chain[i], chain[i + 1])) # Get a set of RH nodes which are not at the LH of another pair: - pairs_dict = dict(pairs) - terminals = set(pairs_dict.values()).difference(pairs_dict.keys()) + terminals = {p[1] for p in pairs}.difference({p[0] for p in pairs}) - for pair in pairs: + for pair in sorted(pairs, key=lambda p: str(p[0])): self._proc_dep_pair(pair, terminals) @classmethod @@ -540,16 +540,12 @@ def _proc_dep_pair( raise GraphParseError(mismatch_msg.format(right)) # Raise error for cycle point offsets at the end of chains - if '[' in right: - if left and (right in terminals): - # This right hand side is at the end of a chain: - raise GraphParseError( - 'Invalid cycle point offsets only on right hand ' - 'side of a dependency (must be on left hand side):' - f' {left} => {right}') - else: - # This RHS is also a LHS in a chain: - return + if '[' in right and left and (right in terminals): + # This right hand side is at the end of a chain: + raise GraphParseError( + 'Invalid cycle point offsets only on right hand ' + 'side of a dependency (must be on left hand side):' + f' {left} => {right}') # Split right side on AND. rights = right.split(self.__class__.OP_AND) @@ -887,7 +883,7 @@ def _compute_triggers( raise ValueError( # pragma: no cover f"Unexpected graph expression: '{right}'" ) - suicide_char, name, output, opt_char = m.groups() + suicide_char, name, offset, output, opt_char = m.groups() suicide = (suicide_char == self.__class__.SUICIDE) optional = (opt_char == self.__class__.OPTIONAL) if output: @@ -895,7 +891,7 @@ def _compute_triggers( if name in self.family_map: fam = True - mems = self.family_map[name] + rhs_members = self.family_map[name] if not output: # (Plain family name on RHS). # Make implicit success explicit. @@ -922,10 +918,13 @@ def _compute_triggers( else: # Convert to standard output names if necessary. output = TaskTrigger.standardise_name(output) - mems = [name] + rhs_members = [name] outputs = [output] - for mem in mems: - self._set_triggers(mem, suicide, trigs, expr, orig_expr) + for mem in rhs_members: + if not offset: + # Nodes with offsets on the RHS do not define triggers. + self._set_triggers(mem, suicide, trigs, expr, orig_expr) for output in outputs: + # But they must be consistent with output optionality. self._set_output_opt(mem, output, optional, suicide, fam) diff --git a/tests/flakyfunctional/cylc-show/04-multi.t b/tests/flakyfunctional/cylc-show/04-multi.t index c01091891d4..5f2b2828418 100644 --- a/tests/flakyfunctional/cylc-show/04-multi.t +++ b/tests/flakyfunctional/cylc-show/04-multi.t @@ -44,7 +44,10 @@ outputs: ('⨯': not completed) ⨯ 2016/t1 succeeded ⨯ 2016/t1 failed output completion: incomplete - ⨯ ┆ succeeded + ┆ ( + ✓ ┆ started + ⨯ ┆ and succeeded + ┆ ) Task ID: 2017/t1 title: (not given) @@ -61,7 +64,10 @@ outputs: ('⨯': not completed) ⨯ 2017/t1 succeeded ⨯ 2017/t1 failed output completion: incomplete - ⨯ ┆ succeeded + ┆ ( + ✓ ┆ started + ⨯ ┆ and succeeded + ┆ ) Task ID: 2018/t1 title: (not given) @@ -78,7 +84,10 @@ outputs: ('⨯': not completed) ⨯ 2018/t1 succeeded ⨯ 2018/t1 failed output completion: incomplete - ⨯ ┆ succeeded + ┆ ( + ✓ ┆ started + ⨯ ┆ and succeeded + ┆ ) __TXT__ contains_ok "${RUND}/show2.txt" <<'__TXT__' diff --git a/tests/functional/graphing/06-family-or.t b/tests/functional/graphing/06-family-or.t index 1a3a76ed8d0..6e1c63f05ec 100755 --- a/tests/functional/graphing/06-family-or.t +++ b/tests/functional/graphing/06-family-or.t @@ -26,9 +26,9 @@ cat >'flow.cylc' <<'__FLOW_CONFIG__' initial cycle point = 2000 [[graph]] T00 = """ - A - B - A[-PT24H]:fail-any | B[-PT24H]:fail-any => c + A? + B? + A[-PT24H]:fail-any? | B[-PT24H]:fail-any? => c """ [runtime] [[A]] diff --git a/tests/integration/reftests/test_pre_initial.py b/tests/integration/reftests/test_pre_initial.py index d1ed6a6e566..3a6d8f35d4a 100644 --- a/tests/integration/reftests/test_pre_initial.py +++ b/tests/integration/reftests/test_pre_initial.py @@ -102,8 +102,8 @@ async def test_over_bracketed(flow, scheduler, reftest): 'final cycle point': '2013-12-25T12:00Z', 'graph': { 'T12': ''' - (a[-P1D]:fail | b[-P1D]:fail | c[-P1D]:fail) => d - a & b & c # Implied by implicit cycling now... + (a[-P1D]:fail? | b[-P1D]:fail? | c[-P1D]:fail?) => d + a? & b? & c? # Implied by implicit cycling now... ''', }, }, diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index 75a0bf95a83..438d7e5f836 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -139,6 +139,31 @@ def test_graph_syntax_errors_2(seq, graph, expected_err): 'Invalid cycle point offsets only on right', id='no-cycle-point-RHS' ), + # See https://github.com/cylc/cylc-flow/issues/6523 + # For the next 4 tests: + param( + # Yes I know it's circular, but it's here to + # demonstrate that the test below is broken: + "foo:finished => foo", + 'Output foo:succeeded can\'t be both required and optional', + id='finish-implies-success-optional' + ), + param( + "foo[-P1]:finish => foo", + 'Output foo:succeeded can\'t be both required and optional', + id='finish-implies-success-optional-offset' + ), + param( + "foo[-P1]:succeeded | foo[-P1]:failed => bar", + # order of outputs varies in the error message + 'must both be optional if both are used', + id='succeed-or-failed-mustbe-optional' + ), + param( + "foo[-P1]:succeeded? | foo[-P1]:failed? => foo", + 'Output foo:succeeded can\'t be both required and optional', + id='succeed-or-failed-implies-success-optional' + ), ] ) def test_graph_syntax_errors(graph, expected_err): @@ -948,8 +973,8 @@ def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]): param((('a', 'b[-P42M]'), {'b[-P42M]'}), 'Invalid cycle point offset'), # No error if offset in NON-terminal RHS: param((('a', 'b[-P42M]'), {}), None), - # Don't check the left hand side if this has a non-terminal RHS: - param((('a &', 'b[-P42M]'), {}), None), + # Check the left hand side if this has a non-terminal RHS: + param((('a &', 'b[-P42M]'), {}), 'Null task name in graph'), ) ) def test_proc_dep_pair(args, err):