diff --git a/cylc/flow/config.py b/cylc/flow/config.py index f942eaebafb..e63300bd7de 100644 --- a/cylc/flow/config.py +++ b/cylc/flow/config.py @@ -1926,6 +1926,7 @@ def load_graph(self): self.workflow_polling_tasks.update( parser.workflow_state_polling_tasks) self._proc_triggers(parser, seq, task_triggers) + self.set_required_outputs(task_output_opt) # Detect use of xtrigger names with '@' prefix (creates a task). @@ -1951,10 +1952,10 @@ def _proc_triggers(self, parser, seq, task_triggers): # foo|bar => baz # @x => baz # """ - # - [] foo - # - [], bar - # - ['foo:succeeded', 'bar:succeeded'], baz - # - ['@x'], baz + # - ([], foo) + # - ([], bar) + # - (['foo:succeeded', 'bar:succeeded'], baz) + # - (['@x'], baz) self.generate_edges(expr, orig, lefts, right, seq, suicide) # Lefts can be null; all appear on RHS once so can generate @@ -1974,8 +1975,7 @@ def _proc_triggers(self, parser, seq, task_triggers): # RHS quals not needed now (used already for taskdef outputs) right = parser.REC_QUAL.sub('', right) self.generate_triggers( - expr, lefts, right, seq, suicide, task_triggers - ) + expr, lefts, right, seq, suicide, task_triggers) if suicide: suicides += 1 @@ -1986,17 +1986,17 @@ def _proc_triggers(self, parser, seq, task_triggers): ) def set_required_outputs( - self, task_output_opt: Dict[Tuple[str, str], Tuple[bool, bool]] + self, task_output_opt: Dict[Tuple[str, str], Tuple[bool, bool, bool]] ) -> None: """set optional/required status of parsed task outputs. Args: - task_output_opt: {(task, output): (is-optional, _)} + task_output_opt: {(task, output): (is-optional, default, is_set)} """ for name, taskdef in self.taskdefs.items(): for output in taskdef.outputs: try: - optional, _ = task_output_opt[(name, output)] + optional, _, _ = task_output_opt[(name, output)] except KeyError: # Output not used in graph. continue diff --git a/cylc/flow/graph_parser.py b/cylc/flow/graph_parser.py index 4dc0d2a4dde..2153747dd48 100644 --- a/cylc/flow/graph_parser.py +++ b/cylc/flow/graph_parser.py @@ -27,7 +27,6 @@ ) import cylc.flow.flags -from cylc.flow import LOG from cylc.flow.exceptions import GraphParseError from cylc.flow.param_expand import GraphExpander from cylc.flow.task_id import TaskID @@ -90,7 +89,6 @@ class GraphParser: """ CYLC7_COMPAT = "CYLC 7 BACK-COMPAT" - FAM_TWEAK = "BUT by the family trigger exemption" OP_AND = '&' OP_OR = '|' @@ -136,24 +134,20 @@ class GraphParser: QUAL_FAM_FINISH_ANY: (TASK_OUTPUT_FINISHED, False), } - # Map of family trigger type to inferred member success/fail - # optionality. True means optional, False means required. - # (Started is never optional; it is only checked at task finish.) - fam_to_mem_optional_output_map: Dict[str, Tuple[str, bool]] = { - # Required outputs. - QUAL_FAM_START_ALL: (TASK_OUTPUT_STARTED, False), - QUAL_FAM_START_ANY: (TASK_OUTPUT_STARTED, False), - QUAL_FAM_SUCCEED_ALL: (TASK_OUTPUT_SUCCEEDED, False), - QUAL_FAM_FAIL_ALL: (TASK_OUTPUT_FAILED, False), - QUAL_FAM_SUBMIT_ALL: (TASK_OUTPUT_SUBMITTED, False), - QUAL_FAM_SUBMIT_FAIL_ALL: (TASK_OUTPUT_SUBMIT_FAILED, False), - # Optional outputs. - QUAL_FAM_SUBMIT_FAIL_ANY: (TASK_OUTPUT_SUBMIT_FAILED, True), - QUAL_FAM_SUCCEED_ANY: (TASK_OUTPUT_SUCCEEDED, True), - QUAL_FAM_FAIL_ANY: (TASK_OUTPUT_FAILED, True), - QUAL_FAM_FINISH_ALL: (TASK_OUTPUT_SUCCEEDED, True), - QUAL_FAM_FINISH_ANY: (TASK_OUTPUT_SUCCEEDED, True), - QUAL_FAM_SUBMIT_ANY: (TASK_OUTPUT_SUBMITTED, True), + # Map family pseudo triggers to affected member outputs. + fam_to_mem_output_map: Dict[str, List[str]] = { + QUAL_FAM_START_ANY: [TASK_OUTPUT_STARTED], + QUAL_FAM_START_ALL: [TASK_OUTPUT_STARTED], + QUAL_FAM_SUCCEED_ANY: [TASK_OUTPUT_SUCCEEDED], + QUAL_FAM_SUCCEED_ALL: [TASK_OUTPUT_SUCCEEDED], + QUAL_FAM_FAIL_ANY: [TASK_OUTPUT_FAILED], + QUAL_FAM_FAIL_ALL: [TASK_OUTPUT_FAILED], + QUAL_FAM_SUBMIT_ANY: [TASK_OUTPUT_SUBMITTED], + QUAL_FAM_SUBMIT_ALL: [TASK_OUTPUT_SUBMITTED], + QUAL_FAM_SUBMIT_FAIL_ANY: [TASK_OUTPUT_SUBMIT_FAILED], + QUAL_FAM_SUBMIT_FAIL_ALL: [TASK_OUTPUT_SUBMIT_FAILED], + QUAL_FAM_FINISH_ANY: [TASK_OUTPUT_SUCCEEDED, TASK_OUTPUT_FAILED], + QUAL_FAM_FINISH_ALL: [TASK_OUTPUT_SUCCEEDED, TASK_OUTPUT_FAILED] } _RE_SUICIDE = r'(?:!)?' @@ -188,7 +182,7 @@ class GraphParser: (?:(?:{TaskID.NAME_RE} # node name (?:{_RE_PARAMS})?|{_RE_PARAMS}))+ # allow task to repeat (?:{_RE_OFFSET})? # cycle point offset - (?:{_RE_QUAL})? # qualifier + (?:{_RE_QUAL})? # qualifier (?:{_RE_OPT})? # optional output indicator """, re.X @@ -199,7 +193,7 @@ class GraphParser: rf""" ({_RE_NODE_OR_XTRIG}) # node name ({_RE_OFFSET})? # cycle point offset - ({_RE_QUAL})? # trigger qualifier + ({_RE_QUAL})? # trigger qualifier ({_RE_OPT})? # optional output indicator """, re.X @@ -244,10 +238,10 @@ class GraphParser: REC_RHS_NODE = re.compile( rf""" - (!)? # suicide mark - ({_RE_NODE}) # node name + (!)? # suicide mark + ({_RE_NODE}) # node name ({_RE_QUAL})? # trigger qualifier - ({_RE_OPT})? # optional output indicator + ({_RE_OPT})? # optional output indicator """, re.X ) @@ -257,7 +251,7 @@ def __init__( family_map: Optional[Dict[str, List[str]]] = None, parameters: Optional[Dict] = None, task_output_opt: - Optional[Dict[Tuple[str, str], Tuple[bool, bool]]] = None + Optional[Dict[Tuple[str, str], Tuple[bool, bool, bool]]] = None ) -> None: """Initialize the graph string parser. @@ -267,7 +261,7 @@ def __init__( parameters: task parameters for expansion here task_output_opt: - {(task-name, output-name): (is-optional, is-fam-member)} + {(name, output): (is-optional, is-opt-default, is-fixed)} passed in to allow checking across multiple graph strings """ @@ -587,19 +581,20 @@ def _proc_dep_pair( # Avoid @xtrigger nodes. continue if name in self.family_map: - # Family; deal with members + # Family; deal with members. try: family_trig_map[(name, trig)] = ( self.__class__.fam_to_mem_trigger_map[trig] ) except KeyError: - # Unqualified (FAM => foo) or bad (FAM:bad => foo). - raise GraphParseError(f"Bad family trigger in {expr}") + # "FAM:bad => foo" in LHS (includes "FAM => bar" too). + raise GraphParseError( + f"Illegal family trigger in {expr}") else: - # Not family + # Not a family. if trig in self.__class__.fam_to_mem_trigger_map: - raise GraphParseError("family trigger on non-" - f"family namespace {expr}") + raise GraphParseError( + "family trigger on non-family namespace {expr}") # remove '?' from expr (not needed in logical trigger evaluation) expr = re.sub(self.__class__._RE_OPT, '', expr) @@ -622,6 +617,7 @@ def _families_all_to_all( expr: the associated graph expression for this graph line """ + # Process left-side expression for defining triggers. n_info = [] n_expr = expr for name, offset, trig, _ in info: @@ -679,8 +675,7 @@ def _set_triggers( oexp = re.sub(r'(&|\|)', r' \1 ', orig_expr) oexp = re.sub(r':succeeded', '', oexp) raise GraphParseError( - f"{oexp} can't trigger both {name} and !{name}" - ) + f"{oexp} can't trigger both {name} and !{name}") # Record triggers self.triggers.setdefault(name, {}) @@ -696,82 +691,67 @@ def _set_output_opt( suicide: bool, fam_member: bool = False ) -> None: - """Set optional/required status of a task output. - - And check consistency with previous settings of the same item, and - of its opposite if relevant (succeed/fail). + """Set or check consistency of optional/required output. Args: name: task name output: task output name optional: is the output optional? suicide: is this from a suicide trigger? - fam_member: is this from an expanded family expression? + fam_member: is this from an expanded family trigger? """ if cylc.flow.flags.cylc7_back_compat: - # Set all outputs optional. Set success requried later. - # Family member or not irrelevant here for back compat. - self.task_output_opt[(name, output)] = (True, False) + # Set all outputs optional (set :succeed required elsewhere). + self.task_output_opt[(name, output)] = (True, True, True) return - if fam_member and output == "" or suicide: - # Do not infer output optionality from suicide triggers - # or from a family name on the right side. + # Do not infer output optionality from suicide triggers: + if suicide: return if output == TASK_OUTPUT_FINISHED: - if optional: + # Interpret :finish pseudo-output + if not optional: raise GraphParseError( - f"The \"{output}\" pseudo-output can't be optional" - ) - # However, from foo:finished we do infer that foo:succeeded and - # foo:failed are optional. - optional = True - for inferred_output in [ - TASK_OUTPUT_SUCCEEDED, - TASK_OUTPUT_FAILED - ]: + f"Pseudo-output {name}:{output} must be optional") + for outp in [TASK_OUTPUT_SUCCEEDED, TASK_OUTPUT_FAILED]: self._set_output_opt( - name, inferred_output, optional, suicide, fam_member - ) + name, outp, optional, suicide, fam_member) try: - already_optional, already_fam_member = ( - self.task_output_opt[(name, output)] - ) + prev_optional, prev_default, prev_fixed = ( + self.task_output_opt[(name, output)]) except KeyError: - # Not already_optional in map; add it. - self.task_output_opt[(name, output)] = optional, fam_member - already_optional = optional - is_fam = fam_member + # Not already set; set it. Fix it if not fam_member. + self.task_output_opt[(name, output)] = ( + optional, optional, not fam_member) else: - is_fam = already_fam_member or fam_member - err = None - if already_optional and not optional: - err = ( - f"Output {name}:{output} is optional" - " so it can't also be required." - ) - elif not already_optional and optional: - err = ( - f"Output {name}:{output} is required" - " so it can't also be optional." - ) - if err: - if not is_fam: - raise GraphParseError(err) - # is_fam - reason = self.__class__.FAM_TWEAK - LOG.warning(f"{err}\n...{reason}: making it optional.") - self.task_output_opt[(name, output)] = (True, is_fam) + # Already set; check consistency with previous. + if prev_fixed: + # optionality fixed already + if fam_member: + pass + else: + if optional != prev_optional: + raise GraphParseError( + f"Output {name}:{output} can't be both required" + " and optional") else: - # Update in case we discovered output is actually in a family - self.task_output_opt[(name, output)] = ( - already_optional, is_fam) + # optionality not fixed yet (only family default) + if fam_member: + # family defaults must be consistent + if optional != prev_default: + raise GraphParseError( + f"Output {name}:{output} can't default to both" + " optional and required (via family trigger" + " defaults)") + else: + # fix the optionality now + self.task_output_opt[(name, output)] = ( + optional, prev_default, True) - # Check opposite output, if relevant. - # Opposites must both be optional if both are referenced. + # Check opposite output where appropriate. for opposites in [ (TASK_OUTPUT_SUCCEEDED, TASK_OUTPUT_FAILED), (TASK_OUTPUT_SUBMITTED, TASK_OUTPUT_SUBMIT_FAILED) @@ -781,39 +761,28 @@ def _set_output_opt( succeed, fail = opposites opposite = fail if output == succeed else succeed try: - already_opposite, already_fam_member = ( + opp_optional, opp_default, opp_fixed = ( self.task_output_opt[(name, opposite)] ) except KeyError: - # opposite not set + # opposite not set, no need to check continue else: - is_fam = is_fam or already_fam_member - err = None - if not already_optional and not already_opposite: - err = ( - f"Output {name}:{opposite} is required" - f" so {name}:{output} can't be required." - ) - elif already_optional and not already_opposite: - err = ( - f"Output {name}:{opposite} is required" - f" so {name}:{output} can't be optional." - ) - elif not already_optional and already_opposite: - err = ( - f"Output {name}:{opposite} is optional" - f" so {name}:{output} can't be required." - ) - if err is not None: - # (can get here after auto-setting opposites to optional) - if not is_fam: - raise GraphParseError(err) - # is_fam - reason = self.__class__.FAM_TWEAK - LOG.warning(f"{err}\n...{reason}: making both optional.") - self.task_output_opt[(name, output)] = (True, is_fam) - self.task_output_opt[(name, opposite)] = (True, is_fam) + # opposite already set; check consistency + optional, default, oset = ( + self.task_output_opt[(name, output)] + ) + msg = (f"Opposite outputs {name}:{output} and {name}:" + f"{opposite} must both be optional if both are used") + if fam_member or not opp_fixed: + if not optional or not opp_default: + raise GraphParseError( + msg + " (via family trigger defaults)") + elif not optional or not opp_optional: + raise GraphParseError( + msg + " (via family trigger)") + elif not optional or not opp_optional: + raise GraphParseError(msg) def _compute_triggers( self, @@ -858,43 +827,36 @@ def _compute_triggers( if output: output = output.strip(self.__class__.QUALIFIER) - if name not in self.family_map: - if output: - output = TaskTrigger.standardise_name(output) - else: + if name in self.family_map: + fam = True + mems = self.family_map[name] + if not output: + # (Plain family name on RHS). # Make implicit success explicit. - output = TASK_OUTPUT_SUCCEEDED - - self._set_triggers( - name, suicide, trigs, expr, orig_expr - ) - self._set_output_opt( - name, output, optional, suicide, False - ) - else: - # Family. - if optional: - raise GraphParseError( - "Family triggers can't be optional: " - f"{name}:{output}{self.__class__.OPTIONAL}" - ) - - # Derive member optional/required outputs. + output = self.__class__.QUAL_FAM_SUCCEED_ALL + else: + if not optional and output.startswith("finish"): + raise GraphParseError( + f"Family pseudo-output {name}:{output} must be" + " optional") try: - output, optional = ( - self.__class__.fam_to_mem_optional_output_map[output] - ) + outputs = self.__class__.fam_to_mem_output_map[output] except KeyError: - if output: - raise GraphParseError( - f"Illegal family trigger: {name}:{output}" - ) + # Illegal family trigger on RHS of a pair. + raise GraphParseError( + f"Illegal family trigger: {name}:{output}") + else: + fam = False + if not output: + # Make implicit success explicit. + output = TASK_OUTPUT_SUCCEEDED + else: + # Convert to standard output names if necessary. + output = TaskTrigger.standardise_name(output) + mems = [name] + outputs = [output] - for member in self.family_map[name]: - # Expand to family members. - self._set_triggers( - member, suicide, trigs, expr, orig_expr - ) - self._set_output_opt( - member, output, optional, suicide, True - ) + for mem in mems: + self._set_triggers(mem, suicide, trigs, expr, orig_expr) + for output in outputs: + self._set_output_opt(mem, output, optional, suicide, fam) diff --git a/cylc/flow/graphnode.py b/cylc/flow/graphnode.py index e7c1ce90a98..46c565fed87 100644 --- a/cylc/flow/graphnode.py +++ b/cylc/flow/graphnode.py @@ -27,7 +27,8 @@ class GraphNodeParser: """Provide graph node parsing and caching service. - Optional outputs are stripped in graph_parser.py before this gets used. + Optional output notation is stripped out before this class gets used. + TODO is any of this redundant with code in the graph_parser module? """ # Match a graph node string. REC_NODE = re.compile( diff --git a/cylc/flow/task_pool.py b/cylc/flow/task_pool.py index e3758ef5671..203e8fbf768 100644 --- a/cylc/flow/task_pool.py +++ b/cylc/flow/task_pool.py @@ -294,7 +294,7 @@ def release_runahead_tasks(self): Incomplete tasks and partially satisfied prerequisites are counted toward the runahead limit, because they represent tasks that will - (or may, in the case or prerequisites) yet run at their cycle points. + (or may, in the case of prerequisites) yet run at their cycle points. Note runahead release can cause the task pool to change size because we spawn parentless tasks on previous-instance release. diff --git a/cylc/flow/taskdef.py b/cylc/flow/taskdef.py index cba9736ff4b..02e1a3b8863 100644 --- a/cylc/flow/taskdef.py +++ b/cylc/flow/taskdef.py @@ -23,7 +23,7 @@ from cylc.flow.task_id import TaskID from cylc.flow.task_state import ( TASK_OUTPUT_SUBMITTED, - TASK_OUTPUT_STARTED, + TASK_OUTPUT_SUBMIT_FAILED, TASK_OUTPUT_SUCCEEDED, TASK_OUTPUT_FAILED ) @@ -173,15 +173,18 @@ def set_required_output(self, output, required): def tweak_outputs(self): """Output consistency checking and tweaking.""" - # If :started is used alone, assume :succeeded is required. + # If :succeed or :fail not set, assume success is required. + # Unless submit (and submit-fail) is optional (don't stall + # because of missing succeed if submit is optional). if ( - self.outputs[TASK_OUTPUT_STARTED][1] is not None - and self.outputs[TASK_OUTPUT_SUCCEEDED][1] is None + self.outputs[TASK_OUTPUT_SUCCEEDED][1] is None and self.outputs[TASK_OUTPUT_FAILED][1] is None + and self.outputs[TASK_OUTPUT_SUBMITTED][1] is not False + and self.outputs[TASK_OUTPUT_SUBMIT_FAILED][1] is not False ): self.set_required_output(TASK_OUTPUT_SUCCEEDED, True) - # In Cylc 7 back compat mode, make success outputs required. + # In Cylc 7 back compat mode, make all success outputs required. if cylc.flow.flags.cylc7_back_compat: for output in [ TASK_OUTPUT_SUBMITTED, diff --git a/tests/flakyfunctional/cylc-show/00-simple/flow.cylc b/tests/flakyfunctional/cylc-show/00-simple/flow.cylc index 2cfc13a61a1..99cdad79daf 100644 --- a/tests/flakyfunctional/cylc-show/00-simple/flow.cylc +++ b/tests/flakyfunctional/cylc-show/00-simple/flow.cylc @@ -11,8 +11,8 @@ [[graph]] PT1H = """ bar => foo - foo:start => SHOW - SHOW:finish-all => end + foo:start => SHOW? + SHOW:finish-all? => end """ [runtime] [[foo]] diff --git a/tests/functional/broadcast/00-simple/flow.cylc b/tests/functional/broadcast/00-simple/flow.cylc index 6667dbb0bdb..24c657297fd 100644 --- a/tests/functional/broadcast/00-simple/flow.cylc +++ b/tests/functional/broadcast/00-simple/flow.cylc @@ -19,8 +19,8 @@ generated reference version. [[graph]] R1 = prep => check & foo T00 = """ - foo => bar & baz & qux & wibble => ENS - ENS:finish-all => wobble + foo => bar & baz & qux & wibble => ENS? + ENS:finish-all? => wobble # Ensure that the first cycle completely finishes before # the second (last) one, so that the broadcast to foo in the first # cycle automatically EXPIRES before the workflow shuts down: diff --git a/tests/functional/cylc-get-config/00-simple.t b/tests/functional/cylc-get-config/00-simple.t index 12dcbc5b0dc..c9f917228a2 100755 --- a/tests/functional/cylc-get-config/00-simple.t +++ b/tests/functional/cylc-get-config/00-simple.t @@ -35,7 +35,7 @@ cmp_ok "${TEST_NAME}.stderr" - VAR +R1 = OPS:finish-all? => VAR __OUT__ cmp_ok "${TEST_NAME}.stderr" - VAR +OPS:finish-all? => VAR __OUT__ cmp_ok "${TEST_NAME}.stderr" - VAR + R1 = OPS:finish-all? => VAR diff --git a/tests/functional/cylc-kill/02-submitted/flow.cylc b/tests/functional/cylc-kill/02-submitted/flow.cylc index a300af698c5..33dba4e8433 100644 --- a/tests/functional/cylc-kill/02-submitted/flow.cylc +++ b/tests/functional/cylc-kill/02-submitted/flow.cylc @@ -6,8 +6,8 @@ [scheduling] [[graph]] R1=""" -KILLABLE:submit-all => killer -KILLABLE:submit-fail-all => stopper +KILLABLE:submit-all? => killer +KILLABLE:submit-fail-all? => stopper """ [runtime] [[KILLABLE]] diff --git a/tests/functional/optional-outputs/03-c7backcompat.t b/tests/functional/optional-outputs/03-c7backcompat.t index 8cdf8a29e83..3da19fa7d10 100644 --- a/tests/functional/optional-outputs/03-c7backcompat.t +++ b/tests/functional/optional-outputs/03-c7backcompat.t @@ -30,10 +30,8 @@ install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}" TEST_NAME="${TEST_NAME_BASE}-validate_as_c8" run_fail "${TEST_NAME}" cylc validate "${WORKFLOW_NAME}" -cmp_ok "${TEST_NAME}.stderr" <<__ERR__ -GraphParseError: Output foo:succeeded is required so \ -foo:failed can't be required. -__ERR__ +ERR="GraphParseError: Opposite outputs .* must both be optional if both are used" +grep_ok "${ERR}" "${TEST_NAME}.stderr" # Rename config to "suite.rc" mv "${WORKFLOW_RUN_DIR}/flow.cylc" "${WORKFLOW_RUN_DIR}/suite.rc" diff --git a/tests/functional/reload/06-graphing-fam/flow.cylc b/tests/functional/reload/06-graphing-fam/flow.cylc index 8229b9cdee9..11ca506b02b 100644 --- a/tests/functional/reload/06-graphing-fam/flow.cylc +++ b/tests/functional/reload/06-graphing-fam/flow.cylc @@ -5,16 +5,16 @@ [scheduling] [[graph]] R1 = """ - reloader => inter => BAR # marker1 - BAR:finish-all => FOO # marker2 + reloader => inter => BAR? # marker1 + BAR:finish-all? => FOO # marker2 """ [runtime] [[inter]] [[reloader]] script = """ # change the order of FOO and BAR in the graphing section: -perl -pi -e 's/(reloader => inter => )BAR( # marker1)/\1FOO\2/' $CYLC_WORKFLOW_RUN_DIR/flow.cylc -perl -pi -e 's/( )BAR:finish-all => FOO( # marker2)/\1FOO:finish-all => BAR\2/' $CYLC_WORKFLOW_RUN_DIR/flow.cylc +perl -pi -e 's/(reloader => inter => )BAR\?( # marker1)/\1FOO?\2/' $CYLC_WORKFLOW_RUN_DIR/flow.cylc +perl -pi -e 's/( )BAR:finish-all\? => FOO( # marker2)/\1FOO:finish-all? => BAR\2/' $CYLC_WORKFLOW_RUN_DIR/flow.cylc # reload cylc reload "${CYLC_WORKFLOW_NAME}" cylc__job__poll_grep_workflow_log -F 'Reload completed' diff --git a/tests/functional/shutdown/07-task-fail/flow.cylc b/tests/functional/shutdown/07-task-fail/flow.cylc index 7d32ed9e100..210f484c985 100644 --- a/tests/functional/shutdown/07-task-fail/flow.cylc +++ b/tests/functional/shutdown/07-task-fail/flow.cylc @@ -5,7 +5,7 @@ [scheduling] [[graph]] - R1 = t1:finish => t2 + R1 = t1:finish? => t2 [runtime] [[t1]] diff --git a/tests/functional/shutdown/08-now1/flow.cylc b/tests/functional/shutdown/08-now1/flow.cylc index ab70384dd9c..72a0f87cd56 100644 --- a/tests/functional/shutdown/08-now1/flow.cylc +++ b/tests/functional/shutdown/08-now1/flow.cylc @@ -6,7 +6,7 @@ [scheduling] [[graph]] - R1 = t1:finish => t2 + R1 = t1:finish? => t2 [runtime] [[t1]] diff --git a/tests/functional/shutdown/09-now2/flow.cylc b/tests/functional/shutdown/09-now2/flow.cylc index 902f3553045..5aefaec3a99 100644 --- a/tests/functional/shutdown/09-now2/flow.cylc +++ b/tests/functional/shutdown/09-now2/flow.cylc @@ -6,7 +6,7 @@ [scheduling] [[graph]] - R1 = t1:finish => t2 + R1 = t1:finish? => t2 [runtime] [[t1]] diff --git a/tests/functional/triggering/00-recovery/flow.cylc b/tests/functional/triggering/00-recovery/flow.cylc index 145387f10d7..145b0fc3afc 100644 --- a/tests/functional/triggering/00-recovery/flow.cylc +++ b/tests/functional/triggering/00-recovery/flow.cylc @@ -17,7 +17,7 @@ final cycle point = 20110101T12 [[graph]] T00,T12 = """ - pre:finish => model? # finish trigger + pre:finish? => model? # finish trigger model:fail? => diagnose => recover # fail trigger model? => !diagnose & !recover # explicit success trigger model:succeed? | recover => post # conditional and explicit success diff --git a/tests/functional/triggering/00a-recovery/flow.cylc b/tests/functional/triggering/00a-recovery/flow.cylc index 5f8907c9dc0..26baf0de0a7 100644 --- a/tests/functional/triggering/00a-recovery/flow.cylc +++ b/tests/functional/triggering/00a-recovery/flow.cylc @@ -20,7 +20,7 @@ final cycle point = 20110101T12 [[graph]] T00,T12 = """ - pre:finish => model? # finish trigger + pre:finish? => model? # finish trigger model:fail? => diagnose => recover # fail trigger model:succeed? | recover => post # conditional and explicit success """ diff --git a/tests/functional/triggering/05-fam-finish-all.t b/tests/functional/triggering/05-fam-finish-all.t index 1762ffcbda8..0cf8402d294 100644 --- a/tests/functional/triggering/05-fam-finish-all.t +++ b/tests/functional/triggering/05-fam-finish-all.t @@ -15,7 +15,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Test correct expansion of FAM:finish-all +# Test correct expansion of 'FAM:finish-all?' . "$(dirname "$0")/test_header" set_test_number 2 reftest diff --git a/tests/functional/triggering/05-fam-finish-all/flow.cylc b/tests/functional/triggering/05-fam-finish-all/flow.cylc index 4896bac184a..d053c0c639b 100644 --- a/tests/functional/triggering/05-fam-finish-all/flow.cylc +++ b/tests/functional/triggering/05-fam-finish-all/flow.cylc @@ -4,7 +4,7 @@ [scheduling] [[graph]] - R1 = "FAM:finish-all => foo" + R1 = "FAM:finish-all? => foo" [runtime] [[FAM]] script = "false" diff --git a/tests/functional/triggering/06-fam-succeed-any/flow.cylc b/tests/functional/triggering/06-fam-succeed-any/flow.cylc index a0a67af52d4..830b3713c27 100644 --- a/tests/functional/triggering/06-fam-succeed-any/flow.cylc +++ b/tests/functional/triggering/06-fam-succeed-any/flow.cylc @@ -5,7 +5,7 @@ [scheduling] [[graph]] R1 = """ - FAM:succeed-any => foo + FAM:succeed-any? => foo a:fail? & c:fail? => handled """ [runtime] diff --git a/tests/functional/triggering/07-fam-fail-any/flow.cylc b/tests/functional/triggering/07-fam-fail-any/flow.cylc index ed760468b24..52d050b9a37 100644 --- a/tests/functional/triggering/07-fam-fail-any/flow.cylc +++ b/tests/functional/triggering/07-fam-fail-any/flow.cylc @@ -4,7 +4,7 @@ [scheduling] [[graph]] - R1 = "FAM:fail-any => foo" + R1 = "FAM:fail-any? => foo" [runtime] [[FAM]] script = "true" diff --git a/tests/functional/triggering/08-fam-finish-any.t b/tests/functional/triggering/08-fam-finish-any.t index 58172de16be..4d3636acaa1 100644 --- a/tests/functional/triggering/08-fam-finish-any.t +++ b/tests/functional/triggering/08-fam-finish-any.t @@ -15,7 +15,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Test correct expansion of FAM:finish-any +# Test correct expansion of 'FAM:finish-any?' . "$(dirname "$0")/test_header" set_test_number 2 reftest diff --git a/tests/functional/triggering/08-fam-finish-any/flow.cylc b/tests/functional/triggering/08-fam-finish-any/flow.cylc index 6d8790a829f..73fdf4e6b2e 100644 --- a/tests/functional/triggering/08-fam-finish-any/flow.cylc +++ b/tests/functional/triggering/08-fam-finish-any/flow.cylc @@ -1,6 +1,6 @@ [scheduling] [[graph]] - R1 = """FAM:finish-any => foo""" + R1 = """FAM:finish-any? => foo""" [runtime] [[FAM]] script = sleep 10 diff --git a/tests/functional/triggering/10-finish/flow.cylc b/tests/functional/triggering/10-finish/flow.cylc index 5a9856cc248..82f0c082dea 100644 --- a/tests/functional/triggering/10-finish/flow.cylc +++ b/tests/functional/triggering/10-finish/flow.cylc @@ -4,8 +4,8 @@ [scheduling] [[graph]] - R1 = """foo:finish => bar - baz:finish => qux""" + R1 = """foo:finish? => bar + baz:finish? => qux""" [runtime] [[foo]] script = false diff --git a/tests/functional/triggering/16-fam-expansion.t b/tests/functional/triggering/16-fam-expansion.t index e1c2b88b6fb..f568bd43c76 100644 --- a/tests/functional/triggering/16-fam-expansion.t +++ b/tests/functional/triggering/16-fam-expansion.t @@ -15,7 +15,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Test correct expansion of (FOO:finish-all & FOO:fail-any) +# Test correct expansion of (FOO:finish-all? & FOO:fail-any) . "$(dirname "$0")/test_header" #------------------------------------------------------------------------------- set_test_number 3 diff --git a/tests/functional/triggering/fam-expansion/flow.cylc b/tests/functional/triggering/fam-expansion/flow.cylc index a8e718994a6..8b4973e4a40 100644 --- a/tests/functional/triggering/fam-expansion/flow.cylc +++ b/tests/functional/triggering/fam-expansion/flow.cylc @@ -1,7 +1,7 @@ #!Jinja2 [scheduling] [[graph]] - R1 = "(FOO:finish-all & FOO:fail-any) => bar" + R1 = "(FOO:finish-all? & FOO:fail-any?) => bar" [runtime] [[FOO]] script = false diff --git a/tests/functional/validate/55-hyphen-finish.t b/tests/functional/validate/55-hyphen-finish.t index 9936d544c25..bbb1605eb89 100755 --- a/tests/functional/validate/55-hyphen-finish.t +++ b/tests/functional/validate/55-hyphen-finish.t @@ -24,7 +24,7 @@ set_test_number 1 cat >'flow.cylc' <<'__FLOW_CONFIG__' [scheduling] [[graph]] - R1 = foo-bar:finish => baz + R1 = foo-bar:finish? => baz [runtime] [[foo-bar,baz]] script = true diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index cbd4ed22104..36db207e79f 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -968,7 +968,7 @@ def test_undefined_custom_output(tmp_path): def test_invalid_custom_output_msg(tmp_path): - """Test invalid output message: colon not allowed.""" + """Test invalid output message (colon not allowed).""" flow_config = """ [scheduling] [[graph]] @@ -1027,3 +1027,52 @@ def test_c7_back_compat_optional_outputs(tmp_path, monkeypatch, caplog): assert required else: assert not required + + +@pytest.mark.parametrize( + 'graph', + [ + "foo:x => bar", + "foo:start => bar", + "foo:submit => bar", + ] +) +def test_implicit_success_required(tmp_path, graph): + """Check foo:succeed is required if success/fail not used in the graph.""" + flow_config = f""" + [scheduling] + [[graph]] + R1 = {graph} + [runtime] + [[bar]] + [[foo]] + [[[outputs]]] + x = "the quick brown fox" + """ + flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE) + flow_file.write_text(flow_config) + cfg = WorkflowConfig(workflow='blargh', fpath=flow_file, options=None) + assert cfg.taskdefs['foo'].outputs[TASK_OUTPUT_SUCCEEDED][1] + + +@pytest.mark.parametrize( + 'graph', + [ + "foo:submit? => bar", + "foo:submit-fail? => bar", + ] +) +def test_success_after_optional_submit(tmp_path, graph): + """Check foo:succeed is not required if foo:submit is optional.""" + flow_config = f""" + [scheduling] + [[graph]] + R1 = {graph} + [runtime] + [[bar]] + [[foo]] + """ + flow_file = tmp_path.joinpath(WorkflowFiles.FLOW_FILE) + flow_file.write_text(flow_config) + cfg = WorkflowConfig(workflow='blargh', fpath=flow_file, options=None) + assert not cfg.taskdefs['foo'].outputs[TASK_OUTPUT_SUCCEEDED][1] diff --git a/tests/unit/test_graph_parser.py b/tests/unit/test_graph_parser.py index f57b054161f..34f7306ce56 100644 --- a/tests/unit/test_graph_parser.py +++ b/tests/unit/test_graph_parser.py @@ -90,10 +90,6 @@ def test_parse_graph_fails_null_task_name(graph): "foo || bar => baz", "The graph OR operator is '|'" ], - [ - "foo:finish? => bar", - "The \"finished\" pseudo-output can't be optional" - ] ] ) def test_graph_syntax_errors(graph, expected_err): @@ -294,7 +290,7 @@ def test_line_continuation(): foo => bar""" ], [ - "foo:finished => bar", # finish trigger + "foo:finished? => bar", # finish trigger "(foo:succeed? | foo:fail?) => bar" ], [ @@ -350,7 +346,7 @@ def test_trigger_equivalence(graph1, graph2): ], [ {'FAM': ['m1', 'm2']}, - "FAM:finish-all => post", + "FAM:finish-all? => post", "((m1? | m1:fail?) & (m2? | m2:fail?)) => post" ] ] @@ -582,35 +578,35 @@ def test_task_optional_outputs(): x:fail? => y - foo:finish => bar + foo:finish? => bar """ ) for i in range(1, 4): for task in (f'a{i}', f'b{i}'): assert ( gp.task_output_opt[(task, TASK_OUTPUT_SUCCEEDED)] - == (REQUIRED, False) + == (REQUIRED, False, True) ) for task in (f'c{i}', f'd{i}'): assert ( gp.task_output_opt[(task, TASK_OUTPUT_SUCCEEDED)] - == (OPTIONAL, False) + == (OPTIONAL, True, True) ) assert ( gp.task_output_opt[('x', TASK_OUTPUT_FAILED)] - == (OPTIONAL, False) + == (OPTIONAL, True, True) ) assert ( gp.task_output_opt[('foo', TASK_OUTPUT_SUCCEEDED)] - == (OPTIONAL, False) + == (OPTIONAL, True, True) ) assert ( gp.task_output_opt[('foo', TASK_OUTPUT_FAILED)] - == (OPTIONAL, False) + == (OPTIONAL, True, True) ) @@ -618,11 +614,10 @@ def test_task_optional_outputs(): 'qual, task_output', [ ('start', TASK_OUTPUT_STARTED), - ('finish', TASK_OUTPUT_SUCCEEDED), ('succeed', TASK_OUTPUT_SUCCEEDED), + ('fail', TASK_OUTPUT_FAILED), ('submit', TASK_OUTPUT_SUBMITTED), ('submit-fail', TASK_OUTPUT_SUBMIT_FAILED), - ('fail', TASK_OUTPUT_FAILED) ] ) def test_family_optional_outputs(qual, task_output): @@ -634,55 +629,64 @@ def test_family_optional_outputs(qual, task_output): gp = GraphParser(fam_map) gp.parse_graph( f""" - # all f:{qual} required (except ":finish"): + # required FAM:{qual}-all => foo - - # except f2:{qual} made optional: + # optional member f2:{task_output}? - # all b:{qual} optional (except ":start"): + # required BAM:{qual}-any => bar """ ) # -all for member in ['f1', 'f2']: - optional = (qual == "finish") or (member == 'f2') - assert gp.task_output_opt[(member, task_output)] == (optional, True) + optional = (member == 'f2') + assert gp.task_output_opt[(member, task_output)][0] == optional # -any - optional = (qual != "start") + optional = False for member in ['b1', 'b2']: - assert gp.task_output_opt[(member, task_output)] == (optional, True) + assert gp.task_output_opt[(member, task_output)][0] == optional -def test_family_output_clash(caplog: pytest.LogCaptureFixture): - """Test member output optionality inferred from family triggers.""" +@pytest.mark.parametrize( + 'graph, error', + [ + [ + """FAM:succeed-all => foo + FAM:fail-all => foo""", + ("must both be optional if both are used (via family trigger" + " defaults") + ], + [ + """FAM:succeed-all => foo + FAM:succeed-any? => bar""", + ("can't default to both optional and required (via family trigger" + " defaults)") + ], + [ + "FAM:blargh-all => foo", # LHS + "Illegal family trigger" + ], + [ + "foo => FAM:blargh-all", # RHS + "Illegal family trigger" + ], + [ + "FAM => foo", # bare family on LHS + "Illegal family trigger" + ], + ] +) +def test_family_trigger_errors(graph, error): + """Test errors via bad family triggers and member output optionality.""" fam_map = { 'FAM': ['f1', 'f2'] } gp = GraphParser(fam_map) - gp.parse_graph( - """ - # f:succeeded required: - FAM:succeed-all => foo - """ - ) - # (Parse graph in two chunks for consistent order of results) - gp2 = GraphParser(fam_map, task_output_opt=gp.task_output_opt) - gp2.parse_graph( - """ - # f:failed required: - FAM:fail-all => foo - """ - ) - for mem in ['f1', 'f2']: - assert gp2.task_output_opt[(mem, "succeeded")] == (True, True) - expected_warning = ( - f"Output {mem}:succeeded is required so" - f" {mem}:failed can't be required.\n" - "...BUT by the family trigger exemption: making both optional." - ) - assert expected_warning in caplog.messages + with pytest.raises(GraphParseError) as cm: + gp.parse_graph(graph) + assert error in str(cm.value) @pytest.mark.parametrize( @@ -691,45 +695,26 @@ def test_family_output_clash(caplog: pytest.LogCaptureFixture): [ """a:x => b a:x? => c""", - "Output a:x is required so it can't also be optional.", + "Output a:x can't be both required and optional", ], [ """a? => c a => b""", - "Output a:succeeded is optional so it can't also be required.", + "Output a:succeeded can't be both required and optional", ], [ """a => c a:fail => b""", - ("Output a:succeeded is required so a:failed " - "can't be required."), + ("must both be optional if both are used"), ], [ - """a:finish => b + """a:fail? => b a => c""", - "Output a:succeeded is optional so it can't also be required.", + ("must both be optional if both are used"), ], [ - """a => c - a:finish => b""", - "Output a:succeeded is required so it can't also be optional.", - ], - # The next case is different to the previous case because - # :succeeded is processed first in the :finished pseudo-output. - [ - """a:fail => c - a:finish => b""", - "Output a:failed is required so a:succeeded can't be optional.", - ], - [ - """a:finish => b - a:fail => c""", - "Output a:failed is optional so it can't also be required.", - ], - [ - """a:fail? => b - a => c""", - "Output a:failed is optional so a:succeeded can't be required.", + "a:finish => b", + "Pseudo-output a:finished must be optional", ], ] ) @@ -738,57 +723,27 @@ def test_task_optional_output_errors_order( caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch ): - """Test optional output errors are raised as expected. - - Parse the graph lines separately (as for separate graph strings) to ensure - that order is preserved (the error is different depending on whether an - output gets set to optional or required first). - """ - graph1, graph2 = graph.split('\n') - gp1 = GraphParser() - gp1.parse_graph(graph1) - gp2 = GraphParser(task_output_opt=gp1.task_output_opt) + """Test optional output errors are raised as expected.""" + gp = GraphParser() with pytest.raises(GraphParseError) as cm: - gp2.parse_graph(graph2) + gp.parse_graph(graph) assert c8error in str(cm.value) # In Cylc 7 back compat mode these graphs should all pass with no warnings. monkeypatch.setattr('cylc.flow.flags.cylc7_back_compat', True) caplog.set_level(logging.WARNING, CYLC_LOG) - gp1 = GraphParser() - gp1.parse_graph(graph1) - gp2 = GraphParser(task_output_opt=gp1.task_output_opt) - gp2.parse_graph(graph2) + gp = GraphParser() + gp.parse_graph(graph) # No warnings logged: assert not caplog.messages # After graph parsing all Cylc 7 back compat outputs should be optional. # (Success outputs are set to required later, in taskdef processing.) - for (_, _), (optional, _) in gp2.task_output_opt.items(): + for (optional, _, _) in gp.task_output_opt.values(): assert optional -def test_fail_bare_family_trigger(): - """Test that "FAM => bar" (no :succeed-all etc.) raises an error.""" - gp = GraphParser({'FAM': ['m1', 'm2']}) - with pytest.raises(GraphParseError) as cm: - gp.parse_graph("FAM => f") - assert ( - str(cm.value).startswith("Bad family trigger in") - ) - - -def test_fail_family_trigger_optional(): - """Test that "FAM:fail-all? => bar" raises an error.""" - gp = GraphParser({'FAM': ['m1', 'm2']}) - with pytest.raises(GraphParseError) as cm: - gp.parse_graph("FAM:fail-all? => f") - assert ( - str(cm.value).startswith("Family triggers can't be optional") - ) - - @pytest.mark.parametrize( 'ftrig', GraphParser.fam_to_mem_trigger_map.keys()