cromwell icon indicating copy to clipboard operation
cromwell copied to clipboard

womtool validate handles optional arrays inconsistently, leading to technically-impossible runtime errors

Open aofarrel opened this issue 1 year ago • 1 comments

basic issue

womtool validate does not catch all scenarios where you are defining a new variable based on an optional variable. Instead, Cromwell fails at runtime -- even if it is actually impossible for that optional variable to be undefined.

A working example is available, but it is a complex WDL, so a more basic example is listed below.

background

WDL doesn't really have a proper understanding of mutual exclusivity, so it doesn't realize that anything under a "is optional variable X defined?" block can only happen if optional variable X is defined. In other words, if variant_caller.errorcode has type Array[String?], the following code block is invalid, and womtool correctly flags it as such:

if(defined(variant_caller.errorcode)) { 
	Array[String] not_optional_error_code = variant_caller.errorcode
}

Failed to process declaration 'Array[String] varcall_error_if_earlyQC_filtered = variant_call_after_earlyQC_filtering.errorcode' (reason 1 of 1): Cannot coerce expression of type 'Array[String?]' to 'Array[String]'

The normal workaround for this is to use select_first() with a bogus fallback value, since the defined check means that fallback value will never be selected.

if(defined(variant_caller.errorcode)) { 
	Array[String] not_optional_error_code = select_first([variant_caller.errorcode, ["according to all known laws of aviation"]])
}

The same holds true if I only care about the first (index 0) variable in the array. That's the case for me, since the actual workflow I'm working on will be run on Terra data tables, eg each instance of the workflow only gets one sample but dozens of instances of the workflow will be created. For compatibility reasons I cannot convert the variant caller into a non-scattered task, so its error code will still have type Array[String]? even though that array will only have one value.

if(defined(variant_caller.errorcode)) { 
	String not_optional_error_code = select_first([variant_caller.errorcode[0], "according to all known laws of aviation"])
}

the womtool bug

I only care about variant_caller.errorcode[0] if it does not equal the word "PASS", so I wrote this:

String pass = "PASS"
if(defined(variant_caller.errorcode)) {
	if(!variant_caller.errorcode[0] == pass)) {
		String not_optional_error_code = select_first([variant_caller.errorcode[0], "according to all known laws of aviation"])
		}
	}

One could argue that this is technically correct, since the equality check only runs if the variant_caller.errorcode is defined. And indeed, womtool validate does not see any issue with this. However, at runtime, I get this error:

Failed to evaluate 'if_condition' (reason 1 of 1): Evaluating !((variant_call_after_earlyQC_filtering.errorcode[0] == pass)) failed: Sorry! Operation == is not supported on empty optional values. You might resolve this using select_first([optional, default]) to guarantee that you have a filled value.

I get this error whether or not the variant caller task actually ran, even though whether or not it ran should cause an issue, since it's under a defined() check. If the defined() check still is not enough like is the case for setting not_optional_error_code, then that should be caught before runtime.

backends effected

The womtool validation bug affects at least Terra-womtool and local-womtool. Runtime error happened on Terra-Cromwell but would probably happen on every backend.

aofarrel avatar Aug 04 '23 23:08 aofarrel

After further testing, I now think the actual issue is the defined() built-in function does not match the spec. I'm inclined to think this is a bug, because if Cromwell is intended to deviate from the spec, then womtool should pick up on it.

https://github.com/broadinstitute/cromwell/issues/7201

aofarrel avatar Aug 09 '23 20:08 aofarrel