fix: resolves naming conflicts, expression type errors, and misc lint issues - #1817
fix: resolves naming conflicts, expression type errors, and misc lint issues#1817claymcleod wants to merge 6 commits into
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.
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
… issues Fixes 17 Sprocket lint errors across 14 files: - Renames tasks that conflict with their parent workflow name by appending `_task` or `Task` suffix in `convert_vcf_to_plink_bed.wdl`, `run_admixture.wdl`, and `PeakCalling.wdl`. - Renames inputs that conflict with output names in `H5adUtils.wdl`, `Glimpse2LowPassImputationQuotaConsumed.wdl`, and `Verifysnm3C.wdl`. - Wraps optional types with `select_first()` where used in arithmetic or string concatenation expressions. - Removes duplicate `cloud_provider` call input in `Multiome.wdl`. - Removes invalid `sep` placeholder option on non-array `File` inputs in `sample_fastq.14.wdl`. - Fixes `cpu` runtime values from `String` to `Int` in two admixture task files.
| String pipeline_version = "aou_9.0.1" | ||
|
|
||
| call convert_vcf_to_plink_bed { | ||
| call convert_vcf_to_plink_bed_task { |
There was a problem hiding this comment.
WDL doesn't allow a task and workflow to share the same name in the same file. Renamed the task to convert_vcf_to_plink_bed_task so it doesn't conflict with the workflow. Same pattern applied to run_admixture.wdl and PeakCalling.wdl.
| docker: "mussmann/admixpipe:3.0" | ||
| memory: "31 GB" | ||
| cpu: "4" | ||
| cpu: 4 |
There was a problem hiding this comment.
cpu expects Int or Float per the WDL 1.0 spec, not String. This was previously masked by the conflicting name error above. Same fix in run_admixture.wdl.
| # hc_contamination will be None if hasContamination is not defined (I think) OR contamination_major not defined OR contamination_minor not defined | ||
| String hasContamination_2 = select_first([hasContamination,"NOT FOUND"]) | ||
| Float? hc_contamination = if run_contamination && hasContamination_2 == "YES" then (if contamination_major == 0.0 then contamination_minor else 1.0 - contamination_major) else 0.0 | ||
| Float? hc_contamination = if run_contamination && hasContamination_2 == "YES" then (if select_first([contamination_major, 0.0]) == 0.0 then contamination_minor else 1.0 - select_first([contamination_major, 0.0])) else 0.0 |
There was a problem hiding this comment.
contamination_major is Float?, so 1.0 - contamination_major is a type error — you can't do arithmetic on optional types. Wrapped with select_first([contamination_major, 0.0]) to unwrap it, using 0.0 as the fallback which matches the else branch's value.
| cpu: "1" | ||
| disks: "local-disk " + disk_size + " HDD" | ||
| cpu: 1 | ||
| disks: "local-disk " + select_first([disk_size, 100]) + " HDD" |
There was a problem hiding this comment.
disk_size is Int? (with default 100), and string concatenation doesn't work on optional types. Using select_first([disk_size, 100]) to unwrap it with the same default.
| " --resource:1000G,training=true " + one_thousand_genomes_resource_vcf + | ||
| " --resource:mills,training=true,calibration=true " + mills_resource_vcf + " " | ||
| String extract_extra_args = if defined(targets_interval_list) then " -L " + targets_interval_list + " " else "" #only train the model over the targets, apply the model to everything | ||
| String extract_extra_args = if defined(targets_interval_list) then " -L " + select_first([targets_interval_list]) + " " else "" #only train the model over the targets, apply the model to everything |
There was a problem hiding this comment.
targets_interval_list is File?, and even though this is inside an if defined(...) guard, Sprocket doesn't narrow the type in the then branch. select_first([targets_interval_list]) unwraps it — this is safe because we know it's defined at this point.
|
|
||
| # Mem is in units of GB but our command and memory runtime values are in MB | ||
| Int machine_mem = if defined(mem_gb) then mem_gb *1000 else default_ram_mb | ||
| Int machine_mem = if defined(mem_gb) then select_first([mem_gb]) *1000 else default_ram_mb |
There was a problem hiding this comment.
Same pattern — mem_gb is Int? and the if defined(mem_gb) guard doesn't narrow the type. select_first([mem_gb]) unwraps it safely inside the then branch.
| # validate that either crams, or cram manifest is provided | ||
| if (defined(crams)) { | ||
| Int quota_consumed = length(select_first([crams])) | ||
| Int crams_quota_consumed = length(select_first([crams])) |
There was a problem hiding this comment.
The conditional declaration quota_consumed on this line conflicted with the workflow output of the same name on line 40. Renamed to crams_quota_consumed to disambiguate.
| File annotation_file | ||
| File? cellbarcodes | ||
| File? library_metrics | ||
| File? input_library_metrics |
There was a problem hiding this comment.
The input library_metrics conflicted with the output library_metrics on line 133. Renamed the input to input_library_metrics since the output name is widely referenced downstream. Updated callers in Optimus.wdl accordingly. Same fix applied to SingleNucleusOptimusH5adOutput in this file.
| Array[File] truth_all_reads_3C_contacts_array | ||
|
|
||
| Boolean? done | ||
| Boolean? previous_done |
There was a problem hiding this comment.
The input done conflicted with the output done on line 66. Renamed to previous_done since it receives the completion signal from a prior step. Updated the caller in Testsnm3C.wdl.
| star_strand_mode = star_strand_mode, | ||
| count_exons = count_exons, | ||
| soloMultiMappers = soloMultiMappers, | ||
| cloud_provider = cloud_provider, |
There was a problem hiding this comment.
cloud_provider was listed twice in this call statement (first on line 93). Removed the duplicate.
| # Cat files for each r1 and r2 together | ||
| cat ~{sep=' ' read1_fastq} > r1.fastq.gz | ||
| cat ~{sep=' ' read2_fastq} > r2.fastq.gz | ||
| cat ~{read1_fastq} > r1.fastq.gz |
There was a problem hiding this comment.
read1_fastq is a single File, not an Array[File] — the sep placeholder option only works on arrays. The workflow scatters individual files to this task, so sep was never doing anything here. Same for read2_fastq on the next line.
|
@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. |
2 participants
Fixes 17 Sprocket lint errors across 14 files, covering naming conflicts, expression type mismatches, and a few miscellaneous issues.
_task/Tasksuffix inconvert_vcf_to_plink_bed.wdl,run_admixture.wdl, andPeakCalling.wdl.H5adUtils.wdl,Glimpse2LowPassImputationQuotaConsumed.wdl, andVerifysnm3C.wdl. Updated callers inOptimus.wdlandTestsnm3C.wdlaccordingly.select_first()where they were used in arithmetic or string concatenation (4 files).cloud_providerentry inMultiome.wdl.sepon non-array.sample_fastq.14.wdlusedsepon singleFileinputs — removed the unnecessary placeholder option.cputype. FixedString-typedcpuruntime values toIntin two admixture files (exposed after the task rename resolved the name conflict).The only remaining errors (6) are all optional-to-default call input mismatches tracked by stjude-rust-labs/sprocket#812.
Depends on #1816.