Alignment with Features Part 4. Fix execution unit.#591
Conversation
fmigneault
left a comment
There was a problem hiding this comment.
PUT edits are OK. Just the execution unit to review.
| @@ -39,8 +42,9 @@ because it can be inferred from the body structure itself. | |||
| ---- | |||
| { | |||
| "executionUnit": { | |||
| "value": { "cwlVersion": "v1.2", "class": "CommandLineTool", ... }, | |||
| "mediaType": "application/cwl" | |||
| "type": "cwl", | |||
| "mediaType": "application/cwl", | |||
| "value": { "cwlVersion": "v1.2", "class": "CommandLineTool", ... } | |||
| } | |||
| } | |||
| ---- | |||
| @@ -54,8 +58,9 @@ In these cases, the <<rc_cwl,CWL>> additional encoding (i.e.: JSON or YAML) need | |||
| ---- | |||
| { | |||
| "executionUnit": { | |||
| "value": "{ \"cwlVersion\": \"v1.2\", \"class\": \"CommandLineTool\", ... }", | |||
| "mediaType": "application/cwl+json" | |||
| "type": "cwl", | |||
| "mediaType": "application/cwl+json", | |||
| "value": "{ \"cwlVersion\": \"v1.2\", \"class\": \"CommandLineTool\", ... }" | |||
| } | |||
| } | |||
| ---- | |||
| @@ -65,8 +70,9 @@ In these cases, the <<rc_cwl,CWL>> additional encoding (i.e.: JSON or YAML) need | |||
| ---- | |||
| { | |||
| "executionUnit": { | |||
| "value": "cwlVersion: 'v1.2'\nclass: CommandLineTool\n...", | |||
| "mediaType": "application/cwl+yaml" | |||
| "type": "cwl", | |||
| "mediaType": "application/cwl+yaml", | |||
| "value": "cwlVersion: 'v1.2'\nclass: CommandLineTool\n..." | |||
| } | |||
There was a problem hiding this comment.
These definitions were already aligned in a similar form to qualifiedValue.yaml vs link.yaml, directly provided within the executionUnit object, using the usual value vs href fields.
If desired, the executionUnit.yaml (for the type/config/image properties) could be aligned to use the qualifiedValue.yaml form, but I believe the CWL examples are already adequately formatted without adding type or link nesting.
There was a problem hiding this comment.
I am confused!
Issue #397 contains a proposed schema that accommodates almost all the forms of CWL illustrated in clause 9 but does not rely on qualifiedValue.yaml (although it seems to be inspired by it). The proposed schema at the end of issue #397 does not include a mediaType property which the CWL examples seem to rely on so should I ...
- extend
qualifiedValue.yamlto define the schema of the execution unit OR - should I simplify the proposed schema in issue The execution unit schema is really confusing! #397 and add a
mediaTypeproperty OR - should I just simply the proposed schema with no
mediaTypeproperty (which would require that the CWL examples in clause 9 be updated to thetyperather thanmediaType)?
My preference is to simply the schema proposed in issue #397 to be this:
oneOf:
- $ref: link.yaml
- anyOf:
- description: |-
The execution unit is a DOCKER or OCI container.
type: object
properties:
type:
enum:
- docker
- oci
value:
$ref: executionUnitContainer.yaml
additionalProperties: false
- description: |-
The execution unit is a CWL script.
type: object
properties:
type:
enum:
- cwl
value:
$ref: "https://raw.githubusercontent.com/common-workflow-language/cwl-v1.2/main/json-schema/cwl.yaml"
additionalProperties: false
- description: |-
Other forms of execution unit (e.g. Python script, shell script) are allowed
by the Standard but are not described in the Standard.
type: object
properties:
type:
type: string
value:
type: object
additionalProperties: true
If I were to define the execution unit schema based on the qualifiedValue.yaml schema it might look like this:
oneOf:
- $ref: link.yaml
- allOf:
- $ref: qualifiedValue.yaml
- type: object
properties:
value:
anyOf:
- type: string
- type: object
- $ref: executionUnitContainer.yaml
- $ref: "https://raw.githubusercontent.com/common-workflow-language/cwl-v1.2/main/json-schema/cwl.yaml"
There is nothing explicit in this case telling you that the execution unit is a Docker container however that information could be determined by looking at the value object which would include the image property. If we wanted to make the Docker case explicit we could add an optional type member with the values "docker" or "oci". There is one other possibility too ... there is no official media type for a docker image. However, there are media types for the parts of a docker image. Specifically the media type application/vnd.oci.image.manifest.v1+json, which is the media type of the container image manifest, could be used as the value of the mediaType property to indicate that the execution unit is a container image.
@fmigneault, @gfenoy thoughts?
There was a problem hiding this comment.
I think we need a cherry-picked mix of the two variant with explicit types required (one way or another). One thing that is "risky" with plain type is that users could come up with their own variants that might not be reliable, when more explicit and reliable mediaType applies to cases like CWL. For example, mediaType: application/cwl+yaml can indicate more specifically the string contents than type: cwl. Indeed, the OGC/docker media-types are very tricky. An optional type seems reasonable for them.
Therefore, something like this?
oneOf:
# 'type' as plain string vs media-type is distinguished via the presence of 'href' or not
- $ref: link.yaml
- allOf:
# separate case for type/mediaType to enforce them to be there with any value
# actual value combinations defined by enum in the other 'allOf.anyOf' items
- anyOf:
- type: object
required: [type]
properties: {type: {type: string}}
- type: object
required: [mediaType]
properties: {mediaType: {type: string}}
- anyOf:
# equivalent to value/mediaType, but 'mediaType/type' enforced above
# this applies on top of the 'value' it already require itself
# no need anymore for the generic open-object at the end of the 1st example
- $ref: qualifiedValue.yaml
# ... rest of 1st example, with added mediaType propertiesThere was a problem hiding this comment.
@fmigneault I had to get Part 2 ready for RFC because OGC was waiting on me so I settled on the schema here. Take a look and let me know your thoughts... If you want to make changes, create a new issue and we can deal with is as an RFC comment.
There was a problem hiding this comment.
OK looks good. Similar result.
I notice the executionUnitBase.yaml contains required: type / mediaType. I think they must be arrays.
Closes #397