common-workflow-language / common-workflow-language

Repository for the CWL standards. Use https://cwl.discourse.group/ for support 😊
https://www.commonwl.org
Apache License 2.0
1.46k stars 197 forks source link

v1.1: Deprecate and make optional the declaration of many *Requirements #597

Open mr-c opened 6 years ago

mr-c commented 6 years ago

Indicate that all are still optional features, but that users no longer have to declare them. Add conformance tests for using those features without declaring them.

anton-khodak commented 6 years ago

Add conformance tests for using those features without declaring them.

but there should be an ability to mark tests as optional first

denis-yuen commented 6 years ago

What would be the user experience be if they wrote/received a workflow that used an optional feature that was not supported by a particular platform? Do they find out by validation or runtime?

inutano commented 6 years ago

Is there any strong reason to do this? I know it's bothering to write lots of Requirement lines every time, but I think these should be explicitly declared. For people, like me, who is implementing a very tiny engine which aims to support CWL, these implicit requirement declarations make it difficult to handle errors of unsupported features.

stain commented 6 years ago

Agree that having explicit requirements is better for engines and validation.

It also makes it clearer for engines and engine users which features are implementable, rather than an engine seemingly not supporting particular deep configurations.

This seems to me like a quick attempt of syntactic sugar to avoid boilerplate. That should be visited in V2 by separating CWL syntax from model.

On 25 Jan 2018 8:58 am, "Tazro Inutano Ohta" notifications@github.com wrote:

Is there any strong reason to do this? I know it's bothering to write lots of Requirement lines every time, but I think these should be explicitly declared. For people, like me, who is implementing a very tiny engine which aims to support CWL, these implicit requirement declarations make it difficult to handle errors of unsupported features.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/common-workflow-language/common-workflow-language/issues/597#issuecomment-360402474, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPd5b611OeIechbDZsx8o27n_w5_QrJks5tOEI_gaJpZM4RgOBr .

mr-c commented 6 years ago

@denis-yuen It is already the case that these features are not required for declaring something to be compliant with CWL v1.0.x. We don't currently have any language about when to inform a user that their workflow uses an unsupported feature. Obviously the sooner the better.

The use of these features can be detected by simple parsing. We will include non-normative psuedo-code for use by implementers in the spec.

Here a snapshot as of 2018-01-25 of CWL implementation feature completeness with links to the test reports:

The following workflow systems/runners/platforms implement the entire spec: CWL reference runner (cwltool) toil-cwl-runner rabix executor

The following systems implement all mandatory features but lack complete conformance test coverage: airflow-cwl-runner Many scatter related tests fail arvados Four InitialWorkDirRequirement tests fail and the dockerOutputDirectory test also fails

The following systems have yet to pass the core CWL conformance tests on ci.commonwl.org: Galaxy Cromwell xenon-cwl-runner AWE ( not yet tested on ci.commonwl.org ) yacle ( not yet tested on ci.commonwl.org ) REANA (passing almost all tests, but not merged; not yet tested on ci.commonwl.org) Apache Taverna ( not yet tested on ci.commonwl.org ) Consonance ( not yet tested on ci.commonwl.org ) IBM's CWL on LSF implementation ( not yet released, not yet tested on ci.commonwl.org )

It was always our intent to drop the required declaration of some of these features once it became apparent which would be universally supported.

denis-yuen commented 6 years ago

Thanks for the in-depth explanation (I also learned that toil was supposed to be fully compliant!) Not sure if this is the right place for ideas, but I was not aware that requirements like InlineJavascriptRequirement or SubworkflowFeatureRequirement were optional. I suspect that it might be easy for others to also believe that these were universal.

If it is the case that some requirements are optional, there should be a standardized way of determining that a CWL workflow is, say Arvados+cwltool+toil compatible while a different workflow is compatible with Airflow+cwltool+toil .

I don't know if the best place is something like a --validate option on the standard cwl-runner interface or something like a central https://validator.w3.org/ But it might be frustrating to be in a future where one engine might tell you well into runtime

tetron commented 6 years ago

We definitely want to have a linter that reports portability problems. Somebody should work on that.

inutano commented 6 years ago

Hmm. A linter may help engine developers, but I suppose remaining these Requirements is the better way to remind users/developers a use of optional features. I think users also should be able to be aware of the use of features easily, even when they run a very long CWL workflow written by another user.

Those Requirement fields doesn't reduce readability of CWL, so it seems to me it just brings a small benefit to users, but a tricky problem to developers.

tom-tan commented 6 years ago

I guess this change will introduce few gains but more disadvantages.

First of all, the requirements is a good feature to indicate that the given CWL uses some optional features.

If requirements field for optional features is deprecated, it will introduce the following problems.

As @stain said, this change will reduce some boilerplates when writing CWL. However, I am not sure there exist benefits than the disadvantages above.

mr-c commented 6 years ago

Thank you all for this great discussion. As a reminder, this is a proposed change for CWL v1.1, not CWL v1.0.x.

What if only the following two feature flags were made optional for v1.1?

mr-c commented 6 years ago

The point of making the declaration of these two feature optional is to be a signal that they will be required features to implement in a future release of the CWL standards.

tom-tan commented 6 years ago

It is good to make MultipleInputFeatureRequirement mandatory in the future release. It is common to use multiple inputs for a workflow and it does not introduce much complexity. It's worth to be mandatory.

I am not sure about SubworkflowFeatureRequirement. There exists many use cases for that but it will introduce an extra complexity to implement it. I guess it depends on the future direction of CWL, that is:

mr-c commented 6 years ago

Deferring for more discussion