From 674d3804a2b043f1c3b6fcb17a649547dbd71f49 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 16 Jan 2024 10:57:59 +0000 Subject: [PATCH 1/8] Error if graph has a dependency offset exclusively on the RHS of a graph pair. --- changes.d/fix.5924.md | 1 + cylc/flow/graph_parser.py | 38 ++++++++++++++++++++++----------- tests/unit/test_graph_parser.py | 24 +++++++++++++++++++-- 3 files changed, 49 insertions(+), 14 deletions(-) create mode 100644 changes.d/fix.5924.md diff --git a/changes.d/fix.5924.md b/changes.d/fix.5924.md new file mode 100644 index 00000000000..be3ba7ad429 --- /dev/null +++ b/changes.d/fix.5924.md @@ -0,0 +1 @@ +Ensure that graphs with dependency offsets on the right hand side are flagged to users. \ No newline at end of file diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index ad0ec280a3d..f63d7193417 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -462,8 +462,13 @@ def parse_graph(self, graph_string: str) -> None: for i in range(0, len(chain) - 1): pairs.add((chain[i], chain[i + 1])) + # Get a set of RH nodes which are not at the LH of another pair: + terminals = { + right for right in {right for _, right in pairs} + if right not in {left for left, _ in pairs}} + for pair in pairs: - self._proc_dep_pair(pair) + self._proc_dep_pair(pair, terminals) @classmethod def _report_invalid_lines(cls, lines: List[str]) -> None: @@ -493,19 +498,23 @@ def _report_invalid_lines(cls, lines: List[str]) -> None: def _proc_dep_pair( self, - pair: Tuple[Optional[str], str] + pair: Tuple[Optional[str], str], + terminals: Set[str], ) -> None: """Process a single dependency pair 'left => right'. - 'left' can be a logical expression of qualified node names. - 'left' can be None, when triggering a left-side or lone node. - 'left' can be "", if null task name in graph error (a => => b). - 'right' can be one or more node names joined by AND. - 'right' can't be None or "". A node is an xtrigger, or a task or a family name. A qualified name is NAME([CYCLE-POINT-OFFSET])(:QUALIFIER). - Trigger qualifiers, but not cycle offsets, are ignored on the right to - allow chaining. + + Args: + pair: + 'left' can be a logical expression of qualified node names. + 'left' can be None, when triggering a left-side or lone node. + 'left' can be "", if null task name in graph error (a => => b). + 'right' can be one or more node names joined by AND. + 'right' can't be None or "". + terminals: + Lits of nodes which are _only_ on the RH end of chains. """ left, right = pair # Raise error for right-hand-side OR operators. @@ -525,10 +534,15 @@ def _proc_dep_pair( if right.count("(") != right.count(")"): raise GraphParseError(mismatch_msg.format(right)) - # Ignore cycle point offsets on the right side. - # (Note we can't ban this; all nodes get process as left and right.) + # Ignore/Error cycle point offsets on the right side: + # (Note we can only ban this for nodes at the end of chains) if '[' in right: - return + if right in terminals: + raise GraphParseError( + 'ERROR, illegal cycle point offset on the right:' + f' {left} => {right}') + else: + return # Split right side on AND. rights = right.split(self.__class__.OP_AND) diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index ddd443a3597..f739a79d09b 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -133,13 +133,33 @@ def test_graph_syntax_errors_2(seq, graph, expected_err): "foo || bar => baz", "The graph OR operator is '|'" ), + param( + # See https://github.com/cylc/cylc-flow/issues/5844 + "foo => bar[1649]", + 'illegal cycle point offset on the right', + id='no-cycle-point-RHS' + ), + param( + "foo => bar[1649] => baz", + '!illegal cycle point offset on the right', + id='ignore-not-exclusively-RHS-cycle-point' + ), + param( + "foo => bar[1649]\nbar[1649] => baz", + '!illegal cycle point offset on the right', + id='ignore-not-exclusively-RHS-cycle-point2' + ), ] ) def test_graph_syntax_errors(graph, expected_err): """Test various graph syntax errors.""" - with pytest.raises(GraphParseError) as cm: + if expected_err[0] == '!': + # Assert method doesn't fail: GraphParser().parse_graph(graph) - assert expected_err in str(cm.value) + else: + with pytest.raises(GraphParseError) as cm: + GraphParser().parse_graph(graph) + assert expected_err in str(cm.value) def test_parse_graph_simple(): From a6ca86fbb6642b04d44988defca4630aa97cf006 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 12 Feb 2024 11:06:30 +0000 Subject: [PATCH 2/8] Restore exiting on RHS containing `[` behaviour. --- cylc/flow/graph_parser.py | 16 +++++++++------- tests/unit/test_graph_parser.py | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index f63d7193417..83fc75a1335 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -534,22 +534,24 @@ def _proc_dep_pair( if right.count("(") != right.count(")"): raise GraphParseError(mismatch_msg.format(right)) + # Split right side on AND. + rights = right.split(self.__class__.OP_AND) + if '' in rights or right and not all(rights): + raise GraphParseError( + f"Null task name in graph: {left} => {right}") + # Ignore/Error cycle point offsets on the right side: # (Note we can only ban this for nodes at the end of chains) if '[' in right: - if right in terminals: + if left and right in terminals: + # This right hand side is at the end of a chain: raise GraphParseError( 'ERROR, illegal cycle point offset on the right:' f' {left} => {right}') else: + # This right hand side is a left elsewhere: return - # Split right side on AND. - rights = right.split(self.__class__.OP_AND) - if '' in rights or right and not all(rights): - raise GraphParseError( - f"Null task name in graph: {left} => {right}") - lefts: Union[List[str], List[Optional[str]]] if not left or (self.__class__.OP_OR in left or '(' in left): # Treat conditional or parenthesised expressions as a single entity diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index f739a79d09b..59058b6f78b 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -916,3 +916,28 @@ def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]): for task, trigs in gp.triggers.items() } assert triggers == expected_triggers + + +@pytest.mark.parametrize( + 'args, err', + ( + # Error if offset in terminal RHS: + param((('a', 'b[-P42M]'), {'b[-P42M]'}), 'illegal cycle point offset'), + # No error if offset in NON-terminal RHS: + param((('a', 'b[-P42M]'), {}), None), + # Null task name in graph warns before cycle point offsets: + param((('a', 'b[-P42M] &'), {}), '^Null task name'), + # Don't check the left hand side if this has a non-terminal RHS: + param((('a &', 'b[-P42M]'), {}), None), + ) +) +def test_proc_dep_pair(args, err): + """ + Unit tests for _proc_dep_pair. + """ + gp = GraphParser() + if err: + with pytest.raises(GraphParseError, match=err): + gp._proc_dep_pair(*args) + else: + assert gp._proc_dep_pair(*args) is None From 50163fcc8958c38d0bee8840cf4e8bbe027cc9e3 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 12 Feb 2024 13:24:23 +0000 Subject: [PATCH 3/8] correction to check order: --- cylc/flow/graph_parser.py | 12 ++++++------ tests/unit/test_graph_parser.py | 2 -- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index 83fc75a1335..61a215d8cc7 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -534,12 +534,6 @@ def _proc_dep_pair( if right.count("(") != right.count(")"): raise GraphParseError(mismatch_msg.format(right)) - # Split right side on AND. - rights = right.split(self.__class__.OP_AND) - if '' in rights or right and not all(rights): - raise GraphParseError( - f"Null task name in graph: {left} => {right}") - # Ignore/Error cycle point offsets on the right side: # (Note we can only ban this for nodes at the end of chains) if '[' in right: @@ -552,6 +546,12 @@ def _proc_dep_pair( # This right hand side is a left elsewhere: return + # Split right side on AND. + rights = right.split(self.__class__.OP_AND) + if '' in rights or right and not all(rights): + raise GraphParseError( + f"Null task name in graph: {left} => {right}") + lefts: Union[List[str], List[Optional[str]]] if not left or (self.__class__.OP_OR in left or '(' in left): # Treat conditional or parenthesised expressions as a single entity diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index 59058b6f78b..dd8cc8d141e 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -925,8 +925,6 @@ def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]): param((('a', 'b[-P42M]'), {'b[-P42M]'}), 'illegal cycle point offset'), # No error if offset in NON-terminal RHS: param((('a', 'b[-P42M]'), {}), None), - # Null task name in graph warns before cycle point offsets: - param((('a', 'b[-P42M] &'), {}), '^Null task name'), # Don't check the left hand side if this has a non-terminal RHS: param((('a &', 'b[-P42M]'), {}), None), ) From 5ad16119942d50a977c9d3047ca633da98590e49 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 20 Feb 2024 09:03:28 +0000 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- cylc/flow/graph_parser.py | 14 ++++++-------- tests/unit/test_graph_parser.py | 29 ++++++++++++----------------- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index 61a215d8cc7..bec2df86a6f 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -463,9 +463,8 @@ 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: - terminals = { - right for right in {right for _, right in pairs} - if right not in {left for left, _ in pairs}} + pairs_dict = dict(pairs) + terminals = set(pairs_dict.values()).difference(pairs_dict.keys()) for pair in pairs: self._proc_dep_pair(pair, terminals) @@ -514,7 +513,7 @@ def _proc_dep_pair( 'right' can be one or more node names joined by AND. 'right' can't be None or "". terminals: - Lits of nodes which are _only_ on the RH end of chains. + Nodes which are _only_ on the RH end of chains. """ left, right = pair # Raise error for right-hand-side OR operators. @@ -534,16 +533,15 @@ def _proc_dep_pair( if right.count("(") != right.count(")"): raise GraphParseError(mismatch_msg.format(right)) - # Ignore/Error cycle point offsets on the right side: - # (Note we can only ban this for nodes at the end of chains) + # Raise error for cycle point offsets at the end of chains if '[' in right: - if left and right in terminals: + if left and (right in terminals): # This right hand side is at the end of a chain: raise GraphParseError( 'ERROR, illegal cycle point offset on the right:' f' {left} => {right}') else: - # This right hand side is a left elsewhere: + # This RHS is also a LHS in a chain: return # Split right side on AND. diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index dd8cc8d141e..f55de9e0d1f 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -139,27 +139,13 @@ def test_graph_syntax_errors_2(seq, graph, expected_err): 'illegal cycle point offset on the right', id='no-cycle-point-RHS' ), - param( - "foo => bar[1649] => baz", - '!illegal cycle point offset on the right', - id='ignore-not-exclusively-RHS-cycle-point' - ), - param( - "foo => bar[1649]\nbar[1649] => baz", - '!illegal cycle point offset on the right', - id='ignore-not-exclusively-RHS-cycle-point2' - ), ] ) def test_graph_syntax_errors(graph, expected_err): """Test various graph syntax errors.""" - if expected_err[0] == '!': - # Assert method doesn't fail: + with pytest.raises(GraphParseError) as cm: GraphParser().parse_graph(graph) - else: - with pytest.raises(GraphParseError) as cm: - GraphParser().parse_graph(graph) - assert expected_err in str(cm.value) + assert expected_err in str(cm.value) def test_parse_graph_simple(): @@ -397,7 +383,16 @@ def test_line_continuation(): foo => bar bar:succeed => baz """ - ] + ], + [ + """ + foo => bar[1649] => baz + """, + """ + foo => bar[1649] + bar[1649] => baz + """ + ], ] ) def test_trigger_equivalence(graph1, graph2): From 0b55e89c8c2bc0c48d26031d73618ef1f4eb218f Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 27 Mar 2024 11:08:54 +0000 Subject: [PATCH 5/8] Update changes.d/fix.5924.md Co-authored-by: Hilary James Oliver --- changes.d/fix.5924.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes.d/fix.5924.md b/changes.d/fix.5924.md index be3ba7ad429..7ce2caf7777 100644 --- a/changes.d/fix.5924.md +++ b/changes.d/fix.5924.md @@ -1 +1 @@ -Ensure that graphs with dependency offsets on the right hand side are flagged to users. \ No newline at end of file +Validation: a cycle offset can only appear on the right of a dependency if the task's cycling is defined elsewhere with no offset. \ No newline at end of file From 36467e1b6cc4466997afb710ff455403d160fded Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 27 Mar 2024 14:57:29 +0000 Subject: [PATCH 6/8] Update cylc/flow/graph_parser.py --- cylc/flow/graph_parser.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index bec2df86a6f..36db8ad4d3e 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -538,7 +538,7 @@ def _proc_dep_pair( if left and (right in terminals): # This right hand side is at the end of a chain: raise GraphParseError( - 'ERROR, illegal cycle point offset on the right:' + '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: From 224faf80ae7cc53ed0079ebde4e32b502e034c27 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 27 Mar 2024 15:01:18 +0000 Subject: [PATCH 7/8] fix flake8 --- cylc/flow/graph_parser.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index 36db8ad4d3e..92427b3cc4b 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -538,7 +538,8 @@ def _proc_dep_pair( 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):' + '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: From b4cebe6aae76228d7677606166c9d6b556ee70b7 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 28 Mar 2024 08:15:52 +0000 Subject: [PATCH 8/8] fix test broken by warning content change --- tests/unit/test_graph_parser.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index f55de9e0d1f..4da3a8c22d9 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -136,7 +136,7 @@ def test_graph_syntax_errors_2(seq, graph, expected_err): param( # See https://github.com/cylc/cylc-flow/issues/5844 "foo => bar[1649]", - 'illegal cycle point offset on the right', + 'Invalid cycle point offsets only on right', id='no-cycle-point-RHS' ), ] @@ -917,7 +917,7 @@ def test_RHS_AND(graph: str, expected_triggers: Dict[str, List[str]]): 'args, err', ( # Error if offset in terminal RHS: - param((('a', 'b[-P42M]'), {'b[-P42M]'}), 'illegal cycle point offset'), + 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: