fix: resolve type mismatch errors across WDL files - #1816
Conversation
Placeholders in WDL cannot contain more than one option (e.g., `default` + `sep` or `true`/`false` paired with a second `default` + `sep` placeholder). This rewrites those expressions using `prefix()`, `sep()`, and `select_first()` to satisfy `sprocket lint` while preserving the original command-line output.
Placeholder options like `default` expect string values. Changes `default=0` to `default="0"` and `default=250` to `default="250"` across two task files.
…n 1.0
Adds `version 1.0` declarations, wraps inputs in `input {}` blocks,
and changes `cpu` runtime values from strings to bare `Int` variables
across seven task/workflow files.
claymcleod
left a comment
There was a problem hiding this comment.
Inline comments explaining each fix for reviewer context.
| vcf_intervals_idx = training_vcf_so_bgz_idx, | ||
| intersecting_intervals=intersecting_intervals, | ||
| id = i | ||
| id = "~{i}" |
There was a problem hiding this comment.
range() returns Array[Int], but the sitesOnlyAndHQFilterVcf task declares String id. WDL doesn't coerce Int to String implicitly, so we use string interpolation to convert.
| output_name="full_data_sites_filtered", | ||
| service_account_json=service_account_json, | ||
| id = j | ||
| id = "~{j}" |
There was a problem hiding this comment.
Same Int to String fix as above — j comes from range() but filter_by_sites_only expects String id.
| String output_filename_idx = output_filename + ".tbi" | ||
| String has_service_account_file = if (defined(service_account_json)) then 'true' else 'false' | ||
| String service_account_basename_pre = if (defined(service_account_json)) then service_account_json else '' | ||
| String service_account_basename_pre = select_first([service_account_json, '']) |
There was a problem hiding this comment.
The pattern if defined(x) then x else y doesn't narrow x from String? to String in the then branch per the WDL spec, so Sprocket flags the result as String? being assigned to String. select_first([x, y]) does the same thing and returns the correct non-optional type.
| String output_filename_idx = output_filename + ".tbi" | ||
| String has_service_account_file = if (defined(service_account_json)) then 'true' else 'false' | ||
| String service_account_basename_pre = if (defined(service_account_json)) then service_account_json else '' | ||
| String service_account_basename_pre = select_first([service_account_json, '']) |
There was a problem hiding this comment.
Same select_first fix as in sitesOnlyAndHQFilterVcf above — identical pattern in the filter_by_sites_only task.
| output { | ||
| Array[String] reference_chunk_file_paths = read_lines("reference_shard_file_paths.tsv") | ||
| Array[Int] mem_gb_per_chunk = read_lines("memory_per_chunk.tsv") | ||
| Array[String] mem_gb_per_chunk = read_lines("memory_per_chunk.tsv") |
There was a problem hiding this comment.
read_lines() always returns Array[String] per the WDL spec. This output isn't used anywhere in the workflow currently, so the type change is safe. If it's consumed downstream in the future, the values are integer strings from int(np.ceil(x)) in the Python command.
| # Compute memory to use based on the CPU count, following the pattern of | ||
| # 3.75GiB / cpu used by GCP's pricing: https://cloud.google.com/compute/pricing | ||
| Int memory = if defined(machine_mem_mb) then machine_mem_mb else round(cpu * 3.75 * 1024) | ||
| Int memory = select_first([machine_mem_mb, round(cpu * 3.75 * 1024)]) |
There was a problem hiding this comment.
machine_mem_mb is Int? (line 886). The if defined(x) then x else y pattern doesn't narrow the optional, so the result type is Int? rather than Int. select_first does the same thing with correct typing.
|
|
||
| python3 -c " | ||
| with open('indices.out') as f: | ||
| indices = [int(line.strip()) for line in f if line.strip()] |
There was a problem hiding this comment.
The original output used read_lines() which returns Array[String], but callers (JointGenotyping.wdl:397, UltimaGenomicsJointGenotyping.wdl:269) need Array[Int] for array indexing. WDL has no String → Int coercion in any spec version, so we convert the indices file to JSON and use read_json() which can deserialize directly to Array[Int].
There was a problem hiding this comment.
This is a little more involved—I have not tested it myself, but perhaps someone should.
|
|
||
| Int memory_size = ceil((if (disk_size < 110) then 5 else 7) * memory_multiplier) | ||
| String java_memory_size = (memory_size - 1) * 1000 | ||
| Int java_memory_size = (memory_size - 1) * 1000 |
There was a problem hiding this comment.
The RHS (memory_size - 1) * 1000 is Int arithmetic, but the declaration was String. This worked at runtime because the value gets interpolated into the command string anyway, but the declared type should match the expression type.
| input: | ||
| input_map = { | ||
| "passes_qc": InputQC.passes_qc, | ||
| "passes_qc": "~{InputQC.passes_qc}", |
There was a problem hiding this comment.
InputQC.passes_qc is Boolean, but this is a Map[String, String] literal so all values must be String. Wrapping in "~{...}" converts the boolean to its string representation ("true" or "false").
Fixes 16 type mismatch lint errors found by sprocket across 10 files.
The errors fall into several categories:
- Optional-to-non-optional mismatches: uses `select_first()` to unwrap
optional values when passing to task inputs that have defaults
- Int-to-String coercion: uses string interpolation (`"~{i}"`) to
convert `Int` values from `range()` to `String` task inputs
- Conditional optional unwrapping: replaces `if defined(x) then x else y`
with `select_first([x, y])` to avoid type narrowing issues
- Incorrect declared types: changes `String` to `Int` where the RHS is
arithmetic, and `Array[Int]` to `Array[String]` for `read_lines()`
- Boolean-in-Map coercion: uses string interpolation to convert a
`Boolean` value to `String` for a `Map[String, String]` literal
- read_lines-to-Array[Int]: converts task output to use `read_json()`
since WDL has no `String` to `Int` coercion in any spec version
492db6f to
db8e352
Compare
|
@claymcleod Thanks so much for this contribution and highlighting the syntax issue here. Unfortunately, we do not yet have tests for any of the AoU pipelines and cannot change them at this time. We will keep this open to revisit in the future. |
|
Thanks @jessicaway. It would be great if these workflows could be used more generally, please let me know if there's anything I can do to help. |
|
Hello, If you could remove (or start a new PR) any of these changes in the AoU directory here: all_of_us/** then we can go ahead and merge the changes across all of the (non-beta) pipelines outside this directory. Thank you! |
Fixes type mismatch errors flagged by
sprocket lintacross 5 files. These fall into a few categories:range()returnsArray[Int], but some task inputs expectString id. We use string interpolation ("~{i}") to convert.if defined(x) then x else ydoesn't narrow the type ofxfromT?toTin thethenbranch per the WDL spec. Replaced withselect_first([x, y]).String java_memory_size = (memory_size - 1) * 1000assigns anIntexpression to aString— changed toInt. Similarly,Array[Int] mem_gb_per_chunk = read_lines(...)is invalid sinceread_linesalways returnsArray[String].InputQC.passes_qcisBoolean, but the map literal expectsStringvalues. Wrapped with"~{...}".String→Intcoercion in any spec version (checked 1.0, 1.1, and 1.2). ChangedGetFingerprintingIntervalIndicesto write JSON and useread_json()so the output can be typed asArray[Int]for array indexing.Note that there are remaining type mismatch errors related to passing optional types (
T?) to non-optional call inputs that have defaults (T x = default). These are left as-is because the code is actually correct — passingNoneto a defaulted input means "use the default." This behavior was formalized in the WDL 1.2 spec (openwdl/wdl#462, openwdl/wdl#634) and is already implemented by Cromwell and miniwdl for all WDL versions. openwdl/wdl#761 tracks backporting this clarification to the 1.0 and 1.1 specs, after which Sprocket will implement it and these errors will resolve.Depends on #1815.