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

Ease optional access with scatter-like block #187

Closed
cjllanwarne opened this issue Jan 29, 2018 · 5 comments
Closed

Ease optional access with scatter-like block #187

cjllanwarne opened this issue Jan 29, 2018 · 5 comments

Comments

@cjllanwarne
Copy link
Contributor

cjllanwarne commented Jan 29, 2018

Port of Cromwell issue broadinstitute/cromwell#1852

Equivalent to scatter blocks for arrays, I propose a new ifDefined block for optionals:

workflow foo {
  String? bar
  ifDefined (concreteBar in bar) {
    call baz { input: x = concreteBar }
  }
  output {
    # Just like outputs from inside a scatter become array outputs, isDefined creates optional outputs:
    String? baz_out = baz.out
  }
}

task baz {
  # Non-optional input:
  String x
  # ...
}
@orodeh
Copy link
Contributor

orodeh commented Jan 30, 2018

I can see the issue, but I can't say I'm crazy about the syntax. Also, I assume the String in isDefined is superfluous.

ifDefined (concreteBar in bar)

@geoffjentry
Copy link
Member

@cjllanwarne Could you provide context on use cases and such? In general it'd be helpful for proposed language constructs to come attached with some concrete examples of "why"

@cjllanwarne
Copy link
Contributor Author

Sure!

The idea here is to provide syntactic sugar for the boilerplate in the following scenario:

workflow foo {
  [...]
  Int? x = ...
  if(defined(x)) {
    Int real_x = select_first([x])
    call bar { input: x = real_x }
  }
}

task bar {
  Int x
  [...]
}

This construction is becoming more and more common in WDLs as people start using if {} blocks more and getting more and more optional values, and at the same time, Cromwell 30 fixed a bug in WDL parsing and now enforces that optional values cannot be passed into tasks which expect a non-optional input.

@cjllanwarne
Copy link
Contributor Author

cjllanwarne commented Feb 1, 2018

In my opinion the Int real_x = select_first([x]) is an awkward and hard to read line that presents a barrier to entry for WDL readers. The more we can let authors write what they want without having to jump through awkward hoops, the better.

@patmagee
Copy link
Member

patmagee commented Mar 8, 2018

Non Deterministic workflows (although already supported) is one thing that I always struggle with, as it defeats the underlying purpose of reproducibility. I always wonder If this type of construct is occurring more and more, whether its something that we should encourage, or try to steer users away from.

That being said if this is something everyone wants, then I will entertain the idea.
When it comes to the specific proposed solution I am with @orodeh on this. I am not really a huge fan of the syntax. the in keyterm is more strongly associated with iterable types and it a bit confusing.

I would not add any additional constructs or execution blocks. I believe this can be solved by properly employing current constructs then adding a better select engine function.

The select function would take any optional element, and return the concrete version of the underlying element. However, if the optional value is not defined, then the select function would throw an error and that stage of the workflow would fail. Yes this introduces a point of failure if used improperly, however I think it is a reasonable burden on the user to ensure that they use the select function within an if block.

workflow foo {
  [...]
  Int? x = ...
  if(defined(x)) {
    call bar { input: x = select(x) }
  }
}

task bar {
  Int x
  [...]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants