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

Added argument to disable RscriptExecutor #7900

Merged
merged 5 commits into from
Jun 24, 2022
Merged

Added argument to disable RscriptExecutor #7900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
94aea0e
added a test demonstrating the failure without R code loaded
orlicohen Jun 10, 2022
a8bd094
Added argument to disable RscriptExecutor
orlicohen Jun 14, 2022
5880730
working on integration tests, in progress
orlicohen Jun 15, 2022
161a164
responding to review comments
orlicohen Jun 16, 2022
b860120
responding to review comments
orlicohen Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,12 @@ public class VariantRecalibrator extends MultiVariantWalker {
@VisibleForTesting
protected int max_attempts = 1;

@Advanced
@Argument(fullName="dont-run-rscript",
doc="Disable the RScriptExecutor to allow RScript to be generated but not run",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"rscript file"

optional=true)
private boolean disableRScriptExecutor = false;

/////////////////////////////
// Debug Arguments
/////////////////////////////
Expand Down Expand Up @@ -403,9 +409,9 @@ public void onTraversalStart() {
if (RSCRIPT_FILE != null) {
rScriptExecutor = new RScriptExecutor();
if(!rScriptExecutor.externalExecutableExists()) {
Utils.warnUser(logger, String.format(
"Rscript not found in environment path. %s will be generated but PDF plots will not.",
RSCRIPT_FILE));
if(!disableRScriptExecutor) {
throw new UserException("Rscript not found in environment path. Fix executor or run with --dont-run-rscript argument to generate rscript file without running.");
}
}
}

Expand Down Expand Up @@ -719,12 +725,14 @@ public Object onTraversalSuccess() {
//skip R plots for scattered tranches because the format is different and the R code parses them
logger.info("Tranches plot will not be generated since we are running in scattered mode");
} else if (RSCRIPT_FILE != null) { //we don't use the RSCRIPT_FILE for tranches, but here it's an indicator if we're setup to run R
// Execute the RScript command to plot the table of truth values
rScriptExecutor.addScript(new Resource(PLOT_TRANCHES_RSCRIPT, VariantRecalibrator.class));
rScriptExecutor.addArgs(TRANCHES_FILE.getAbsoluteFile(), TARGET_TITV);
// Print out the command line to make it clear to the user what is being executed and how one might modify it
logger.info("Executing: " + rScriptExecutor.getApproximateCommandLine());
rScriptExecutor.exec();
if(!disableRScriptExecutor) {
// Execute the RScript command to plot the table of truth values
rScriptExecutor.addScript(new Resource(PLOT_TRANCHES_RSCRIPT, VariantRecalibrator.class));
rScriptExecutor.addArgs(TRANCHES_FILE.getAbsoluteFile(), TARGET_TITV);
// Print out the command line to make it clear to the user what is being executed and how one might modify it
logger.info("Executing: " + rScriptExecutor.getApproximateCommandLine());
rScriptExecutor.exec();
}
}
return true;
}
Expand Down Expand Up @@ -1136,9 +1144,11 @@ private void createVisualizationScript(

// Execute Rscript command to generate the clustering plots
final RScriptExecutor executor = new RScriptExecutor();
executor.addScript(RSCRIPT_FILE);
logger.info("Executing: " + executor.getApproximateCommandLine());
executor.exec();
if(!disableRScriptExecutor) {
executor.addScript(RSCRIPT_FILE);
logger.info("Executing: " + executor.getApproximateCommandLine());
executor.exec();
}
}

// The Arrange function is how we place the 4 model plots on one page
Expand Down
66 changes: 49 additions & 17 deletions .../org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibratorIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,19 @@
import org.broadinstitute.hellbender.CommandLineProgramTest;
import org.broadinstitute.hellbender.cmdline.StandardArgumentDefinitions;
import org.broadinstitute.hellbender.exceptions.UserException;
import org.broadinstitute.hellbender.testutils.ArgumentsBuilder;
import org.broadinstitute.hellbender.utils.Utils;
import org.broadinstitute.hellbender.testutils.IntegrationTestSpec;
import org.broadinstitute.hellbender.utils.io.IOUtils;
import org.testng.Assert;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand All @@ -31,6 +35,11 @@
*/
public class VariantRecalibratorIntegrationTest extends CommandLineProgramTest {

private final String tmpDir = createTempDir(this.getTestedClassName()).getAbsolutePath();
private final String modelReportFilename = tmpDir + "/snpSampledModel.report";
private final String modelReportRecal = getLargeVQSRTestDataDir() + "expected/snpSampledRecal.vcf";
private final String modelReportTranches = getLargeVQSRTestDataDir() + "expected/snpSampledTranches.txt";

private final String[] VQSRSNPParamsWithResources =
new String[] {
"--variant",
Expand Down Expand Up @@ -148,6 +157,23 @@ public class VariantRecalibratorIntegrationTest extends CommandLineProgramTest {
"--" + StandardArgumentDefinitions.ADD_OUTPUT_VCF_COMMANDLINE, "false"
};


final private String[] variantRecalibratorSamplingParams = {
" --variant " + getLargeVQSRTestDataDir() + "phase1.projectConsensus.chr20.1M-10M.raw.snps.vcf" +
" -L 20:1,000,000-10,000,000" +
" --resource:known,known=true,prior=10.0 " + getLargeVQSRTestDataDir() + "dbsnp_132_b37.leftAligned.20.1M-10M.vcf" +
" --resource:truth_training1,truth=true,training=true,prior=15.0 " + getLargeVQSRTestDataDir() + "sites_r27_nr.b37_fwd.20.1M-10M.vcf" +
" --resource:truth_training2,training=true,truth=true,prior=12.0 " + getLargeVQSRTestDataDir() + "Omni25_sites_1525_samples.b37.20.1M-10M.vcf" +
" -an QD -an HaplotypeScore -an HRun" +
" --trust-all-polymorphic" + // for speed
" --output %s" +
" -tranches-file %s" +
" --output-model " + modelReportFilename +
" -mode SNP --max-gaussians 3" + //reduce max gaussians so we have negative training data with the sampled input
" -sample-every 2" +
" --" + StandardArgumentDefinitions.ADD_OUTPUT_VCF_COMMANDLINE +" false"
};

@Override
public String getToolTestDataDir(){
return toolsTestDir + "walkers/VQSR/";
Expand Down Expand Up @@ -359,35 +385,41 @@ public void testBothAggregateRecalMode() throws IOException {
spec.executeTest("testBothRecalMode", this);
}

private final String tmpDir = createTempDir(this.getTestedClassName()).getAbsolutePath();
private final String modelReportFilename = tmpDir + "/snpSampledModel.report";
private final String modelReportRecal = getLargeVQSRTestDataDir() + "expected/snpSampledRecal.vcf";
private final String modelReportTranches = getLargeVQSRTestDataDir() + "expected/snpSampledTranches.txt";

@Test
public void testVariantRecalibratorSampling() throws IOException {
final String inputFile = getLargeVQSRTestDataDir() + "phase1.projectConsensus.chr20.1M-10M.raw.snps.vcf";
final String args = StringUtils.join(variantRecalibratorSamplingParams, " ");

final IntegrationTestSpec spec = new IntegrationTestSpec(
" --variant " + inputFile +
" -L 20:1,000,000-10,000,000" +
" --resource:known,known=true,prior=10.0 " + getLargeVQSRTestDataDir() + "dbsnp_132_b37.leftAligned.20.1M-10M.vcf" +
" --resource:truth_training1,truth=true,training=true,prior=15.0 " + getLargeVQSRTestDataDir() + "sites_r27_nr.b37_fwd.20.1M-10M.vcf" +
" --resource:truth_training2,training=true,truth=true,prior=12.0 " + getLargeVQSRTestDataDir() + "Omni25_sites_1525_samples.b37.20.1M-10M.vcf" +
" -an QD -an HaplotypeScore -an HRun" +
" --trust-all-polymorphic" + // for speed
" --output %s" +
" -tranches-file %s" +
" --output-model " + modelReportFilename +
" -mode SNP --max-gaussians 3" + //reduce max gaussians so we have negative training data with the sampled input
" -sample-every 2" +
" --" + StandardArgumentDefinitions.ADD_OUTPUT_VCF_COMMANDLINE +" false",
args,
Arrays.asList(
modelReportRecal,
modelReportTranches));
spec.executeTest("testVariantRecalibratorSampling"+ inputFile, this);
}


@Test
public void testVariantRecalibratorRScriptOutput() throws IOException {
final String inputFile = getLargeVQSRTestDataDir() + "phase1.projectConsensus.chr20.1M-10M.raw.snps.vcf";
final File unrunRscript = createTempFile("rscriptOutput", ".R");
final String args = StringUtils.join(variantRecalibratorSamplingParams, " ");

final IntegrationTestSpec spec = new IntegrationTestSpec(
args +
" --dont-run-rscript " +
" --rscript-file " + unrunRscript,
Arrays.asList(
modelReportRecal,
modelReportTranches));

spec.executeTest("testVariantRecalibratorRscriptOutput"+ inputFile, this);
Assert.assertTrue(Files.exists(IOUtils.fileToPath(unrunRscript)), "Rscript file was not generated.");
Assert.assertTrue(Files.size(IOUtils.fileToPath(unrunRscript)) > 0, "Rscript file was empty.");
}


@Test(dependsOnMethods = {"testVariantRecalibratorSampling"})
public void testVariantRecalibratorModelInput() throws IOException {
final String inputFile = getLargeVQSRTestDataDir() + "phase1.projectConsensus.chr20.1M-10M.raw.snps.vcf";
Expand Down