search

stjude-rust-labs / sprocket

A bioinformatics workflow engine built on top of the Workflow Description Language (WDL).
https://sprocket.bio
Apache License 2.0
31 stars 1 forks source link

Incorrect type-mismatch error #21

Closed theAeon closed 1 month ago

theAeon commented 1 month ago

It would seem as of version 0.7.0, sprocket is reporting that a coercion from Array[File] to Array[String] is a type mismatch.

Cromwell's implementation of the 1.0 spec disagrees and to my understanding is correct to do so. Files can be coerced to strings, so it follows that arrays of files can be coerced to arrays of strings.

peterhuene commented 1 month ago

Hi @theAeon. Thank you very much for reporting this issue!

The type coercion part of the WDL specification contains a table of valid type coercions.

It defines a coercion from String to File, but not File to String. In fact, there is an example in that section (coercion_fail.wdl, albeit with some syntax errors) that shows it is expected to be an error to coerce a File to a String.

That said, I do agree that if there were a coercion from File to String, then Array[File] should coerce to Array[String] based on the coercion Array[X] -> Array[Y] (where X is coercible to Y).

However, I did just find a counterexample where it is expecting File to coerce as an argument to the sub function named change_extension_task.wdl, where it is expecting a coercion from File to String.

I believe this warrants an issue on the WDL spec for clarification and to explicitly call out a coercion from File to String. If the spec is corrected, then this will be very easy to fix in sprocket.

peterhuene commented 1 month ago

I'll file that issue with the spec and link it back to this issue.

peterhuene commented 1 month ago

I've filed https://github.com/openwdl/wdl/issues/685 to track the spec issue.

theAeon commented 1 month ago

I've filed openwdl/wdl#685 to track the spec issue.

Interesting, so the coercion is explicitly defined in spec 1.0.

peterhuene commented 1 month ago

@theAeon it appears that way! That makes me suspect it was an unintentional omission as to why it's not supported in 1.1 and 1.2; removing a coercion would certainly be a breaking change!

I'm going to preemptively fix this in sprocket so that we allow the coercion from 1.x versions, under the assumption it is an error in the spec.

peterhuene commented 1 month ago

I have a fix for this included with my "finish static analysis" work (i.e. fully analyze workflows). I had to include it there as otherwise we can't pass a check on our own workflows repo.