common-workflow-language / cwl-v1.3

Apache License 2.0
4 stars 5 forks source link

[Proposal] Deprecating `CommandInputEnumSchema#inputBinding` #12

Open tom-tan opened 1 year ago

tom-tan commented 1 year ago

It is available since CWL v1.0 but its usecase can be covered with CommandInputParameter#inputBinding and CommandInputRecordField#inputBinding, I guess.

In addition to that, the current behavior of CommandInputEnumSchema#inputBinding with CommandInputParameter#inputBinding in cwltool seems to be unintended.

Here is an example.

enum-order-root-leaf.cwl:

class: CommandLineTool
cwlVersion: v1.2

baseCommand: python
inputs:
  - id: args.py
    type: File
    default:
      class: File
      location: args.py
    inputBinding:
      position: -1
  - id: inp1
    type:
      type: enum
      symbols: [a, b, c]
      inputBinding:
        position: 4
        prefix: -x
    inputBinding:
      position: 1
      prefix: -p
  - id: inp2
    type:
      type: enum
      symbols: [d, e, f]
      inputBinding:
        position: 3
        prefix: -z
    inputBinding:
      position: 2
      prefix: -q
outputs:
  - id: args
    type:
      type: array
      items: string

hints:
  - class: DockerRequirement
    dockerPull: docker.io/python:3-slim

enum-order-job.json:

{
    "inp1": "a",
    "inp2": "d"
}

I expect that it returns the following in which the value a and d occur only once:

{
    "args": [
        "-p",
        "-x",
        "a",
        "-q",
        "-z",
        "d"
    ]
}

However, here is the actual output of cwltool:

$ cwltool enum-order-root-leaf.cwl enum-order-job.json 
...
{
    "args": [
        "-p",
        "a",
        "-x",
        "a",
        "-q",
        "d",
        "-z",
        "d"
    ]
}

There are three options to fix it:

What do you think about it?

Related: common-workflow-language/common-workflow-language#783

tom-tan commented 1 year ago

Note: Once we have a consensus about the deprecation, we can leave some of the cases for common-workflow-language/common-workflow-language#783 as undocumented in CWL v1.2.1.

mr-c commented 1 year ago

@tom-tan I agree with your expected results; @tetron and I think that this is a cwltool bug; we should add conformance tests for this.

tom-tan commented 1 year ago

we should add conformance tests for this.

Does it mean that we take the second option? (i.e., "Fix cwltool for nested inputBinding" in the above description)

mr-c commented 1 year ago

we should add conformance tests for this.

Does it mean that we take the second option? (i.e., "Fix cwltool for nested inputBinding" in the above description)

Yes, the second option