Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Propagate non-zero Picard tool exit codes. #4437

Merged
merged 4 commits into from
Feb 23, 2018
Merged

Propagate non-zero Picard tool exit codes. #4437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3cb9bb1
Propagate non-zero Picard tool exit codes.
cmnbroad Feb 22, 2018
90e89d9
Fix additional test that now behaves differently.
cmnbroad Feb 22, 2018
a3497bd
Code review comments.
cmnbroad Feb 22, 2018
4bf6530
Ensure Main tests fail when then should (currently they could pass wh…
cmnbroad Feb 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/main/java/org/broadinstitute/hellbender/Main.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import htsjdk.samtools.util.StringUtil;
import org.broadinstitute.hellbender.cmdline.CommandLineProgram;
import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
import org.broadinstitute.hellbender.exceptions.PicardNonZeroExitException;
import org.broadinstitute.hellbender.exceptions.UserException;
import org.broadinstitute.hellbender.utils.ClassUtils;
import org.broadinstitute.hellbender.utils.runtime.RuntimeUtils;
Expand Down Expand Up @@ -201,6 +202,10 @@ protected final void mainEntry(final String[] args) {
}
handleUserException(e);
System.exit(COMMANDLINE_EXCEPTION_EXIT_VALUE);
} catch (PicardNonZeroExitException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final

// a Picard tool returned a non-zero exit code
handleResult(e.getToolReturnCode());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should the result be printed on the command line?

Copy link
Collaborator Author

@cmnbroad cmnbroad Feb 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

System.exit(ANY_OTHER_EXCEPTION_EXIT_VALUE);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} catch (final UserException e){
handleUserException(e);
System.exit(USER_EXCEPTION_EXIT_VALUE);
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/org/broadinstitute/hellbender/cmdline/PicardCommandLineProgramExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.broadinstitute.hellbender.cmdline;

import org.broadinstitute.hellbender.exceptions.GATKException;
import org.broadinstitute.hellbender.exceptions.PicardNonZeroExitException;

/**
* Adapter shim for use within GATK to run Picard tools. Note that this class does not have
Expand All @@ -21,7 +22,11 @@ public PicardCommandLineProgramExecutor(final picard.cmdline.CommandLineProgram
*/
@Override
public Object instanceMain(final String[] argv) {
return picardCommandLineProgram.instanceMain(argv);
int toolReturnCode = picardCommandLineProgram.instanceMain(argv);
if (toolReturnCode != 0) {
throw new PicardNonZeroExitException(toolReturnCode);
}
return toolReturnCode;
}

@Override
Expand Down
22 changes: 22 additions & 0 deletions src/main/java/org/broadinstitute/hellbender/exceptions/PicardNonZeroExitException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package org.broadinstitute.hellbender.exceptions;

import org.broadinstitute.hellbender.utils.Utils;

/**
* Exception used to propagate non-zero return values from Picard tools.
*/
public class PicardNonZeroExitException extends RuntimeException {
private static final long serialVersionUID = 0L;
private final int picardToolReturnCode;

/**
*
* @param picardToolReturnCode the non-zero return code returned from the Picard tool
*/
public PicardNonZeroExitException(final int picardToolReturnCode) {
this.picardToolReturnCode = picardToolReturnCode;
Utils.validateArg(picardToolReturnCode != 0, "Return code must be non-zero");
}

public int getToolReturnCode() { return picardToolReturnCode; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javadoc for this method

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
13 changes: 13 additions & 0 deletions src/test/java/org/broadinstitute/hellbender/MainTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,4 +94,17 @@ public void testMainErrorWithoutStackTrace() {
System.setSecurityManager(backup);
}
}
@Test(singleThreaded = true)
public void testNonZeroPicardReturnValue() {
final SecurityManager backup = System.getSecurityManager();
try {
System.setSecurityManager(new ThrowOnExitSecurityManager());
new Main().mainEntry(new String[]{"ExtractSequences"});
Copy link
Collaborator

@droazen droazen Feb 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@cmnbroad cmnbroad Feb 22, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} catch (ExitNotAllowedException e) {
Assert.assertNotEquals(e.status, 0);
} finally {
System.setSecurityManager(backup);
}
}

}