Workflow output should be optional when referencing the output of a conditional step

[jdidion]

In https://github.com/common-workflow-language/cwl-v1.2/blob/1.2.1_proposed/tests/conditionals/cond-wf-002.cwl and https://github.com/common-workflow-language/cwl-v1.2/blob/1.2.1_proposed/tests/conditionals/cond-wf-002_nojs.cwl, the workflow output parameter out1 references step1/out1. step1 has a when, which means its outputs will be null when the condition is false. Thus, the workflow output parameter should be optional.

[mr-c]

This conformance test has the note "Should give validation warning because of required sink"

cwl-v1.2/tests/conditionals/test-index.yaml

Line 26 in c0c31c5

doc: Should give validation warning because of required sink

[jdidion]

Does that mean the workflow output type should be automatically converted to optional?

[mr-c]

Not really; this workflow will fail if the workflow inputval is not greater than 2. Converting the output to optional would cover that up.

[jdidion]

Ok so it's supposed to be a runtime failure rather than a validation error. Is that covered anywhere in the spec?

[jdidion]

In these two conformance tests, the value being passed in is 1, which means the conditional does not execute. Yet the tests are not expected to fail. So I still don't understand why these are valid tests.

[mr-c]

I see your point @jdidion ; @tetron is this a bug in cwltool, why is a workflow that produces null for a required output not failing?

$ cwltool --debug tests/conditionals/cond-wf-002.cwl tests/conditionals/val.1.job.yaml 
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220121153928
INFO Resolved 'tests/conditionals/cond-wf-002.cwl' to 'file:///home/michael/cwl-v1.2/tests/conditionals/cond-wf-002.cwl'
WARNING Workflow checker warning:
tests/conditionals/cond-wf-002.cwl:14:11: Source 'out1' of type ["null", "string"] may be
                                          incompatible
tests/conditionals/cond-wf-002.cwl:18:5:    with sink 'out1' of type "string"
                                            Source is from conditional step and may produce `null`
tests/conditionals/cond-wf-002.cwl:14:11: Source 'out1' of type ["null", "string"] may be
                                          incompatible
tests/conditionals/cond-wf-002.cwl:18:5:    with sink 'out1' of type "string"
DEBUG [workflow ] initialized from file:///home/michael/cwl-v1.2/tests/conditionals/cond-wf-002.cwl
INFO [workflow ] start
DEBUG [workflow ] inputs {
    "val": 1
}
INFO [workflow ] starting step step1
DEBUG [step step1] job input {
    "file:///home/michael/cwl-v1.2/tests/conditionals/cond-wf-002.cwl#step1/a_new_var": 1,
    "file:///home/michael/cwl-v1.2/tests/conditionals/cond-wf-002.cwl#step1/in1": 1
}
DEBUG [step step1] conditional $(inputs.a_new_var > 2) evaluated to False
DEBUG [step step1] inputs was {
  "a_new_var": 1,
  "in1": 1
}
INFO [step step1] will be skipped
DEBUG [step step1] produced output {
    "file:///home/michael/cwl-v1.2/tests/conditionals/cond-wf-002.cwl#step1/out1": null
}
INFO [step step1] completed skipped
INFO [workflow ] completed success
DEBUG [workflow ] outputs {
    "out1": null
}
DEBUG Removing intermediate output directory /tmp/sa9h9mf1
{
    "out1": null
}
INFO Final process status is success

[mr-c]

@jdidion Well, this may be valid after all. According to the CWL v1.2 Workflow standard:

https://www.commonwl.org/v1.2/Workflow.html#Workflow_success_and_failure

If all workflow steps are executed and complete with success, then the workflow status is success.

Therefore, since the step was successfully skipped, and there were no other steps, then the overall workflow is a success even if the results don't match the output types.

Here's a simpler reproducer, without the conditional [edited to show that this is still true in CWL v1.0]

#!/usr/bin/env cwl-runner
cwlVersion: v1.0
class: Workflow

inputs: []

steps:
  step1:
    in: {}
    run:
      class: ExpressionTool
      inputs: []
      requirements:
        InlineJavascriptRequirement: {}
      expression: |
        $({"result": null})
      outputs:
        result: string?
    out: [ result ] 

outputs:
  required:
    type: string
    outputSource: step1/result

$ cwltool missing_reqd_output.cwl
INFO /home/michael/cwltool/env3.10/bin/cwltool 3.1.20220121153928
INFO Resolved 'missing_reqd_output.cwl' to 'file:///home/michael/cwl-v1.2/missing_reqd_output.cwl'
WARNING Workflow checker warning:
missing_reqd_output.cwl:19:12: Source 'result' of type ["null", "string"] may be incompatible
missing_reqd_output.cwl:23:5:    with sink 'required' of type "string"
INFO [workflow ] start
INFO [workflow ] starting step step1
INFO [step step1] start
INFO [step step1] completed success
INFO [workflow ] completed success
{
    "required": null
}
INFO Final process status is success

[jdidion]

So is it a special case to allow a non-optional workflow output to be optional when it references a conditional step? What if I say that the workflow output type is int but it references a string output of a conditional workflow step? Would that be allowed so long as the conditional step is not executed?

[mr-c]

I think there are two aspects here: workflow validation up-front, and reporting workflow success.

For workflow validation upfront, the types of a workflow output must overlap the types of the step output it connects to. Ideally the author would make the type lists match the exactly, but CWL v1.0 - v1.2 does not require that (and there are no plans at this time to change that in a future version of the CWL standards).

For reporting workflow success, according to the CWL v1.2 Workflow standard, (sub-)workflow success is defined by the success of all the steps, not if the output types match:

https://www.commonwl.org/v1.2/Workflow.html#Workflow_success_and_failure

If all workflow steps are executed and complete with success, then the workflow status is success.

Returning to your questions:

So is it a special case to allow a non-optional workflow output to be optional when it references a conditional step?

Workflow outputs types only matter when checking up front validation: if there is overlap between a workflow output type and the outputSource type, as modulated by any condtional, pickValue (if present), and linkMerge (if there are multiple sources). For example: a workflow output oftype: string and who's outputSource is a single reference to a result of type: string from a conditional step; and there is no pickValue specified. Therefore the computed type of the outputSource is type: [ string, 'null' ]; this overlaps with the workflow output type: string and the connection validates.

At runtime, type matching is not checked in CWL v1.2 for workflow outputs: if all steps are successful then the workflow itself will be a success even if that particular workflow output received a null from the conditional step.

We can have the same story without conditional steps or optional outputs: the key is overlapping type definitions:

#!/usr/bin/env cwl-runner
cwlVersion: v1.0
class: Workflow

inputs: []

steps:
  step1:
    in: {}
    run:
      class: ExpressionTool
      inputs: []
      requirements:
        InlineJavascriptRequirement: {}
      expression: |
        $({"result": 42})
      outputs:
        result: [ string, int ]
    out: [ result ] 

outputs:
  workflow_result:
    type: string
    outputSource: step1/result

INFO [step step1] completed success
INFO [workflow ] completed success
{
    "required": 42
}
INFO Final process status is success

What if I say that the workflow output type is int but it references a string output of a conditional workflow step? Would that be allowed so long as the conditional step is not executed?

That would not be allowed by the validation rules. The types have to overlap, and there is no overlap between type: int and the computed type of the conditional step: type: string?.

ERROR Tool definition failed validation:

146b.cwl:23:12: Source 'result' of type ["null", "string"] is incompatible
146b.cwl:27:5:     with sink 'required' of type "int"

[tetron]

@jdidion I think you have a valid complaint here, it ought to enforce (at runtime) that a workflow output matches the outputs definition. Michael is right that the text of the spec doesn't say anything about it, but I think that's an oversight.

Separately, the cwltool static checker treats a source ["null", "string"] going to sink ["string"] as a warning and not an error to accommodate situations where the output of a step is always string in practice (e.g. it is a tool option, which in a particular workflow, the option is always enabled). In those situations, the checker doesn't have enough information and shouldn't get in the way.

[mr-c]

Thanks @tetron .

For CWL v1.2.1 we could try to adjust the conformance tests so that the output type matches the final outputs (and still achieves the goals of the conformance tests). This would allow some implementations (including cwltool, and perhaps DNAnexus) to be more strict without requiring all implementations to do so.

And then in CWL v1.3+: declare that workflow outputs not matching the declared type result in a permanentFailure and add mandatory should_fail: true conformance tests to enforce that

[tetron]

I agree that the conformance tests should be fixed so that the outputs section matches the actual shape of the output.

[jdidion]

Great! I will open a PR.