Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Write a specification for unsatisfied task inputs. #359
Write a specification for unsatisfied task inputs. #359
Changes from 9 commits
10b055e
dcbfafa
ecd270d
6ba8da5
60fcf59
134f090
549eed6
2d19af2
583e0a8
9bd28ac
1e28d58
cd69299
53daafa
0e95a27
addd64f
cd672b7
653efbe
138b469
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Period should go outside the backtick, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this point. Does it contradict the previous one? I.e. the previous one seems to introduce an option, but this point seems to state it is not possible. Can you please clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first point is about the input json (or whatever other input format)
The outer workflow's call block is in the WDL code itself. When you call a workflow from inside a workflow. In that area there is currently no way to supply nested inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be made allowed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes maybe. But we should do that in another PR I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel it might be better to add some form of syntax in the WDL file itself. That way one would be able to see that there may be interesting nested inputs in the WDL files and, thus, that it would be good to use an engine which supports them.
In addition, a workflow might be written without the intention for the bubble-up behavior to be used. If someone then uses such a flag or configuration to allow for the bubble-up, they might be able to set optional inputs which will mess up the workflow.
An alternative to the last two points in the list:
etc
must be added to a workflow's input section to enable the bubble-up behavior described above. Only if thisetc
keyword is present in the workflow's input section may an engine allow for this bubble-up behavior and an engine may choose not to support to feature at all.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
etc
keyword makes sense to me. However, why not just allow it in the workflowmeta
section? This way, we don't need new syntax.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DavyCats good points! @orodeh good suggestion! Setting it in the workflow seems indeed better than setting it in the engine, and using the meta section for this seems the most logical thing to do.
Something like:
Except that I don't like the bubbled-up word. It is in the PR, but more on "a lack of a better term" basis.
How about nested inputs?
Or some other suggestions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that the meta section is the appropriate place for this. The meta section is just a set of key-value pairs that hold no relevance to the execution of a workflow. It is intended to be a place where you might store contact information or a link to the documentation. This was also discussed (briefly) in #351.
This feels more like something that would fit in the runtime section, except that there is no runtime section for workflows right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow-nested-inputs
seems fine to me, though I wonder if using-
s might cause issues with the grammars.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinging @patmagee for the grammar question.
Well we could consider adding a runtime section for workflows, but that requires another spec/language change. (Which is why I wanted to set it engine-side) . Adding a runtime section could be useful, but there need to be other relevant things stored there. Having an entire runtime section for
allow-nested-inputs
seems a bit ludicrous to me.I think the meta section is quite appropriate: It states that this workflow has nice nested inputs. A user does not have to use them. I think it is more documentation than runtime information, since an engine does not have to support nested-inputs per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are actually really great examples of something that can be added to the
hints
section, and we leave it up to the engine to decide what to do. We tried to really trim down the runtime lock and make it the bare minimum requirements needed to run a workflow.We already have the idea of "semi reserved" kw used in the hints section. These are hints that have similar meanings across all engines but do not need to be implemented.
I'd suggest doing something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patmagee, yes, that seems to be the best section but in the PR for the hints section I only see it at the
task
level. Will there also be a hints section at the the workflow level?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't feel like it quite matches the semantics of a "hint" to me to be honest.
A hint to me should mean "if an engine completely ignores this, it might miss out on some runtime optimization but the logical evaluation of the workflow should still be possible".
What we're talking about here is:
I'd almost like to add a way to flag
plugin
s ornon-standard-features
orengine-requirements
. I know Ohad has a number of other ideas for features which would be really cool but tricky for Cromwell. A way to flag these high up in the context of a WDL file (perhaps even above the workflow scope) would be really nice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eg something like:
it's not 100% ideal because it allows engines to diverge, but at least it'd be obvious right at the top of the file whether your workflow will work with any particular engine (which would presumably be able to publish or otherwise indicate which features they support). It would also allow engines to "get out quickly" if they see required features which they didn't have.