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 support for namespaced inputs again. #5317

Closed

Conversation

rhpvorderman
Copy link
Collaborator

Support for namespaced inputs was dropped from 48 as mentioned in BA-6149.

The code was reworked in #5214 which disallowed namespaced inputs. There was one line of code that actively prohibited namespaced inputs. Removing this line allows namespaced inputs again.

The namespaces are a bit different from the ones in 47, so I documented it in the changelog. Also I wrote some documentation on the inputs as I couldn't find any on the latest development documentation.

I like the newer namespaces much better than the old ones. A great job! This PR makes sure the fruit of this effort can be plucked by the pipeline developers.

If womtool inputs needs to give a cleaner output, then maybe we can change a few other things. Changing it in the inputs parsing is not desirable IMO because that also affects cromwell. In an ideal world all the namespaced inputs are available for advanced pipeline users in cromwell, while not cluttering the womtool inputs output (unless a flag is set).

@cjllanwarne cjllanwarne added the Community Contribution A pull request from the Cromwell open-source community label Dec 10, 2019
@cjllanwarne
Copy link
Contributor

The reason this exists in the first place is couple of comments about sub-workflow inputs in the WDL spec (https://github.com/openwdl/wdl/blob/master/versions/development/SPEC.md#computing-workflow-inputs).

I think if you can submit a short PR and get approval to remove that restriction from the new WDL spec version (and I suspect that it would be unanimously approved) then I'd be OK with "unofficially" pretending that those lines never existed in the WDL 1.0 spec either.

@geoffjentry
Copy link
Contributor

@cjllanwarne That would of course require the new WDL spec to be approved, which of course requires a number of things be implemented ..... ;)

@rhpvorderman
Copy link
Collaborator Author

@cjllanwarne Thanks for the clarification. I was already wondering why you would negate your own well-written namespace code with a single line... Anyway I created a pull request on the spec here: openwdl/wdl#347, your feedback would be much appreciated. Here's to hoping that it gets unanimously approved 🤞 .

@geoffjentry yes, the Cromwell team has a lot of influence on the spec by implementing or not implementing things. I can understand the temptation to use this for "the greater good" 😉 . But I am quite happy that the Cromwell developers chose to be in touch with the community and aggressively implement the development spec in the development version of Cromwell. This allows us to see how certain spec changes turn out before they get implemented in production. In this case I came across this when I was testing the code for #5312 and found that I could not set my resource requirements for BWA anymore (in BioWDL all tasks default to the least number of cores needed, and sometimes you want to override this for more power). Since BWA was nested in a subworkflow this turned out not to be possible. So now we can fix the spec and Cromwell before this ever gets into a release. I think it is great work by the Cromwell team. It can't always be easy to follow the spec that closely.

@cjllanwarne
Copy link
Contributor

@geoffjentry well, Christmas is coming... 😄

@rhpvorderman
Copy link
Collaborator Author

I will close this PR for now. Discussion about how to implement this in the spec is ongoing in openwdl/wdl#359. After a resolution is accepted, I can take it upon myself to add this to Cromwell. @cjllanwarne architected the code in such a way that it is really easy to add support for namespaced inputs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A pull request from the Cromwell open-source community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants