Propagate non-zero Picard tool exit codes. #4437

cmnbroad · 2018-02-22T00:19:39Z

magicDGS · 2018-02-22T09:16:34Z

src/main/java/org/broadinstitute/hellbender/Main.java

@@ -201,6 +202,10 @@ protected final void mainEntry(final String[] args) {
            }
            handleUserException(e);
            System.exit(COMMANDLINE_EXCEPTION_EXIT_VALUE);
+        } catch (PicardNonZeroExitException e) {


magicDGS · 2018-02-22T09:17:38Z

src/main/java/org/broadinstitute/hellbender/Main.java

@@ -201,6 +202,10 @@ protected final void mainEntry(final String[] args) {
            }
            handleUserException(e);
            System.exit(COMMANDLINE_EXCEPTION_EXIT_VALUE);
+        } catch (PicardNonZeroExitException e) {
+            // a Picard tool returned a non-zero exit code
+            handleResult(e.getToolReturnCode());


Why should the result be printed on the command line?

I did that to retain the Picard return code, which may be different than the one returned by GATK. We could make it return the actual Picard value directly, but that might be misleading since those values are not drawn from the same space as the current GATK values.

I though that the idea was to exit with the same return code as Picard, not to print out the Picard return code and then exit with a different one. For example, for ValidateSamFile the actual exit value has a meaning (see https://github.com/broadinstitute/picard/blob/b9b87773e9339ef6be7d61096adfc7f074b5aa74/src/main/java/picard/sam/ValidateSamFile.java#L174).

That's why I put this comment, but if that was not the idea it's fine too.

The first order concern was to make sure that failure resulted in a non-zero return code. Otherwise, as mentioned above, the values themselves are hard not that reliable - I don't think Picard is even self-consistent, since ValidateSamFile returns 1 if there are validation warnings or if there is a command line parsing error.

droazen · 2018-02-22T21:22:17Z

src/main/java/org/broadinstitute/hellbender/Main.java

+        } catch (final PicardNonZeroExitException e) {
+            // a Picard tool returned a non-zero exit code
+            handleResult(e.getToolReturnCode());
+            System.exit(ANY_OTHER_EXCEPTION_EXIT_VALUE);


I think it would be better to have a separate exit status code for the case where a Picard tool exits with non-0, so that clients will know to check the log to get the underlying Picard exit code.

droazen · 2018-02-22T21:23:31Z

src/main/java/org/broadinstitute/hellbender/exceptions/PicardNonZeroExitException.java

+        Utils.validateArg(picardToolReturnCode != 0, "Return code must be non-zero");
+    }
+
+    public int getToolReturnCode() { return picardToolReturnCode; }


javadoc for this method

droazen · 2018-02-22T21:25:52Z

src/test/java/org/broadinstitute/hellbender/MainTest.java

+        final SecurityManager backup = System.getSecurityManager();
+        try {
+            System.setSecurityManager(new ThrowOnExitSecurityManager());
+            new Main().mainEntry(new String[]{"ExtractSequences"});


Check the return value of mainEntry() to be sure that it's equal to the expected (non-zero) value

We can't do that directly, because mainEntry never returns in this case; it exits when it detects the failure code. The custom security manager class intercepts the exit and throws the ExitNotAllowedException, which includes the code, which is asserted in the catch clause.

droazen

Review complete, back to @cmnbroad, merge after addressing comments.

codecov-io · 2018-02-23T14:27:29Z

Codecov Report

Merging #4437 into master will increase coverage by 0.041%.
The diff coverage is 83.333%.

@@               Coverage Diff               @@
##              master     #4437       +/-   ##
===============================================
+ Coverage     79.104%   79.145%   +0.041%     
- Complexity     16456     16507       +51     
===============================================
  Files           1046      1048        +2     
  Lines          59165     59319      +154     
  Branches        9668      9697       +29     
===============================================
+ Hits           46802     46948      +146     
- Misses          8603      8604        +1     
- Partials        3760      3767        +7

Impacted Files	Coverage Δ	Complexity Δ
...stitute/hellbender/cmdline/CommandLineProgram.java	`84.475% <ø> (+1.935%)`	`73 <0> (+39)`	⬆️
...nder/cmdline/PicardCommandLineProgramExecutor.java	`70% <100%> (+12.857%)`	`3 <2> (+1)`	⬆️
.../main/java/org/broadinstitute/hellbender/Main.java	`71.939% <66.667%> (+1.472%)`	`45 <0> (+2)`	⬆️
...lbender/exceptions/PicardNonZeroExitException.java	`80% <80%> (ø)`	`2 <2> (?)`
...der/tools/walkers/mutect/M2ArgumentCollection.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...ecaller/AssemblyBasedCallerArgumentCollection.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...ls/walkers/mutect/M2FiltersArgumentCollection.java	`100% <0%> (ø)`	`1% <0%> (ø)`	⬇️
...hellbender/tools/spark/pipelines/SortSamSpark.java	`70.588% <0%> (ø)`	`4% <0%> (?)`
...park/sv/discovery/alignment/AlignmentInterval.java	`90.038% <0%> (+0.766%)`	`74% <0%> (+1%)`	⬆️
... and 4 more

…en they should fail).

Propagate non-zero Picard tool exit codes.

3cb9bb1

magicDGS reviewed Feb 22, 2018

View reviewed changes

cmnbroad force-pushed the cn_picard_return_codes branch from 35d61e2 to 90e89d9 Compare February 22, 2018 17:29

Fix additional test that now behaves differently.

90e89d9

droazen reviewed Feb 22, 2018

View reviewed changes

droazen suggested changes Feb 22, 2018

View reviewed changes

droazen assigned cmnbroad Feb 22, 2018

Code review comments.

a3497bd

cmnbroad force-pushed the cn_picard_return_codes branch from 7f75058 to a3497bd Compare February 23, 2018 13:34

Ensure Main tests fail when then should (currently they could pass wh…

4bf6530

…en they should fail).

cmnbroad merged commit 78fbcd8 into master Feb 23, 2018

cmnbroad deleted the cn_picard_return_codes branch February 23, 2018 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagate non-zero Picard tool exit codes. #4437

Propagate non-zero Picard tool exit codes. #4437

cmnbroad commented Feb 22, 2018

magicDGS Feb 22, 2018

magicDGS Feb 22, 2018

cmnbroad Feb 22, 2018 •

edited

Loading

magicDGS Feb 24, 2018

cmnbroad Feb 26, 2018

droazen Feb 22, 2018

cmnbroad Feb 22, 2018

droazen Feb 22, 2018

cmnbroad Feb 22, 2018

droazen Feb 22, 2018 •

edited

Loading

cmnbroad Feb 22, 2018 •

edited

Loading

droazen left a comment

codecov-io commented Feb 23, 2018 •

edited

Loading

Propagate non-zero Picard tool exit codes. #4437

Propagate non-zero Picard tool exit codes. #4437

Conversation

cmnbroad commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad Feb 22, 2018 • 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

droazen Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

cmnbroad Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 23, 2018 • edited Loading

Codecov Report

cmnbroad Feb 22, 2018 •

edited

Loading

droazen Feb 22, 2018 •

edited

Loading

cmnbroad Feb 22, 2018 •

edited

Loading

codecov-io commented Feb 23, 2018 •

edited

Loading