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 <[email protected]>

* Deflake a test.

* Sort pairs to make errors deterministic.

* dict(was hiding stuff)

* tweak variable name (review suggestion)

---------

Co-authored-by: Tim Pillinger <[email protected]>
Co-authored-by: Tim Pillinger <[email protected]>

Author: hjoliver

changes.d/6526.fix.md

@@ -0,0 +1 @@
+Output optionality validation now checks tasks with cycle offsets.

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,15 +883,15 @@ 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:
                 output = output.strip(self.__class__.QUALIFIER)
 
             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)

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__'

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]]

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...
                 ''',
             },
         },

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):