Reduce use of "if defined(argument)"

[multimeric]

Changes:

• Make use of string + null in WDL (closes if defined(argument) being used for optional scalar arguments #2)
• Pull in miniWDL for testing, and use it for testing the changes
• Remove large chunk of duplicated code

I'm unsure if I've got the string syntax 100% right given it has 3 nested levels of quotes, so another pair of eyes would be good. Also if you don't want the miniWDL dependency then feel free to just delete that part.

[illusional]

Hey @TMiguelT, thanks for putting this together. I'm very happy with the miniwdl stuff in there, I think it's a great addition.

I tried your approach to start, but it appears MiniWDL and Cromwell don't cascade the (string + null + string) the same.

~{'"--java" "' + java + '"'}

• MiniWDL: '' evaluates to a blank string
• Cromwell: '"' evaluates to a single quote char - causing task execution to fail.

I don't know if this is an error in Cromwell, or just the behaviour isn't defined as well in OpenWDL. What do you think the approach from here should be? I'm not a big fan of the way this task generation happens. I'm pretty happy to overhaul it to make it better. It's a hodepodge of legitimate template options and helpers. We know our criteria now, like the way arrays are handled is not as clean as it could be.

Example

Task:

version development
task quotetest {

  input {
    String? str
  }

  command <<<
      echo ~{'"--prefix" "' + str + '"'}
  >>>

  output {
    String out = read_string(stdout())
  }
}

MiniWDL:

miniwdl run quotetest.wdl 
# "outputs": {
#     "quotetest.out": ""
# }

miniwdl run quotetest.wdl str="Hello"
# "outputs": {
#     "quotetest.out": "--prefix Hello"
# },

Cromwell

java -jar cromwell-48.jar run quotetest.wdl 
# Job quotetest:NA:1 exited with return code -1
# STDERR: unexpected EOF while looking for matching '"'

java -jar cromwell-48.jar run quotetest.wdl -i quotestest-inp.json 
# "outputs": {
#  "quotetest.out": "--prefix Hello"
# }

[multimeric]

Based on the much more clear development spec, it seems that Cromwell is actually wrong here: https://github.com/openwdl/wdl/blob/master/versions/development/SPEC.md#interpolating-and-concatenating-optional-strings

Within interpolations, string concatenation with the + operator has special typing properties to facilitate formulation of command-line flags. [...] If either operand has an optional type, then the concatenation has type String?, and the runtime result is None if either operand is None

So, like miniWDL it should evaluate that entire chunk to None, which becomes an empty string after interpolation. I'll file that as a bug for cromwell (not that it will get any attention). In the short term, I guess this means either giving up the use of quotes around the argument (which might be dangerous), or giving up the simpler syntax, which I guess is the best optional the moment.

[illusional]

Yeah I'm split:

• This library should be about generating pure valid WDL and not really have to consider the engine
• I use this library and I need Cromwell support. Hence why I sort of am considering an API refresh.

I'm just not confident it'll get fixed in Cromwell.

[multimeric]

I think it's reasonable to want to produce a subset of valid WDL that works on both execution engines. It means messier code and also generated WDL but that's life.

[illusional]

I've added a PR to Cromwell to bring its behaviour in line with the OpenWDL spec:

• Optional string concatenation broadinstitute/cromwell#5464

When this is merged, I'm happy to action this card.

[illusional]

Hey @TMiguelT, I see there isn't action on the Cromwell side, I've restructured the v0.3.0 API (#5) which allows you to build the command input yourself. The existing commandinput helper functionality was preserved (but updated to use development features, notably the sep function and removing command interpolator options).

I share the following snippet of code which I use to build command inputs now (to handle all cases from scalar and quotable values, to optional arrays of optional quotable values (which is easier now).

https://github.com/PMCC-BioinformaticsCore/janis-core/blob/d531ad87ad190f0418f4fc906718ec5c5695e00c/janis_core/translations/wdl.py#L1148-L1248

Happy to answer any questions - for now I'd recommend you pin your projects to <0.3.0.