Better support for optional arrays

[adamstruck]

I would like there to be support for the following type of scenario:

task foo {
    Array[String] bar
    Array[String]? baz

   command {
       something.py --input ${sep=' ' bar} ${"--optionalInput" + ${sep=' ' baz}}
   }

   ...

As far as I can tell there isn't a way to do this now.

[adamstruck]

Additionally, how would one provide a default value to an array?

I'd like to do something like this:

task echo {
  Array[String]?

  command {
    echo ${default=["foo", "bar"] sep=" " phrase}`
  }

[scottfrazer]

Yeah, I've wanted to get away from using the key/value pairs inside of ${...} at some point. However since we support them already and will support them for a while, I think the correct syntax to for your first example should be

task foo {
  Array[String] bar
  Array[String]? baz

  command {
    something.py --input ${sep=' ' bar} ${prefix="--optionalInput" sep=' ' baz}
  }
}

However, the spec should be updated and Cromwell/wdl4s still need to support that.

[scottfrazer]

Also the "default" syntax you used should be supported, but it doesn't surprise me too much if it's not working in Cromwell (I bet nobody has even tried it!). I'll bundle them into one ticket.

[adamstruck]

Cromwell doesn't support the default syntax I was looking to use. It returns wdl4s.parser.WdlParser$Ast cannot be cast to wdl4s.parser.WdlParser$Terminal.

I'll let you know what else I uncover as I continue to explore cwl2wdl conversion. Thanks!

[scottfrazer]

Yes, you are right, we took a shortcut in Cromwell and assumed that default is going to be a string. Definitely a bug.

[philloooo]

hey @scottfrazer I am facing the same issue now, when will the prefix keyword be supported?

[philloooo]

also is there a workaround for this case?

[scottfrazer]

We're targeting 0.19 (early april) this will be fixed. Until that happens, here's a work-around:

task arr {
  Array[String]? foo

  command {
    if [ -n '${sep=',' foo}' ]; then
      FLAG=--prefix=${sep=',' foo}
    else
      FLAG=''
    fi

    echo $FLAG
  }

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

workflow w {
  call arr
}

if the foo array is non-empty, then FLAG will be --prefix=val1,val2,val3 otherwise FLAG will be an empty string

[adamstruck]

@scottfrazer was this issue addressed in 0.19?

[geoffjentry]

Hi @adamstruck - I can't find any evidence that it has, although I also don't see a ticket for it either. Just FYI @scottfrazer recently left our group, although perhaps he's watching this and can chime in one way or the other.

@ruchim - Can you refine this into a ticket, or perhaps your ticket search-fu is better than mine and you can find an already existing one? Also I think it'd be good to note in the ticket that this might already have been done and hiding in plain site or perhaps in that wdl4s branch of Scott's

[adamstruck]

@geoffjentry do you have an update on this?

Additionally, I noticed that your GATK wrappers in this repo are using unsupported syntax that stems from this issue. Here are two examples (there are many more):

• https://github.com/broadinstitute/wdl/blob/develop/scripts/wrappers/gatk/WDLTasks_3.6/CountIntervals_3.6.wdl#L23
• https://github.com/broadinstitute/wdl/blob/develop/scripts/wrappers/gatk/WDLTasks_3.6/VariantRecalibrator_3.6.wdl#L52

The syntax above works only if no args are passed to that input. If the user defines the array in question, cromwell fails with an expression evaluation error (which makes sense since 'sep' is not defined for the array.

Here is an example:

task test {
  Array[String]? args

  command {
    ls \
    ${default="" "-" + args}
  }

  output {
    File foo = stdout()
  }

  runtime {
    docker: 'ubuntu:14.04'
  }
}

workflow run {
  call test
}

Which results in the follow errors in the log:

[2016-11-02 09:56:49,15] [error] SharedFileSystemAsyncJobExecutionActor [36273b38run.test:NA:1]: Error attempting to Execute the script
java.lang.UnsupportedOperationException: Could not evaluate expression: "-" + args
    at wdl4s.command.ParameterCommandPart.instantiate(ParameterCommandPart.scala:45)
    at wdl4s.Task$$anonfun$instantiateCommand$1$$anonfun$apply$6.apply(Task.scala:237)
    at wdl4s.Task$$anonfun$instantiateCommand$1$$anonfun$apply$6.apply(Task.scala:237)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
    at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
    at scala.collection.Iterator$class.foreach(Iterator.scala:893)
    at scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
    at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
    at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
    at scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
    at scala.collection.AbstractTraversable.map(Traversable.scala:104)
    at wdl4s.Task$$anonfun$instantiateCommand$1.apply(Task.scala:237)
    at wdl4s.Task$$anonfun$instantiateCommand$1.apply(Task.scala:237)
    at scala.util.Try$.apply(Try.scala:192)
    at wdl4s.Task.instantiateCommand(Task.scala:237)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$$anonfun$4.apply(SharedFileSystemAsyncJobExecutionActor.scala:171)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$$anonfun$4.apply(SharedFileSystemAsyncJobExecutionActor.scala:170)
    at scala.util.Success.flatMap(Try.scala:231)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$class.instantiatedScript(SharedFileSystemAsyncJobExecutionActor.scala:170)
    at cromwell.backend.impl.sfs.config.BackgroundConfigAsyncJobExecutionActor.instantiatedScript(ConfigAsyncJobExecutionActor.scala:110)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$class.executeScript(SharedFileSystemAsyncJobExecutionActor.scala:205)
    at cromwell.backend.impl.sfs.config.BackgroundConfigAsyncJobExecutionActor.executeScript(ConfigAsyncJobExecutionActor.scala:110)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$$anonfun$executeOrRecover$2.apply(SharedFileSystemAsyncJobExecutionActor.scala:182)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$$anonfun$executeOrRecover$2.apply(SharedFileSystemAsyncJobExecutionActor.scala:179)
    at scala.util.Try$.apply(Try.scala:192)
    at cromwell.backend.sfs.SharedFileSystemAsyncJobExecutionActor$class.executeOrRecover(SharedFileSystemAsyncJobExecutionActor.scala:178)
    at cromwell.backend.impl.sfs.config.BackgroundConfigAsyncJobExecutionActor.executeOrRecover(ConfigAsyncJobExecutionActor.scala:110)
    at cromwell.backend.async.AsyncBackendJobExecutionActor$$anonfun$cromwell$backend$async$AsyncBackendJobExecutionActor$$robustExecuteOrRecover$1.apply(AsyncBackendJobExecutionActor.scala:45)
    at cromwell.backend.async.AsyncBackendJobExecutionActor$$anonfun$cromwell$backend$async$AsyncBackendJobExecutionActor$$robustExecuteOrRecover$1.apply(AsyncBackendJobExecutionActor.scala:45)
    at cromwell.core.retry.Retry$.withRetry(Retry.scala:37)
    at cromwell.core.retry.Retry$$anonfun$withRetry$2$$anonfun$applyOrElse$1.apply(Retry.scala:41)
    at cromwell.core.retry.Retry$$anonfun$withRetry$2$$anonfun$applyOrElse$1.apply(Retry.scala:41)
    at akka.pattern.FutureTimeoutSupport$$anonfun$after$1.liftedTree1$1(FutureTimeoutSupport.scala:25)
    at akka.pattern.FutureTimeoutSupport$$anonfun$after$1.apply$mcV$sp(FutureTimeoutSupport.scala:25)
    at akka.actor.Scheduler$$anon$4.run(Scheduler.scala:126)
    at akka.dispatch.TaskInvocation.run(AbstractDispatcher.scala:39)
    at akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(AbstractDispatcher.scala:409)
    at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
    at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
    at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
    at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)
Caused by: wdl4s.WdlExpressionException: Cannot perform operation: - + ["l", "t"]
    at wdl4s.values.WdlValue$class.invalid(WdlValue.scala:12)
    at wdl4s.values.WdlString.invalid(WdlString.scala:7)
    at wdl4s.values.WdlString.add(WdlString.scala:15)
    at wdl4s.expression.ValueEvaluator$$anonfun$evaluate$2$$anonfun$apply$1.apply(ValueEvaluator.scala:39)
    at wdl4s.expression.ValueEvaluator$$anonfun$evaluate$2$$anonfun$apply$1.apply(ValueEvaluator.scala:39)
    at scala.util.Success$$anonfun$map$1.apply(Try.scala:237)
    at scala.util.Try$.apply(Try.scala:192)
    at scala.util.Success.map(Try.scala:237)
    at wdl4s.expression.ValueEvaluator$$anonfun$evaluate$2.apply(ValueEvaluator.scala:39)
    at wdl4s.expression.ValueEvaluator$$anonfun$evaluate$2.apply(ValueEvaluator.scala:39)
    at scala.util.Success.flatMap(Try.scala:231)
    at wdl4s.expression.ValueEvaluator.evaluate(ValueEvaluator.scala:39)
    at wdl4s.WdlExpression$.evaluate(WdlExpression.scala:79)
    at wdl4s.WdlExpression.evaluate(WdlExpression.scala:192)
    at wdl4s.command.ParameterCommandPart.instantiate(ParameterCommandPart.scala:36)
    ... 40 common frames omitted

[vdauwera]

@knoblett FYI there is a Cromwell bug that breaks your GATK wrappers. May be worth adding a note in the wrappers README to warn people that this is currently a breaking point in versions of Cromwell up to current.

[cjllanwarne]

Hello to all in this thread!

• We're sorting out and making-sane the optional types in an upcoming release, probably 24 (around Christmas, but don't quote me on that!)
• I've already got a PR in the works in wdl4s towards this, making the syntax for optionals more general-purpose and standard - see broadinstitute/wdl4s#43
• Although I personally don't like the format, I don't see any harm in continuing with String defaults in the ${}.
• However, I think we should say that the canonical way to specify defaults should be as part of the declaration.
  • E.g we should prefer this:

task foo {
  Array[Int]? bar = [1, 2, 3]
  command {
    ./baz.sh ${sep=" " bar}
  }
}

• To this:

task foo {
  Array[Int]? bar
  command {
    ./baz.sh ${default=[1, 2, 3] sep=" " bar}
  }
}

[vdauwera]

OMG yes, having the default values in the declaration rather than the command is so much better. Is that already an accepted idiom or are you saying you're going to make changes to that effect, @cjllanwarne?

[cjllanwarne]

@vdauwera it's merely my personal belief that it's a better way to move forwards :)

FWIW the in-declaration method might already work for Array[X] defaults. I haven't checked that yet though.

As a side-note, since the in-string version is currently broken for anything other than String (which we could 'deprecate' but continue to allow), technically this isn't a loss of existing functionality... Although it probably would be a documentation change

[vdauwera]

Ah okay, well then I support your personal belief on this one.

Sounds like this should be in @katevoss 's pile of things for now then.

[adamstruck]

@cjllanwarne thanks for the update.

I am fine with deprecating the in-string version of default - I prefer to use the in-declaration method. However, there are additional issues with defaults that I have found noted elsewhere (broadinstitute/cromwell#1632).

Can you clarify what syntax you are supporting going forward with regards to using prefixes with optional arrays?

• ${prefix="--foo " sep="," optional_array}
• ${sep="," "--foo " + optional_array}
• some other syntax?

[vdauwera]

@cjllanwarne Did the syntax you proposed on Nov 3 make it into Cromwell 24?

Specifically, enabling this bit:

task foo {
  Array[Int]? bar = [1, 2, 3]
  command {
    ./baz.sh ${sep=" " bar}
  }
}

[kshakir]

Currently available testing and feedback in the cromwell develop branch, and scheduled to be release with cromwell 25, are better support for empty arrays, optionals, and a new function prefix(String, Array[X]).

Example:

task pfx {
  Array[String] kvs = ["k1=v1", "k2=v2", "k3=v3"]
  Array[String] prefixed = prefix("-e ", kvs)
  command {
    echo "${sep=' ' prefixed}"
  }
  output {
    String out = read_string(stdout())
  }
  runtime { docker: "ubuntu:latest" }
}

[geetduggal]

Thanks for your work on this! I ran into a similar issue when converting optional arrays in CWL to WDL and previously ended up modifying the converter to generate the appropriate string in bash by prepending to the command. The prefix is a welcome addition to address this issue. Do you all still plan on implementing the syntax @adamstruck hinted at a couple of comments earlier? Thanks in advance for your thoughts.

[cjllanwarne]

@geetduggal I'm going to close this ticket after this answer as I believe the original issue to be fixed now. If you have a follow up please check out the WDL forums.

If you have an array, there is now a prefix function that can prepend values to the start of it:

  Array[String] without_prefixes = ["a", "b"]
  Array[String] with_prefix = prefix("X", without_prefixes) # i.e. ["Xa", "Xb"]
  command { ${sep=" " with_prefix} }

If it's an optional array, I'm not sure that prefix will work, so we might need to get creative:

  Array[String]? without_prefixes
  String initial_pre = if defined(without_prefixes) then "--pre " else ""
  command { ${initial_pre}${sep=" --pre " without_prefixes} }

I hope that helps. If not, please do feel free to follow up on the WDL forums I mentioned above, they're a great resource!