Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Workflow engine and version to RunWorkflow parameters #181

Closed
vsmalladi opened this issue Jul 11, 2022 · 11 comments · Fixed by #182
Closed

Add Workflow engine and version to RunWorkflow parameters #181

vsmalladi opened this issue Jul 11, 2022 · 11 comments · Fixed by #182

Comments

@vsmalladi
Copy link
Contributor

As workflow engines evolve there will be a difference between workflow language definition vs the version of the runner. I am suggesting we add two new required parameters to include in the WES spec to support running workflows that are actually supported by and tested by the developers of the workflow in question.

If a workflow is on Dockstore and has only been tested on Cromwell version 79, you should be able to run on Cromwell and can test with different Cromwell versions. WDL can be made with the new 1.1 spect, but Cromwell won't support that and thus WES with only Cromwell and no MiniWDL support wouldn't be able to run the 1.1 spec version.
With Nextflow there is a DSL-1/DSL-2 of the langauge that has different support for version of the Nextflow runner.

Overall I think this will provide the user more control on what runners and versions they can run their workflow so they can be confident that their workflow runs.

Suggesting to add these:

  The `workflow_runner` is the runner of workflow such as  "Cromwell" or "Nextflow" or "MiniWDL".
  The `workflow_runner_version` is the version of the workflow runner submitted and must be one supported by this WES instance.
@patmagee
Copy link
Contributor

@vsmalladi thanks for suggesting this! I agree, that as we see broader adoption of WES we will run into this issue more and more. Especially when we have multi-engine backends behind a singular WES entry point.

I have a few suggestions, so interested to hear what you think:

  • I wonder if instead of making these fields required we force the engines to broadcast the default engine and default engine version (possibly as the /service-info), then if the user does not define the engine/version to use in the run request we just use the default values. This reduces the burden on the user and simplifies the basic run request
  • Did you consider reusing the workflow_engine_params? I do not personally like that approach as much since at the moment that section is pretty much just a grab bag of random things to feed to the API, however we could come to an agreement on certain semantic meanings for a number of reserved keys .. ie workflow_runner and workflow_runner_version. If we did this though, I think it would only make sense to consider the same approach for the workflow_language
  • I would probably change the name to workflow_engine and workflow_engine_version, since the API already represents the engine/version in the service-info

@vsmalladi
Copy link
Contributor Author

@patmagee Here are some of my thoughts

  • I agree I think having a default that is broadcasted from /service-info makes perfect sense. This will also allow users to know the current supported engines
  • I looked into using 'workflow_engine_params' my worry is to overload this with everything. I think in this case its so important to be able to run workflows, just like language, that it makes sense to actually expose it as the API level. This will also allow us to say use 'CWL' with 'Cromwell' as the engine.
  • Agreed I think we should change it to workflow_engine and workflow_engine_version since they already are returned.

@uniqueg
Copy link
Contributor

uniqueg commented Aug 8, 2022

I agree with the need for this, and I also agree with defaults being broadcast so that, e.g., one can also pull an appropriate WES from a service registry ("give me a WES for engine X version Y"). Semantically and for consistency, I tend to not favor including these in the workflow_enginge_parameters (those are rather things that you would pass as CLI args to a given workflow engine of a given version).

Couple of thoughts here:

  • Some WES instances wrap multiple engines (e.g. WESkit, Sapporo WES), possibly even multiple versions of the same engine (I don't know any example but it's not unreasonable that sooner or later there would be one), or multiple different engines that could process the same workflow type/language (version); or people may want to make use of gateway WES instances that relay incoming requests to actual WES instances that can deal with them (e.g., proWES) - those ones would invariably "support" multiple engines/types
  • Some workflow types/languages are themselves not versioned (e.g., Snakemake)'
  • Should we have a controlled vocabulary for workflow engines? For workflow types, those are currently enumerated only in TRS, not in WES
  • A workflow will be written following a specific workflow type version, if available, but it may be executed by a range of workflow engine versions

I therefore think that

  • we should discuss this issue with the TRS community
  • we need to carefully consider what fields (or combinations of fields) are required (e.g., in Snakemake you would need the workflow type and a range of workflow engine versions, but not a workflow type version)
  • there may need to be nesting of workflow types/versions and corresponding engines/versions for broadcasting and requesting a WES's engines and versions, and we should assume that at least some of these could have multiple entries/ranges; it could get as complicated as, e.g., this (not a proper schema, but I hope it helps to illustrate the problem):
      {
          "workflow_type": {
      	    "CWL": {
      		    "1.2": {
      			    "toil": ["4.0"]
      		    },
      		    "1.1": {
      			    "toil": ["3.1", "4.0"],
      			    "cwltool": ["2.1"]
      		    }
      	    },
      	    "SMK": {
      		    "snakemake": ["6.10"]
      	    },
      	    "NFL": {
      		    "dsl-2": {
      			    "nextflow": ["3.2.1"]
      		    },
      		    "dsl-1": {
      			    "nextflow": ["2.2.1"]
      		    }
      	    }
          }
      }

note that there's no workflow type version for SMK

Tagging some more people: @denis-yuen @svedziok @vinjana

@vsmalladi
Copy link
Contributor Author

Shouldn't the engine version be able to error correctly based on the workflow_type version and repo that is provided?

So if you say wdl: 1.1 and cromwell 79 then here are the errors I see:

  1. WES API fails if wdl 1.1 and cromwell 79 are not supported
  2. If combination is not support workflow engine will pass that error back to WES client

Otherwise keeping this combination will be very onerous on the implementer

@svedziok
Copy link

svedziok commented Aug 9, 2022

I agree that support for multiple engine versions on the WES side is necessary for reproducibility. However, documenting all valid combinations of language and engine versions can become very complex for the WES implementer/provider. From the WES provider side, it would be much easier to support only specific engine versions (for nextflow, cromwell, cwltool, toil, etc.) and give this information back to the user. The engine should be able to handle the language version of a specific run and return an error if a workflow language version is not supported by the chosen engine.

@uniqueg
Copy link
Contributor

uniqueg commented Aug 9, 2022

My last point was motivated by the idea that it would be rather terrible user experience if if a client first needed to test WES instance by instance to see if there's a WES that supports running a given workflow, especially if it involves user interaction. But thinking about it, I agree with you @vsmalladi and @vsmalladi, returning an error is probably not too terrible, because it is arguably a fair requirement on users to check themselves if a given workflow engine (version) is capable of running a workflow of a specific type and version.

Perhaps it's helpful to play this through a bit and summarize the current state as well as the one proposed in #182:

  1. Presumably, in most cases, users just want their workflows to run. For that they'd need to know the supported workflow types and, if applicable, workflow type versions. For that, the existing schema in the service info, a composite of ServiceInfo and WorkflowTypeVersion suffices, although the property names are rather confusing:

    workflow_type_versions:
        CWL:
            workflow_type_version: ["1.0", "1.1", "1.2"]
        SMK:
            workflow_type_version: [""]

    Now a client knows that they can send most SMK and CWL workflows to the service, but not NFL workflows.

  2. Then, sometimes, a user/client would like a specific engine or engine version to be used. Any WES would only have a definite and limited number of workflow engines / workflow engine versions that they wrap. So this could be easily broadcast separately, as is already partly provided for in the current specs:

    workflow_engine_versions:
        toil: "3.1.8"
        cwltool: "2.0.20200126090152"
        snakemake: "6.15.5"

    Now a user knows that their brandnew Snakemake workflow that requires version >7.0 features wouldn't run on that particular WES. For CWL it becomes more complex in this example. I might know from workflow_type_versions that CWL 1.2 workflows are supported by this WES, but there are several engines for CWL. If I wanted to use cwltool to run a 1.2 workflow, I would be out of luck and receive an error (the listed cwltool version doesn't support 1.2) - which is acceptable if it's my responsibility to verify that the supported cwltool version is able to run my workflow. Similarly, if a user/client provided a nonsensical combination like snakemake for the workflow engine when the workflow type is CWL, they arguably deserve their error well.

    What the current specs do not provide for, is that a WES may support multiple versions of the same engine (because the additionalProperties of the workflow_engine_versions property are of type string, unlike in the WorkflowTypeVersion schema. feat: add in support for workflow engine and version when submitting a request. Closes #181 #182 proposes to rectify this by replacing the string type with a reference to an object WorkflowEngineVersion with a single property workflow_engine_version of type array, analogous to how workflow types and versions are broadcast:

    workflow_engine_versions:
        toil:
            workflow_engine_version: ["3.1.8", "5.7.1"]
        cwltool:
            workflow_engine_version: ["2.0.20200126090152"]
        snakemake:
            workflow_engine_version: ["6.15.5"]

So in terms of broadcasting workflow types and type versions, the proposed solution covers all the angles I can think of. But one thing might still be worth discussing:

  • Would it make sense to explicitly allow or forbid ranges/selectors of versions in workflow type versions and workflow engine versions? E.g., instead of enumerating individually all supported workflow type versions, an implementer could broadcast <=1.2 or >=1.0 <=1.2. Currently that would be legit (it's just a string), but clients couldn't possibly know how to interpret that. I would tend to think that unless we can agree on a syntax for ranges/selectors (and there doesn't seem to exist one commonly accepted standard, but npm, PyPI, Conda all do their own thing), that we should explicitly forbid using them.

@patmagee
Copy link
Contributor

patmagee commented Aug 9, 2022

This is a really great discussion and It looks like we ended up in pretty good place. FWIW, I also agree that trying to map the supported workflow types and version to every single engine would be very tedious for the engine implementer, and likely would be tedious for the user as well. To that end, I like the initial suggestion from @vsmalladi with the tweak you suggested @uniqueg to make it more inline with the workflow_type_versions .

In regards to the range selectors, I do not think that it really makes sense for the current use case. I doubt an implementer would allow for a dynamic range of engines as opposed to offering several discrete versions of a single engine. For example If you specified a range of >=1.0 <=1.2 would a user be able to select ANY version within that range?

@uniqueg
Copy link
Contributor

uniqueg commented Aug 9, 2022

I was thinking of ranges more for specifying supported workflow type versions (rather than engine versions).

The workflow types I am familiar with only have a couple of versions, but there may be some with frequent updates and where engines are backward compatible with respect to the workflow type versions they support. In such cases it would be tedious to enumerate every one of them.

However, it's the definition of feature creep to try to address problems that nobody ever ran into. If we ever get a complaint we can still reevaluate.

1 similar comment
@uniqueg
Copy link
Contributor

uniqueg commented Aug 9, 2022

I was thinking of ranges more for specifying supported workflow type versions (rather than engine versions).

The workflow types I am familiar with only have a couple of versions, but there may be some with frequent updates and where engines are backward compatible with respect to the workflow type versions they support. In such cases it would be tedious to enumerate every one of them.

However, it's the definition of feature creep to try to address problems that nobody ever ran into. If we ever get a complaint we can still reevaluate.

@vsmalladi
Copy link
Contributor Author

Ya i think it worth making a new issue for this range discussion. But this also goes into discussion about how many legacy versions of an engine are support and for how long.

@uniqueg
Copy link
Contributor

uniqueg commented Aug 10, 2022

I don't even think it's worth opening an issue. We can leave that up to the first person running in to that problem :)

patmagee pushed a commit that referenced this issue Mar 28, 2023
…a request. Closes #181 (#182)

* Add in support for workflow enginer and version when submitting a request.

* Expand out Workflow Engine version to by array of strings.

* Update RunRequest.yaml

* Add in workflow_engine and version to the run spec.

* Fix typo

* Fix for consitency

* Add more clarity about engine and version

* Update description

* Address changes requested.

* Fix indentation.

* Resolve reorganization of yaml's into 1 single yaml.

* Remove preempted with accidental merge.

* Add required parameter for:
- workflow_type
- workflow_type_version
- workflow_url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants