Update iChor WDLs for WDL 1.0 #77

geoffjentry · 2025-01-06T18:36:27Z

This PR does a couple of things:

Updates to WDL 1.0, including necessary syntactical changes
Removes the use of optional inputs when a default is provided
Minor formatting adjustments

The removal of the optionals was useful because of constructs like disks: "local-disk " + diskGB + " HDD" which is not technically valid (e.g. what if diskGB is None?) although Cromwell does allow it. It also removed some warnings from miniwdl check listing.

I don't have access to the docker containers at the moment so can't run this to verify the changes are valid. They shouldn't be modifying any behavior, but it'd be good to run this first before merging.

EDIT NB: While this has been open we merged another commit, so now this is doing a bit more. Specifically (from the other PR description):

The majority of line changes are changes to spacing.
- Pretty-format input JSON for readability
- Changes to WDL indents
- Changes to python spacing suggested by the tool ruff format
Changes suggested by shellcheck
- Files should be quoted by strings to avoid breaking at white space
- Lists of files are best handled in arrays to prevent splitting
General WDL best-practices
- prefer ~{var_name} to ${var_name}
- prefer "~{foo}-bar" to foo + "-bar"
- set -euxo pipefail at the beginning of tasks so that errors cause task failures rather than returning nonsense
- Replace optional input and select_first with default values when they are known in advance.
General python best-practices
- Use context manager to open files so that they are auto-closed, always specify mode even if it's 'r'.

geoffjentry · 2025-01-06T18:37:06Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

-	Float maxFracCNASubclone
-	Float maxFracGenomeSubclone
+	input {
+		File bam_file


These weren't modified, they're just tabbed over one level

geoffjentry · 2025-01-06T18:37:32Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+		String sample_id
+		Int bin_size
+
+		Int min_qual = 20


As mentioned, the optionals with a default were converted to standard types

geoffjentry · 2025-01-06T18:37:50Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+		Float altFracThreshold
+		Int lambdaScaleHyperParam
+		Int rmCentromereFlankLength
+		String plotFileType = "pdf"


This is one other example of the de-optioning

geoffjentry · 2025-01-06T18:38:23Z

iChor/iChorCNA_step2/iChorCNA_step2.wdl


    call bundle_plots {
        input: CNA_plots = CNA_plots,
               summary_script = summary_script,
               sample_set_name = sample_set_name
    }
    output {
-        bundle_plots.*
+        File bundledPDF = bundle_plots.bundledPDF


There's only this one output from bundle_plots, so I just kept the same name from the task

geoffjentry · 2025-03-17T14:34:04Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+    }
+
+    command <<<
+        set -euxo pipefail


The set additions aren't formatting. However there does not to appear to be any situation in this WDL where it'd not be indicated.

geoffjentry · 2025-03-17T14:35:09Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+            --estimateScPrevalence ~{estimateClonality} \
+            --scStates "~{scStates}" \
+            --centromere ~{centromere} \
+            ~{"--exons.bed '" + exons + "'"} \


Better handling for the optional exons

MicahR-Y

@geoffjentry Added a few suggested edits, but overall it looks good! Have you tested running this WDL on Terra?

MicahR-Y · 2025-03-18T15:21:01Z

iChor/iChorCNA_hg19/iChorCNA_hg19.inputs.json

+  "ichorCNA.mapWig": "gs://gptag/ichorCNA_resources/map_hg19_1000kb.wig",
+  "ichorCNA.maxFracCNASubclone": "${0.7}",
+  "ichorCNA.maxFracGenomeSubclone": "${0.5}",
+  "ichorCNA.normalPanel": "gs://gptag/ichorCNA_resources/Stover_Lennon_UH2_20HD_PON_1X_ULP_median.rds",


Since future samples will all be aligned on DRAGEN, I'd recommend using the PoN generated from DRAGEN-aligned samples: gs://gptag/ichorCNA_resources/Stover_Lennon_UH2_20HD_PON_1X_ULP_median.dragen_aln.rds

@MicahR-Y For both this & the switch to Float - the intent here was to not change actual behavior in this PR. There'll be follow on PRs to change behavior (e.g. the next one is already baking in the Float update)

MicahR-Y · 2025-03-18T15:23:00Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+            bam_index = bam_index,
+            sample_id = sample_id,
+            bin_size = bin_size,
+            chrs = chr_counter,


I don't know if this throws an error, but I believe the comma after the last input in each of these input blocks is unnecessary.

Yeah I think this is actually a bug in miniwdl. We'll update.

🤦 I misread. I thought this was an input block not a call block, and thus was mystified how it was working. This is fine.

MicahR-Y · 2025-03-18T15:28:40Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+        String subclone_fraction = read_string("subclone_fraction")
+        String fraction_genome_subclonal = read_string("fraction_genome_subclonal")
+        String fraction_cna_subclonal = read_string("fraction_cna_subclonal")
+        String gc_map_correction_mad = read_string("gc-map_correction_mad")


As discussed gc_map_correction_mad can be read in as a Float with read_float

geoffjentry · 2025-03-18T17:28:23Z

@MicahR-Y re the three individual comments: looking to make the 2 changes to future PRs (one was already planned). And we can remove those commas.

We're not able to run this locally as we don't yet have access to the PON/WIG/etc files and I would have thought on Terra w/ the dock container having been private & us not having access to those buckets that'd also be an issue. The actual script changes (vs WDL syntax) were tested locally.

This raises another point which is the hg19 vs hg38. It looks like for OC we do need to keep support for both references. But the only delta is in the inputs JSON, not the WDL. Can also cross that bridge in a future PR.

geoffjentry · 2025-03-18T18:55:10Z

@MicahR-Y So @TedBrookings pointed out I had been misreading the WDL surrounding your comment on the commas. It's fine.

geoffjentry · 2025-03-25T23:36:36Z

iChor/iChorCNA_hg19/iChorCNA_hg19.wdl

+            "~{sample_id}.cna.seg" \
+            "~{sample_id}.seg.txt" \
+            "~{sample_id}.seg" \
+            "~{sample_id}.optimalSolution/"


@MicahR-Y this was an issue in the original PR. I'm going to rerun w/ my rebased version just to make sure there wasn't a merge conflict.

MicahR-Y

Everything I asked about has been or will be addressed, looks good to me!

geoffjentry commented Jan 6, 2025

View reviewed changes

geoffjentry commented Mar 17, 2025

View reviewed changes

MicahR-Y requested changes Mar 18, 2025

View reviewed changes

geoffjentry and others added 3 commits March 25, 2025 19:34

Update iChor WDLs for WDL 1.0

96acdc4

Lint iChorCNA_hg19 WDL and input JSON (#1)

c3e0819

fix script issue

50321d6

geoffjentry force-pushed the jg_ichor_wdl_1_0 branch from e8c7258 to 50321d6 Compare March 25, 2025 23:35

geoffjentry commented Mar 25, 2025

View reviewed changes

Found some merge issues w/ docker name

a0b81ee

MicahR-Y approved these changes Mar 28, 2025

View reviewed changes

MicahR-Y merged commit 27672b3 into broadinstitute:master Mar 28, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update iChor WDLs for WDL 1.0 #77

Update iChor WDLs for WDL 1.0 #77

geoffjentry commented Jan 6, 2025 •

edited

Loading

geoffjentry Jan 6, 2025

geoffjentry Jan 6, 2025

geoffjentry Jan 6, 2025

geoffjentry Jan 6, 2025

geoffjentry Mar 17, 2025

geoffjentry Mar 17, 2025

MicahR-Y left a comment

MicahR-Y Mar 18, 2025

geoffjentry Mar 18, 2025

MicahR-Y Mar 18, 2025

geoffjentry Mar 18, 2025

geoffjentry Mar 18, 2025

MicahR-Y Mar 18, 2025

geoffjentry commented Mar 18, 2025

geoffjentry commented Mar 18, 2025

geoffjentry Mar 25, 2025

MicahR-Y left a comment

Update iChor WDLs for WDL 1.0 #77

Update iChor WDLs for WDL 1.0 #77

Conversation

geoffjentry commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MicahR-Y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffjentry commented Mar 18, 2025

geoffjentry commented Mar 18, 2025

Choose a reason for hiding this comment

MicahR-Y left a comment

Choose a reason for hiding this comment

geoffjentry commented Jan 6, 2025 •

edited

Loading