Updated WDL generation, upgrade to Barclay 4.0. #6800

cmnbroad · 2020-09-08T15:16:41Z

The last commit adds WDL annotations to a number of new tools. Might be best to review that separately (or we can even move it to a separate PR - I just wanted it here to force increased WDL gen test coverage).

gatk-bot · 2020-09-08T16:03:11Z

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

Test Type	JDK	Job ID	Logs
integration	openjdk11	31339.12	logs
integration	openjdk8	31339.2	logs

droazen

@cmnbroad Back to you with my comments

droazen · 2020-09-08T16:05:33Z

build.gradle

 }

 task gatkWDLGenValidation(dependsOn: [gatkWDLGen, shadowJar]) {
+    // running this task requires a local cromwell installation, with environment variables CROMWELL_JAR,
+    // WOMTOOL_JAR set to the jar locations


Can you add a new section to the README on WDL generation? For now, just the essential instructions on how to run it, anything users might need to know about the output WDLs, and a section for developers on how to write the annotations (including examples for BAM and FASTA inputs/outputs, and also an example of a directory output).

Done. I put the build instructions in the readme, along with a link to the wiki article on the annotations. I'm not sure what you mean by "an example of a directory output". There isn't anything special required for WDL or @Argument annotations for an input or output that is a directory - its all in the code used by the tool to process it.

If I have a tool output that is a directory, I need to know how to write the Workflow annotation for it. If there's nothing special required, the docs should make that clear with an example.

Updated in https://github.com/broadinstitute/gatk/wiki/How-to-Prepare-a-GATK-tool-for-WDL-Auto-Generation.

droazen · 2020-09-08T16:06:21Z

build.gradle

+    // running this task requires a local cromwell installation, with environment variables CROMWELL_JAR,
+    // WOMTOOL_JAR set to the jar locations
+    if (System.getenv('CROMWELL_JAR') == null || System.getenv('WOMTOOL_JAR') == null) {
+        logger.warn("Running this task requires the CROMWELL_JAR and WOMTOOL_JAR environment variables to be set")


Is this a warning that you can recover from, or should it be a fatal error?

Not recoverable, so I made it throw. I had to move it into a doFirst closure though so it only happens when this task is actually executed - it probably should have been that way anyway.

droazen · 2020-09-08T16:15:54Z

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

+         * @return true if program should be executed, false if an information only argument like {@link SpecialArgumentsCollection#HELP_FULLNAME} was specified
+         * @throws CommandLineException if command line validation fails
+         */
+        protected boolean parseArgs(final String[] argv) {


Fix indentation here

Also, I assume you removed final because you actually had to override this method?

Yes, but I wound up not needing to do so. Restored the final and indentation fixed.

droazen · 2020-09-08T16:17:27Z

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

+     * @throws CommandLineException if command line argument validation fails
+     */
+    protected boolean validateArgs(final String[] argv) {
+        return picardCommandLineProgram.getCommandLineParser().parseArguments(System.out, argv);


Will the Picard command-line validation recognize both legacy and POSIX syntax?

No, but only because we never use the legacy parser in gatk. There was a PR a few weeks ago that changed that, but those changes were abandoned.

droazen · 2020-09-08T16:19:59Z

...stitute/hellbender/cmdline/argumentcollections/OptionalReferenceInputArgumentCollection.java

    @Argument(fullName = StandardArgumentDefinitions.REFERENCE_LONG_NAME, shortName = StandardArgumentDefinitions.REFERENCE_SHORT_NAME, doc = "Reference sequence", optional = true)
+    @WorkflowInput(requiredCompanions = { StandardArgumentDefinitions.REFERENCE_INDEX_COMPANION, StandardArgumentDefinitions.REFERENCE_DICTIONARY_COMPANION})


The annotations look a lot better than before!

Yeah, they do.

droazen · 2020-09-08T17:41:29Z

src/test/java/org/broadinstitute/hellbender/utils/help/TestWDLTool.java

+            shortName = "requiredListFileInput",
+            doc = "requiredListFileInput doc",
+            optional = false)
+    @WorkflowInput(requiredCompanions={"requiredListFileInputDictionary", "requiredListFileInputIndex"}, localizationOptional = true)


Can you include at least one @WorkflowInput / @WorkflowOutput in this test class that does not have any companion files?

droazen · 2020-09-08T17:41:54Z

src/test/java/org/broadinstitute/hellbender/utils/help/TestWDLTool.java

+            shortName = "optionalListFileOutput",
+            doc = "optionalListFileOutput doc",
+            optional = true)
+    @WorkflowOutput(requiredCompanions={"optionalListFileOutputDictionary", "optionalListFileOutputIndex"})


No test coverage for optionalCompanions?

Oh wow - done. I also added some mixed companions cases.

droazen · 2020-09-08T17:44:22Z

src/test/java/org/broadinstitute/hellbender/utils/help/WDLGenerationIntegrationTest.java

-    @Test
-    public static void wdlGenSmokeTest() {
-        final File wdlTestTarget = createTempDir("wdlgentest");
+    public void doWDLGenTest(List<String> testPackages, final String sourcePath, final File wdlTestTargetDir) {


droazen · 2020-09-08T17:45:39Z

src/test/resources/org/broadinstitute/hellbender/utils/wdltest/TestWDLTool.wdl

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


Expected results files should be clearly labeled as such, either by being put into a directory with "expected" in the name, or by having "expected" in their file names.

droazen · 2020-09-08T17:47:06Z

src/test/resources/org/broadinstitute/hellbender/utils/wdltest/index.html

+        	<a href="https://gatkforums.broadinstitute.org/gatk/">Support Forum</a>
+        </p>
+
+        <p class="version">GATK version 1.1-111 built at 2016/11/11 11:11:11.


GATK version 1.1-111?

Oh - fixed.

gatk-bot · 2020-09-09T15:11:55Z

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

Test Type	JDK	Job ID	Logs
cloud	openjdk8	31349.1	logs
integration	openjdk11	31349.12	logs
integration	openjdk8	31349.2	logs

cmnbroad · 2020-09-09T20:13:50Z

Back to @droazen.

gatk-bot · 2020-09-09T20:30:03Z

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

Test Type	JDK	Job ID	Logs
integration	openjdk11	31358.12	logs
integration	openjdk8	31358.2	logs

cmnbroad · 2020-09-09T21:30:57Z

@droazen I rebased this on master to pick up the disabling of htsget tests so it can pass tests. I think I responded to everything - feel free to merge if so.

gatk-bot · 2020-09-09T21:42:13Z

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

Test Type	JDK	Job ID	Logs
cloud	openjdk8	31368.1	logs

droazen

@cmnbroad This is approved for merge after you address my remaining comments below. There is one section of commented-out code in build.gradle that looks suspicious to me.

droazen · 2020-09-16T17:36:59Z

build.gradle

+//        wdlFiles.any() { wdlFile ->
+//            final validateWDLCommand = "java -jar $womtoolLocation validate $wdlFile"
+//            execWDLValidation(validateWDLCommand)
+//        }


Looks like you left the womtool validation step commented out -- was this intentional?

Oh good catch- it was a temporary shortcut and not intentional. Fixed.

droazen · 2020-09-16T17:40:11Z

build.gradle

+        final cromwellLocation = System.getenv('CROMWELL_JAR')
+        try {
+            wdlFiles.any() { wdlFile ->
+                final testInputJSON = getWLDInputJSONTestFileNameFromWDLName(wdlFile)


getWLDInputJSON -> getWDLInputJSON

…tor.

…puts.

…lues.

… outputs.

…ng of jopt option clustering.

This reverts commit 9770001.

…e existing test case naming convention.

cmnbroad requested a review from droazen September 8, 2020 15:17

droazen suggested changes Sep 8, 2020

View reviewed changes

droazen assigned droazen and cmnbroad Sep 8, 2020

cmnbroad force-pushed the cn_wdl_fixes_barclay branch from 3608f07 to 4bfbc9f Compare September 9, 2020 21:14

cmnbroad mentioned this pull request Sep 15, 2020

Backport new WDL test cases from GATK. broadinstitute/barclay#171

Merged

droazen approved these changes Sep 16, 2020

View reviewed changes

droazen removed their assignment Sep 16, 2020

cmnbroad force-pushed the cn_wdl_fixes_barclay branch from 4bfbc9f to cc1f618 Compare September 21, 2020 12:13

cmnbroad added 15 commits September 21, 2020 08:17

Enable cromwell execution test to run locally.

0524839

Enable command line validation mode for PicardCommandLineProgramExecu…

7cf7640

…tor.

Generate better execution test values.

cd38474

TEMP: Force WDL generation for all GATK/Picard tools.

32add54

Use Barclay support for required runtime properties and companion out…

266fae3

…puts.

Use ammended summary for companions.

88d3def

Add a WDL test tool to ensure the GATK templates are correct.

2316736

Update templates.

924b733

Properly quote inital values and test values.

9cf8e47

Code update.

18b6030

Move quoting code to Barclay.

71d461a

Update doc for companions in expected WDL test outputs.

8f4a6e4

Use requiredOutputs in default template.

c51edd8

Use requiredCompanions in templates.

d60f84e

Update templates for optional companions.

ac32f45

cmnbroad added 27 commits September 21, 2020 08:17

Generate valid required array test values.

a1a7dd8

Update expected outputs for Barclay change for null optional array va…

6d93f15

…lues.

Update templates and expected outputs for Barclay change for optional…

768e13b

… outputs.

Change RuntimeProperties to WorkflowProperties and CPU defaults to 2.

0e8ebf1

Use string constants from Barclay.

724f017

Fix whitespace in templates.

315cf67

Add localization optional and normalize positionalArgs name.

d389a2d

Remove unnecessary expression placeholders to enable optional outputs.

4f5d00b

Add param_meta section and update test outputs.

644e236

Remove quotes and newlines in summary string to allow use in param_meta.

9eb63a3

Use gatk as app name in template updates ported from Barclay.

56477d6

Update optional args template to include companions as outputs.

62bd3eb

Update method name to reflect Barclay change.

0a2241e

Suppress remove warning for Java 11 build.

2f671b9

Remove extraneous debugging code.

04ca710

Update expected WDL output.

29640d2

Upgrade to Barclay 4.0.0 and update test to reflect Barclay's disabli…

8d2b477

…ng of jopt option clustering.

Revert "TEMP: Force WDL generation for all GATK/Picard tools."

5c6d826

This reverts commit 9770001.

Annotate some tools for WDL gen.

08fbdc2

Miscelleaneous code review comments.

0c03060

Clean up temporary files resulting from WDL validation task.

b82f52f

Update README.md.

fcc3501

Fix WDL test dummy version number.

e470f3a

Add optional-companion and no-companion WDL test cases , and normaliz…

eedcf08

…e existing test case naming convention.

Add mixed companion test cases.

ab1964b

Rename expected test output files.

928e379

Final code review comments for build.gradle.

cc1f618

cmnbroad merged commit 21ca91a into master Sep 21, 2020

cmnbroad deleted the cn_wdl_fixes_barclay branch September 21, 2020 14:10

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

Updated WDL generation, upgrade to Barclay 4.0. (#6800)

35b7c01

		@Argument(fullName = StandardArgumentDefinitions.REFERENCE_LONG_NAME, shortName = StandardArgumentDefinitions.REFERENCE_SHORT_NAME, doc = "Reference sequence", optional = true)
		@WorkflowInput(requiredCompanions = { StandardArgumentDefinitions.REFERENCE_INDEX_COMPANION, StandardArgumentDefinitions.REFERENCE_DICTIONARY_COMPANION})

Updated WDL generation, upgrade to Barclay 4.0. #6800

Updated WDL generation, upgrade to Barclay 4.0. #6800

Conversation

cmnbroad commented Sep 8, 2020 • edited Loading

gatk-bot commented Sep 8, 2020 • edited Loading

droazen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Sep 8, 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

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 Sep 9, 2020 • edited Loading

cmnbroad commented Sep 9, 2020

gatk-bot commented Sep 9, 2020 • edited Loading

cmnbroad commented Sep 9, 2020

gatk-bot commented Sep 9, 2020

droazen 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

cmnbroad commented Sep 8, 2020 •

edited

Loading

gatk-bot commented Sep 8, 2020 •

edited

Loading

cmnbroad Sep 8, 2020 •

edited

Loading

gatk-bot commented Sep 9, 2020 •

edited

Loading

gatk-bot commented Sep 9, 2020 •

edited

Loading