validate: incorrect rejection of completion condition

[dpmatthews]

With this workflow:

[scheduling]
    [[graph]]
        R1 = "a => b"
[runtime]
    [[a]]
    [[b]]
        completion = succeeded or failed

cylc validate fails with:

WorkflowConfigError: b:succeeded is required in the graph, but optional in the completion expression
succeeded or failed

Since #6999, that is not correct and this workflow should validate.

[oliver-sanders]

This would appear to be a bug in the graph parser rather than the completion expression validation.

Inspecting the taskdef object for task b reveals that the succeeded output is marked as required:

cylc-flow/cylc/flow/taskdef.py

Line 184 in 0d4736c

self.outputs = {} # {output: (message, is_required)}

(Pdb) self.taskdefs['b'].outputs['succeeded']
('succeeded', True)
(Pdb) self.taskdefs['b'].outputs['failed']
('failed', None)

Which is curiously inconsistent with the failed output.

[hjoliver]

So without a custom completion expression, b on the right side defaults to success-required.

I know it can be argued that the completion expression should dictate that b is success-optional here, given that it only defaults to success-required in the graph, but it can also be argued the other way, and I don't recall that we decided to do that - did we??

As I recall, completion expressions may add additional information that can't be expressed in the graph, but should not be inconsistent with the graph. (Well, here there is at least apparent inconsistency because the output is not marked optional anywhere in the graph).

In my opinion, to avoid ambiguity, changing a default-required to optional should need an explicit ? in the graph.

Changing "bug" to "bug?" !

[hjoliver]

The user guide is kinda conflicted on this:

• This admonition says "success is presumed to be required unless explicitly stated otherwise, either in the graph ... or in the completion expression".
• But this admonition says "If task outputs are optional in the graph they must also be optional in the completion condition and vice versa." - i.e. if a task is optional in the completion condition it must also be optional in the graph.
  • (It has a left-side example, but that's a general statement).
  • (trying to spin this as the completion condition alone causing the graph to be consistent with it by implicitly overriding the default is a bit perverse).

I see #6999 added explicit tests for defaulting to success required, but did not use custom completion expressions. There's no mention of this completion-driven optionality on the "optional outputs tweak" proposal or PR. Did we discuss it anywhere else?

[hjoliver]

Inspecting the taskdef object for task b reveals that the succeeded output is marked as required:
... Which is curiously inconsistent with the failed output.

From the graph parser, which just parses the graph, b:succeeded defaults to required, but b:failed is completely undetermined by the graph config.

[oliver-sanders]

I don't see a conflict in those docs, pretty sure this is a bug (or is bug? an optional bug 😬?).

1. inferring optionality from the right side is NOT intuitive - right-side outputs have nothing to do with triggering! - Tweak output optionality inference to avoid confusing syntax. #6744

• If a task is referenced only on the RHS, we cannot infer optionality of :succeeded from the graph.
• I.E, the output is neither optional, nor required from this information alone.

2. success is presumed to be required unless explicitly stated otherwise, either in the graph ... or in the completion expression

• The completion expression and graph can both be used to infer optionality of :succeeded.
• If we cannot infer the optionality of :succeeded, we default to "required".

3. If task outputs are optional in the graph they must also be optional in the completion condition and vice versa.

• Optionality in the graph and completion expression must be consistent.
• Note that from point (1), we do not infer optionality from the RHS.

In my opinion, to avoid ambiguity, changing a default-required to optional should need an explicit ? in the graph.

This conflicts with point (1), i.e, - we do not infer optionality from the RHS, therefore there is no logical inconsistency because the graph does not infer that success is optional in the first place.

I think this is just a small oversight of #6999 which implemented its logic in the graph parser (where there are no completion expressions) rather than the config (where there are).

[hjoliver]

pretty sure this is a bug (or is bug? an optional bug 😬?).

"bug?" is "this may be a bug, but I'm not sure".

Note there is a distinction between inferring X and defaulting to X.

I said it can be argued both ways, so I'm not entirely averse to "fixing?" this "bug?", but I think I prefer the status quo, plus clarified documentation that an explicit left-side ? is needed for any optional output - because that is completely unambiguous, and there is nothing illogical about demanding that all optional outputs must be declared as such in the graph, whether or not there is a custom completion expression - do you disagree with that?

a => b
b?
# b completion = succeeded or failed

or:

a => b?
# b completion = succeeded or failed

I think this is just a small oversight of #6999 which implemented its logic in the graph parser (where there are no completion expressions) rather than the config (where there are).

(Agreed on that, if it was an oversight).

[oliver-sanders]

do you disagree with that?

Don't think I agree here.

there is nothing illogical about demanding that all optional outputs must be declared as such in the graph

I feel that this goes against the logic of #6999, either we do or do not infer optionality from the RHS.

To explain by reversing the statement, since optionality is not inferred from the RHS, we should have to explicitly ensure the output is marked as required on the LHS somewhere right?!

[oliver-sanders]

Here is my understanding / model.

Graph	Comp Expression (`:succeeded`)
R1 = """ # graph = None a => b # graph = Opt a => b b? # graph = Req a => b b """	# completion expr = None completion = # completion expr = Opt completion = succeeded or failed # completion expr = Req completion = succeeded

And the resulting truth table

#	Graph	Completion Expr	Validation
1	Opt	Opt	✔️ - Opt
2	Opt	None	✔️ - Opt
3	None	Opt	✔️ - Opt
4	Req	Req	✔️ - Req
5	Req	None	✔️ - Req
6	None	Req	✔️ - Req
7	None	None	✔️ - Req (default)
8	Opt	Req	❌
9	Req	Opt	❌

We are considering #3 in this table.

Note, the required status of :succeeded only applies to #7.

[hjoliver]

I don't think there's any logical contradiction with either approach; it's a matter of opinion.

The status quo: optionality is determined solely by the graph

• if it cannot be inferred from left sides it defaults to required
• and custom completions must conform to that

Or: optionality is determined by the graph or by completions

• if it cannot inferred from left sides, it defaults to required
• unless a custom completion makes it optional

To explain by reversing the statement, since optionality is not inferred from the RHS, we should have to explicitly ensure the output is marked as required on the LHS somewhere right?!

No, because we default to success required if we can't infer it from left sides. That was deliberate, for safety, and for brevity (because success-required is by fair the most common case).

[hjoliver]

I feel that this goes against the logic of #6999, either we do or do not infer optionality from the RHS.

Again, we made a distinction (quite clearly, in the proposal I think) between inferring optionality, and defaulting to success-required if it cannot be inferred from left sides.

The only question is, should the graph-based default count as determination, or should custom completions be able to override that.

[oliver-sanders]

The status quo: optionality is determined solely by the graph

This is not the status quo. For example:

R1 = a => b

[runtime]
  [[b]]
    completion  = succeeded or expired or submit_failed

In proposal: optional output extension, the source of truth for optionality changed from the graph to the completion expression. This is how we were able to make it so that :expired? did not infer succeeded?.

The optionality in the graph is used to derive the default completion expression. Otherwise, optionality is derived from the completion expression with validator logic to ensure that a manually specified expression is consistent with the graph.

Switching from internal logic to user expectations and looking at the truth table above, it would be rather inconsistent if #3 were to flip from Opt to Req. We never specified that it is required, but did explicitly state to the contrary.