-
Notifications
You must be signed in to change notification settings - Fork 303
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
Better support for optional arrays #25
Comments
Additionally, how would one provide a default value to an array? I'd like to do something like this:
|
Yeah, I've wanted to get away from using the key/value pairs inside of
However, the spec should be updated and Cromwell/wdl4s still need to support that. |
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. |
Cromwell doesn't support the default syntax I was looking to use. It returns I'll let you know what else I uncover as I continue to explore cwl2wdl conversion. Thanks! |
Yes, you are right, we took a shortcut in Cromwell and assumed that |
hey @scottfrazer I am facing the same issue now, when will the |
also is there a workaround for this case? |
We're targeting 0.19 (early april) this will be fixed. Until that happens, here's a work-around:
if the |
@scottfrazer was this issue addressed in 0.19? |
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 |
@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):
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:
Which results in the follow errors in the log:
|
@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. |
Hello to all in this thread!
|
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? |
@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 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 |
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. |
@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?
|
@cjllanwarne Did the syntax you proposed on Nov 3 make it into Cromwell 24? Specifically, enabling this bit:
|
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 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" }
} |
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 |
@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
If it's an optional array, I'm not sure that
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! |
I would like there to be support for the following type of scenario:
As far as I can tell there isn't a way to do this now.
The text was updated successfully, but these errors were encountered: