WDL generation. #6504

cmnbroad · 2020-03-16T14:02:24Z

This requires a Barclay upgrade (that is not yet published) since most of the basic WDL generation code is in Barclay. WDL is only generated for tools that are annotated with @RuntimeProperties, and that have input/output files arguments that are annotated with@WorkflowResource.

For each WDL generated, an accompanying JSON input file is generated that contains all the tool's arguments, with the optional arguments initialized to the tool's default values, and the required args initialized to a string that describes the required type. A temporary commit that contains a sample WDL/JSON generated by the WDL gen task in included to make it easier to see the WDL that results.

The only commits in this branch that are directly WDL-gen related are the WDL Gen commit itself, and the sample output commit. The other commits are either related to GATKPathSpecifier migration (not required for WDL gen) or Barclay upgrade migration (required for WDL gen).

cmnbroad · 2020-03-17T19:58:50Z

@bshifaw This is the PR we discussed in slack. Since we won't actually check the generated files into the repo, I added a temporary commit with some example WDL/JSON files so you can see what currently gets generated, and am looking for comments on the WDL structure, etc. Thanks.

droazen · 2020-04-08T15:28:44Z

@bshifaw Just a reminder that this branch is awaiting your review. To clarify, we don't need you to review the code, we just need you to review the generated WDLs/JSONs themselves, looking for problems / missing elements / etc., and suggesting any tweaks you think are necessary. Thanks!

cmnbroad · 2020-04-08T15:49:25Z

@bshifaw Minor tweak to David's comment: you won't be able to run the WDL gen directly from this branch because it requires a barclay upgrade, but I did check a couple of output WDLs in as a temporary commit so you can look at those. If you'd like, I can generate WDL for all the tools and check those in, but I didn't do that because the tools will require some annotation in addition to this branch in order to get WDLs with proper output sections out.

bshifaw · 2020-04-08T15:52:57Z

Sorry, just seeing this now.
I'm mainly looking at the sample output and it looks great!! I can't find anything wrong with it but here are some suggestions for improvement

If its too late now maybe for later it would be nice to add a runtime block for each task so that it would executable in a cloud environment.
Why have inputs declared at a workflow level input block (which requires them to be repeated in the call block, and again at the task input block) when they can be declared simply at the task level?
If it's an easy fix it would be nice to see the full parameter name instead of abbreviation for input and output (e.g. "I", "O")

cmnbroad · 2020-04-13T13:35:37Z

Thanks for the review @bshifaw.

If its too late now maybe for later it would be nice to add a runtime block for each task so that it would executable in a cloud environment.

There is limited support for runtime blocks now (you can see one here) but it requires annotating the tool with the proper values. It currently only supports the memory attribute, but we can certainly add support for other attributes if it makes sense for them to have static values. Perhaps we should add docker ? Others ?

Why have inputs declared at a workflow level input block (which requires them to be repeated in the call block, and again at the task input block) when they can be declared simply at the task level?

I chose to make each WDL have both a workflow and a task, and generated an input JSON that contains the default values for each optional arg so that only the required args have to be provided (they're initialized with the type of value required similar to the skeleton that womtool generates). But maybe theres an alternative structure thats better. Can you point me to a WDL that uses the structure you're proposing? I'm not sure I understand how it would be used.

If it's an easy fix it would be nice to see the full parameter name instead of abbreviation for input and output (e.g. "I", "O")

I used the full names wherever possible, but many of the tools have args with names that are WDL reserved works like input and output, so I had to use the short name in those cases. An alternative would be to mangle/annotate them instead, i.e., turn input into something like input_ or some such thing.

bshifaw · 2020-04-17T12:26:58Z

Runtime Block. Generally diskpace, memory, and docker image would be the main cloud runtime attributes to have for a task. Maybe give them default values, so that users can change them in the json if they would like.
Variable declaration placement. Not absolutely necessary but thought it might help reduce the number of lines in the wdl. Here is an example where the task declares a variable that is not in the call or workflow block: here. Here is a wdl doc about it (link) though its old. Hard to find a direct statement about it in the wdl 1.0 spec but this section hints at it link.

Input naming. Ahh. that makes sense. I often see input_file being used in wdls so that could work.

jonn-smith

I think this looks pretty great. I still need to look at the Barclay branch.

I had a couple of comments / questions. I primarily commented on the PrintReads wdl with a few other comments interspersed.

@cmnbroad and @bshifaw - In terms of the WDL workflow / task inputs - I think the way it's written up now is good. It would save space to only have task level inputs, but this gives more of the sense that each tool can stand on its own as an entity. It's also auto-generated so it's no more work to do it this way.

jonn-smith · 2020-05-08T21:16:55Z

.travis.yml

  - CROMWELL_JAR=$HOME/cromwell-36.jar
+  - WOMTOOL_JAR=$HOME/womtool-36.jar


Do we want to update to a newer cromwell / womtool version? Looks like v47 is used in the gatkWDLGenValidation build task.

Done. I made a separate PR to upgrade to v51, which is merged now, and this is rebased on that.

jonn-smith · 2020-05-08T22:20:49Z

build.gradle

+        def wdlFiles = fileTree(dir: wdlGenFolder)
+        wdlFiles = wdlFiles.filter { f -> !f.getAbsolutePath().endsWith(".html") && !f.getAbsolutePath().endsWith(".json")}
+        return wdlFiles.any() { wdlFile ->
+            //TODO: fix this before submitting


Reminder to remove.

jonn-smith · 2020-05-12T14:13:09Z

tempCommit/PrintReads.wdl

@@ -0,0 +1,297 @@
+version 1.0
+
+# Run PrintReads (GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)


Suggested change

# Run PrintReads (GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)

# Run PrintReads (WDL Autogenerated from: GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)

Oh yeah, good idea.

jonn-smith · 2020-05-12T17:53:18Z

tempCommit/PrintReads.wdl

+
+  input {
+    #Docker to use
+    String dockerImage


I've been thinking about versioning and how it applies to the docker image and GATK executables here.

I think we should have a default value here for the gatk executable so this can run out of the box on a GATK docker image.

Should we have a default for a docker image here? Or should we make a check in the command block below to make sure the GATK is the right version? I'm a little concerned about version mismatches.

This now defaults to broadinstitue/gatk:version.

jonn-smith · 2020-05-12T17:56:23Z

tempCommit/PrintReads.wdl

+  >>>
+
+  runtime {
+      docker: dockerImage


Do we want to give some default parameters for memory and disk?

I typically start with some defaults. Here's something like what I usually start with:

input { Int? mem_gb Int? preemptible_attempts Int? disk_space_gb Int? cpu Int? boot_disk_size_gb } Boolean use_ssd = false Float input_files_size_gb = size(reads_file, "GiB") + size(segments_fasta, "GiB") + size(boundaries_file, "GiB") # You may have to change the following two parameter values depending on the task requirements Int default_ram_mb = 2048 Int default_disk_space_gb = ceil((input_files_size_gb * 2) + 1024) Int default_boot_disk_size_gb = 15 # 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 * 1024 else default_ram_mb runtime { docker: docker_image memory: machine_mem + " MB" disks: "local-disk " + select_first([disk_space_gb, default_disk_space_gb]) + if use_ssd then " SSD" else " HDD" bootDiskSizeGb: select_first([boot_disk_size_gb, default_boot_disk_size_gb]) preemptible: select_first([preemptible_attempts, 1]) cpu: select_first([cpu, 1]) }

The defaults are set in the JSON input files (based on either the default or override values taken from the RuntimeProperties annotations).

jonn-smith · 2020-05-12T18:21:16Z

tempCommit/PrintReads.wdl

+    ~{true='--tmp_dir ' false='' defined(tmp_dir)}~{sep=' --tmp_dir ' tmp_dir} \
+    ~{true='--use_jdk_deflater ' false='' defined(use_jdk_deflater)}~{sep=' --use_jdk_deflater ' use_jdk_deflater} \
+    ~{true='--use_jdk_inflater ' false='' defined(use_jdk_inflater)}~{sep=' --use_jdk_inflater ' use_jdk_inflater} \
+    ~{true='--verbosity ' false='' defined(verbosity)}~{sep=' --verbosity ' verbosity} \


Do we need another newline here because of the trailing backslash?

Oh, yeah, done.

jonn-smith · 2020-05-12T18:22:38Z

src/main/java/org/broadinstitute/hellbender/tools/PrintReads.java

@@ -83,17 +85,20 @@
        programGroup = ReadDataManipulationProgramGroup.class
 )
 @DocumentedFeature
+@RuntimeProperties(memoryRequirements = "1GB")


I was looking for something like this. This is great!

jonn-smith · 2020-05-12T18:23:16Z

src/main/java/org/broadinstitute/hellbender/engine/FeatureManager.java

@@ -248,29 +247,29 @@ void addToFeatureSources(final int featureQueryLookahead, final FeatureInput<? e
    }

    /**
-     * Given a Field known to be of type FeatureInput (or a Collection thereof), retrieves the type
+     * Given a ArgumentDefinition for an argument known to be of type FeatureInput (or a Collection thereof), retrieves the type


Suggested change

* Given a ArgumentDefinition for an argument known to be of type FeatureInput (or a Collection thereof), retrieves the type

* Given an ArgumentDefinition for an argument known to be of type FeatureInput (or a Collection thereof), retrieves the type

This commit is from another branch that was required for the new Barclay version, but thats been merged into master now and this is rebased on that, so this commit is gone.

jonn-smith · 2020-05-12T18:23:54Z

src/main/java/org/broadinstitute/hellbender/utils/help/GATKHelpDocWorkUnitHandler.java

@@ -58,4 +64,27 @@ protected void addCustomBindings(final DocWorkUnit currentWorkUnit) {
        }
    }

+//    @Override


Can this be removed since it's commented out?

Yeah, done.

jonn-smith · 2020-05-12T18:25:55Z

src/main/resources/org/broadinstitute/hellbender/utils/wdlTemplates/common.html

+        			<#if group.supercat == supercat>
+						<li><a data-toggle="collapse" data-parent="#sidenav" href="#${group.id}">${group.name}</a>
+							<div id="${group.id}"
+								<?php echo ($group == '${group.name}')? 'class="accordion-body collapse in"'.chr(62) : 'class="accordion-body collapse"'.chr(62);?>


hoo boy. Are we really still using PHP? Not really a question for this code review… but I'm surprised.

eitanbanks · 2020-05-13T17:38:18Z

Cool stuff!
Some small comments after using this:

Why do you call the task “(toolname)Task” and not “(toolname)”? You might want to ask around for the convention here.
It's weird that --help shows up as one of the command-line options.
Is there a way to disable printing of non-tool-specific arguments? You may even want to consider an option that disables printing out of any arguments that aren't required (since that's what I happened to have wanted to do in my case).

eitanbanks · 2020-05-13T17:51:07Z

Bug: it should be --output in the command block, not --output_arg

cmnbroad · 2020-06-23T21:06:45Z

@jonn-smith Back to you. It took a fair amount of machinery to get a cromwell execution test running on travis with dummy inputs, etc., but its finally working. For the moment, the only tool that is annotated for WDLGen is PrintReads, but as soon as this goes in I'll start annotating others in a separate PR.

ldgauthier · 2020-06-24T16:46:42Z

Are we morally opposed to tracking the generated WDLs with git? That would be the easiest way to get up-to-date single-tool WDLs into Terra, i.e. add each to Dockstore tracking the GATK repo and import into Terra from Dockstore.

droazen · 2020-06-24T16:53:48Z

@ldgauthier Yes, we plan to put the generated WDLs in Dockstore (Comms team has also requested this), and update them with each release.

jonn-smith

One or two very minor questions / suggestions.

Feel free to merge after looking at / addressing them.

jonn-smith · 2020-06-30T14:10:55Z

src/main/java/org/broadinstitute/hellbender/cmdline/CommandLineProgramValidator.java

+/**
+ * Adapter shim for use within GATK to run tools in command line validation mode. Note that this
+ * class does not have it's own CommandLineProgramProperties annotation, and isn't intended to be
+ * run directly from the command line.


It might be worth adding in some documentation / naming the class to indicate that it really only validates the command line arguments for a given tool.

jonn-smith · 2020-06-30T14:26:43Z

src/main/java/org/broadinstitute/hellbender/utils/help/GATKWDLDoclet.java

+    /**
+     * Adds a super-category so that we can custom-order the categories in the doc index
+     *
+     * @param docWorkUnit


javadoc needs updating here

jonn-smith · 2020-06-30T14:31:54Z

src/main/java/org/broadinstitute/hellbender/utils/help/GATKWDLWorkUnitHandler.java

+    }
+
+    /**
+     * Add the named argument {@code argDed}to the property map if applicable.


Suggested change

* Add the named argument {@code argDed}to the property map if applicable.

* Add the named argument {@code argDed} to the property map if applicable.

…ons on a separate branch.

gatk-bot · 2020-06-30T19:43:16Z

Travis reported job failures from build 30848
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
cloud	openjdk11	30848.14	logs

cmnbroad requested a review from bshifaw March 23, 2020 13:38

cmnbroad force-pushed the cn_wdl_gen_barclay branch from bfccfa3 to e36a243 Compare March 24, 2020 22:47

cmnbroad mentioned this pull request Mar 24, 2020

WDL Generation. broadinstitute/barclay#162

Merged

droazen requested a review from jonn-smith April 1, 2020 17:42

droazen assigned jonn-smith and bshifaw Apr 1, 2020

bshifaw approved these changes Apr 8, 2020

View reviewed changes

cmnbroad force-pushed the cn_wdl_gen_barclay branch from e36a243 to c46d362 Compare April 28, 2020 22:34

jonn-smith requested changes May 12, 2020

View reviewed changes

cmnbroad force-pushed the cn_wdl_gen_barclay branch 2 times, most recently from de42cb2 to 42e2493 Compare June 16, 2020 17:28

cmnbroad mentioned this pull request Jun 16, 2020

Upgrade to Barclay 3.0.0. broadinstitute/picard#1526

Merged

cmnbroad force-pushed the cn_wdl_gen_barclay branch 7 times, most recently from 45b362c to 70e1bbd Compare June 23, 2020 20:50

cmnbroad marked this pull request as ready for review June 23, 2020 21:02

cmnbroad changed the title ~~DRAFT: WDL generation.~~ WDL generation. Jun 23, 2020

jonn-smith requested changes Jun 30, 2020

View reviewed changes

cmnbroad force-pushed the cn_wdl_gen_barclay branch from ce2be87 to 6550649 Compare June 30, 2020 17:21

cmnbroad added 2 commits June 30, 2020 13:22

WDL generation.

6550649

Fix a template reference issue discvoered while testing with annotati…

62b7d5e

…ons on a separate branch.

jonn-smith approved these changes Jun 30, 2020

View reviewed changes

cmnbroad merged commit 896e54e into master Jun 30, 2020

cmnbroad deleted the cn_wdl_gen_barclay branch June 30, 2020 21:42

jonn-smith pushed a commit that referenced this pull request Jul 14, 2020

WDL generation. (#6504)

59d202b

cmnbroad mentioned this pull request Aug 6, 2020

WDL optional output support [BA-6545] broadinstitute/cromwell#5720

Merged

mwalker174 pushed a commit that referenced this pull request Nov 3, 2020

WDL generation. (#6504)

0ce57a4

		- CROMWELL_JAR=$HOME/cromwell-36.jar
		- WOMTOOL_JAR=$HOME/womtool-36.jar

		@@ -0,0 +1,297 @@
		version 1.0

		# Run PrintReads (GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)

	# Run PrintReads (GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)
	# Run PrintReads (WDL Autogenerated from: GATK Version 4.1.7.0-7-gcd21b9c-SNAPSHOT)

	* Given a ArgumentDefinition for an argument known to be of type FeatureInput (or a Collection thereof), retrieves the type
	* Given an ArgumentDefinition for an argument known to be of type FeatureInput (or a Collection thereof), retrieves the type

	* Add the named argument {@code argDed}to the property map if applicable.
	* Add the named argument {@code argDed} to the property map if applicable.

WDL generation. #6504

WDL generation. #6504

Conversation

cmnbroad commented Mar 16, 2020 • edited Loading

cmnbroad commented Mar 17, 2020 • edited Loading

droazen commented Apr 8, 2020 • edited Loading

cmnbroad commented Apr 8, 2020

bshifaw commented Apr 8, 2020 • edited Loading

cmnbroad commented Apr 13, 2020

bshifaw commented Apr 17, 2020

jonn-smith 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

Choose a reason for hiding this comment

cmnbroad Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Jun 23, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eitanbanks commented May 13, 2020

eitanbanks commented May 13, 2020

cmnbroad commented Jun 23, 2020

ldgauthier commented Jun 24, 2020

droazen commented Jun 24, 2020

jonn-smith 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

gatk-bot commented Jun 30, 2020

cmnbroad commented Mar 16, 2020 •

edited

Loading

cmnbroad commented Mar 17, 2020 •

edited

Loading

droazen commented Apr 8, 2020 •

edited

Loading

bshifaw commented Apr 8, 2020 •

edited

Loading

cmnbroad Jun 23, 2020 •

edited

Loading

cmnbroad Jun 23, 2020 •

edited

Loading