Added argument to disable RscriptExecutor by orlicohen · Pull Request #7900 · broadinstitute/gatk

orlicohen · 2022-06-14T18:49:27Z

Fixes #7697. Added argument to allow RScriptExecutor to disable but still output Rscript file.

jamesemery

It looks like this is rebased off of one of your previous branches. We should fix that. I also think make the tests a little more agnostic to whatever settings were being tested in testVariantRecalibratorSampling.

jamesemery · 2022-06-14T19:10:48Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/variantutils/SelectVariants.java

    public void onTraversalStart() {
        final Map<String, VCFHeader> vcfHeaders = Collections.singletonMap(getDrivingVariantsFeatureInput().getName(), getHeaderForVariants());

+        final List<String> genotypeField = getHeaderForVariants().getGenotypeSamples();


This is from the wrong PR. #7887. Can you rebase and remove the SelectVariants commit please?

jamesemery · 2022-06-14T19:12:49Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java

            if(!rScriptExecutor.externalExecutableExists()) {
-                Utils.warnUser(logger, String.format(
+                if(!disableRScriptExecutor) {
+                    throw new UserException("Rscript not found in environment path. Fix executor or run with disableRScriptExecutor argument to generate Rscript without running.");


If you are adding an argument "--" so its easy for the user to paste it into their command line.

Also be sure that the argument is properly kebab-cased (--disable-rscriptexecutor) in the message.

jamesemery · 2022-06-14T19:13:17Z

.../org/broadinstitute/hellbender/tools/walkers/variantutils/SelectVariantsIntegrationTest.java


+    @Test(expectedExceptions = UserException.ValidationFailure.class)
+    public void testResortingFileWarning() throws IOException {
+        final String testFile = getToolTestDataDir() + "unsortedGenotypeFieldsTestFile.vcf";


more wrong-pr code.

jamesemery · 2022-06-14T19:13:54Z

...ava/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibratorIntegrationTest.java

    }

+    // Expected exception is a UserException but gets wrapped in a RuntimeException
+    @Test(expectedExceptions = RuntimeException.class)


this should fail now no? You changed that to a user exception?.

jamesemery · 2022-06-14T19:30:23Z

...ava/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibratorIntegrationTest.java

+    // Expected exception is a UserException but gets wrapped in a RuntimeException
+    @Test(expectedExceptions = RuntimeException.class)
+    public void testVariantRecalibratorFailedRscriptOutput() throws IOException {
+        final String inputFile = getLargeVQSRTestDataDir() + "phase1.projectConsensus.chr20.1M-10M.raw.snps.vcf";


The integration test spec and these arguments are a lot. I would recommend taking the String[] VQSRSNPParamsWithResources above and adding your rscript arguments to it and running that through the runCommandLineArguments() method. Furthermore i think this test is actually sketchy since it could be the case that R is properly installed... The second one is probably sufficient.

gatk-bot · 2022-06-14T20:26:01Z

Github actions tests reported job failures from actions build 2497440564
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	8	2497440564.0	logs

codecov · 2022-06-15T14:48:47Z

Codecov Report

Merging #7900 (161a164) into master (d22b752) will increase coverage by 0.074%.
The diff coverage is 61.290%.

❗ Current head 161a164 differs from pull request most recent head b860120. Consider uploading reports for the commit b860120 to get more accurate results

@@               Coverage Diff               @@
##              master     #7900       +/-   ##
===============================================
+ Coverage     86.933%   87.007%   +0.074%     
- Complexity     36950     36977       +27     
===============================================
  Files           2221      2221               
  Lines         173833    173849       +16     
  Branches       18778     18782        +4     
===============================================
+ Hits          151118    151260      +142     
+ Misses         16083     15956      -127     
- Partials        6632      6633        +1

Impacted Files	Coverage Δ
...bender/tools/walkers/vqsr/VariantRecalibrator.java	`86.168% <8.333%> (+24.841%)`	⬆️
...lkers/vqsr/VariantRecalibratorIntegrationTest.java	`98.315% <94.737%> (-0.481%)`	⬇️
.../tools/walkers/vqsr/VariantRecalibratorEngine.java	`81.538% <0.000%> (+1.538%)`	⬆️
...lbender/tools/walkers/vqsr/VariantDataManager.java	`78.947% <0.000%> (+7.287%)`	⬆️

gatk-bot · 2022-06-15T15:29:28Z

Github actions tests reported job failures from actions build 2503081068
Failures in the following jobs:

Test Type	JDK	Job ID	Logs
integration	11	2503081068.12	logs
integration	8	2503081068.0	logs

jamesemery

Two very minor style comments and then you can merge.

jamesemery · 2022-06-24T17:12:14Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java

-                Utils.warnUser(logger, String.format(
+                if(!disableRScriptExecutor) {
+                    throw new UserException("Rscript not found in environment path. Fix executor or run with --dont-run-rscript argument to generate Rscript without running.");
+                /* Utils.warnUser(logger, String.format(


drop quoted code.

jamesemery · 2022-06-24T17:13:11Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/vqsr/VariantRecalibrator.java


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


"rscript file"

orlicohen · 2022-06-24T21:12:13Z

Fixes #7697. Added argument option to generate and output rscript file without running it.

orlicohen requested a review from jamesemery June 14, 2022 18:49

orlicohen assigned jamesemery Jun 14, 2022

jamesemery suggested changes Jun 14, 2022

View reviewed changes

orlicohen added 3 commits June 15, 2022 10:45

added a test demonstrating the failure without R code loaded

94aea0e

Added argument to disable RscriptExecutor

a8bd094

working on integration tests, in progress

5880730

orlicohen force-pushed the oc_fix7697_disablerscriptexecutor branch from a7829e8 to 5880730 Compare June 15, 2022 14:47

responding to review comments

161a164

orlicohen requested a review from droazen June 16, 2022 15:13

orlicohen assigned droazen Jun 17, 2022

jamesemery approved these changes Jun 24, 2022

View reviewed changes

responding to review comments

b860120

orlicohen merged commit a28dfff into master Jun 24, 2022

orlicohen deleted the oc_fix7697_disablerscriptexecutor branch June 24, 2022 21:13

Conversation

orlicohen commented Jun 14, 2022

Uh oh!

jamesemery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatk-bot commented Jun 14, 2022

Uh oh!

codecov bot commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gatk-bot commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesemery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlicohen commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jun 15, 2022 •

edited

Loading

gatk-bot commented Jun 15, 2022 •

edited

Loading