prevent users from requesting g.vcf.gz in Spark #4277

lbergelson · 2018-01-26T21:26:08Z

this is currently broken, see HaplotypeCallerSpark doesn't write g.vcf.gz files #4274
add a check to HaplotypeCallerSpark and VariantSparkSink and throw a clear exception in this case
added test for GVCF writing in VariantSparkSink which previously didn't exist
added new UserException.UnimplementedFeature class
closes prevent running HaplotypeCallerSpark with broken output formats #4275

* this is currently broken, see #4274 * add a check to HaplotypeCallerSpark and VariantSparkSink and throw a clear exception in this case * added test for GVCF writing in VariantSparkSink which previously didn't exist * added new UserException.UnimplementedFeature class * closes #4275

codecov-io · 2018-01-29T20:14:05Z

Codecov Report

Merging #4277 into master will increase coverage by 0.06%.
The diff coverage is 87.5%.

@@              Coverage Diff               @@
##              master     #4277      +/-   ##
==============================================
+ Coverage     79.087%   79.147%   +0.06%     
- Complexity     16612     16707      +95     
==============================================
  Files           1049      1051       +2     
  Lines          59580     59808     +228     
  Branches        9730      9758      +28     
==============================================
+ Hits           47120     47336     +216     
- Misses          8681      8685       +4     
- Partials        3779      3787       +8

Impacted Files	Coverage Δ	Complexity Δ
...stitute/hellbender/tools/HaplotypeCallerSpark.java	`83.673% <100%> (+0.516%)`	`26 <4> (+3)`	⬆️
...institute/hellbender/exceptions/UserException.java	`71.111% <100%> (+0.434%)`	`3 <0> (ø)`	⬇️
...er/engine/spark/datasources/VariantsSparkSink.java	`86.047% <66.667%> (+2.713%)`	`17 <0> (+4)`	⬆️
...ollections/RequiredIntervalArgumentCollection.java	`83.333% <0%> (ø)`	`6% <0%> (+3%)`	⬆️
...ellbender/engine/filters/ReadLengthReadFilter.java	`100% <0%> (ø)`	`8% <0%> (+4%)`	⬆️
...ender/engine/filters/FragmentLengthReadFilter.java	`100% <0%> (ø)`	`8% <0%> (+4%)`	⬆️
...ender/engine/filters/MappingQualityReadFilter.java	`100% <0%> (ø)`	`10% <0%> (+5%)`	⬆️
...e/hellbender/engine/filters/LibraryReadFilter.java	`100% <0%> (ø)`	`6% <0%> (+3%)`	⬆️
...llbender/engine/filters/OverclippedReadFilter.java	`100% <0%> (ø)`	`18% <0%> (+9%)`	⬆️
...te/hellbender/engine/filters/ReadStrandFilter.java	`66.667% <0%> (ø)`	`6% <0%> (+3%)`	⬆️
... and 11 more

droazen

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

droazen · 2018-01-29T21:18:55Z

src/main/java/org/broadinstitute/hellbender/engine/spark/datasources/VariantsSparkSink.java

+
+        //TODO remove me when https://github.com/broadinstitute/gatk/issues/4274 is fixed
+        if (writeGvcf && (AbstractFeatureReader.hasBlockCompressedExtension(outputFile) || outputFile.endsWith(IOUtil.BCF_FILE_EXTENSION))) {
+            throw new UserException.UnimplementedFeature("It is currently not possible to write a compressed g.vcf or bcf.gz on spark.  See https://github.com/broadinstitute/gatk/issues/4274 for more details.");


Doesn't this forbid writing any BCFs when in GVCF mode (not just bcf.gz)? Was this the intent? Both the error message and the ticket #4274 seem incomplete, since they don't mention disallowing g.bcf

That was the intent, it's currently not possible to write g.bcf at all, compressed or not. I think we are actually compressing bcf though automatically on spark, but it interacts strangely with the .gz codepaths. I've updated the comment and opened a new issue #4303

droazen · 2018-01-29T21:21:42Z

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

+        private static final long serialVersionUID = 0L;
+
+        public UnimplementedFeature(String message){
+            super(message);


New UserException subclasses should generally prepend some boilerplate text to the error message in their constructors (eg, "Feature is not currently implemented: %s")

I feel like this generally leads to more confusing error messages since you have to fight the existing text to get it to say what you want.

It leads to more consistent error messages. You don't have to "fight" the existing text so much as just look at it before adding your message.

droazen · 2018-01-29T21:23:52Z

src/main/java/org/broadinstitute/hellbender/tools/HaplotypeCallerSpark.java

+        if(hcArgs.emitReferenceConfidence == ReferenceConfidenceMode.GVCF
+                && (AbstractFeatureReader.hasBlockCompressedExtension(output) || output.endsWith(IOUtil.BCF_FILE_EXTENSION))) {
+            throw new UserException.UnimplementedFeature("It is currently not possible to write a compressed g.vcf or bcf.gz from HaplotypeCallerSpark.  " +
+                                            "See https://github.com/broadinstitute/gatk/issues/4274 for more details.");


Are you repeating this check so that the tool can fail early, or would the check in VariantSparkSink not catch this?

Either way, my comment above also applies here: Doesn't this forbid writing any BCFs when in GVCF mode (not just bcf.gz)? Was this the intent? Both the error message and the ticket #4274 seem incomplete, since they don't mention disallowing g.bcf

Repeating it to catch it early, otherwise it will run the entire haplotype caller and then fail. Updated here as well.

droazen · 2018-01-29T21:30:20Z

...t/java/org/broadinstitute/hellbender/engine/spark/datasources/VariantsSparkSinkUnitTest.java

+    @Test(expectedExceptions = UserException.UnimplementedFeature.class)
+    public void testGVCF_GZ_isDisallowed() throws IOException {
+        JavaSparkContext ctx = SparkContextFactory.getTestSparkContext();
+        VariantsSparkSink.writeVariants(ctx, createTempFile("test",".g.vcf.gz").toString(), null,


Also test that .g.bcf and .g.bcf.gz are disallowed.

droazen · 2018-01-29T21:33:10Z

...t/java/org/broadinstitute/hellbender/engine/spark/datasources/VariantsSparkSinkUnitTest.java

+            public String getTestedToolName(){
+                return IndexFeatureFile.class.getSimpleName();
+            }
+        }.runCommandLine(new String[]{"-F", output.getAbsolutePath()});


Isn't new Main().instanceMain(args) simpler here?

It is but it misses out on some things like setting the default test verbosity.

droazen · 2018-01-29T21:34:24Z

src/test/java/org/broadinstitute/hellbender/tools/HaplotypeCallerSparkIntegrationTest.java

+        final String[] args = {
+                "-I", NA12878_20_21_WGS_bam,
+                "-R", b37_2bit_reference_20_21,
+                "-O", createTempFile("testGVCF_GZ_throw_exception", ".g.vcf.gz").getAbsolutePath(),


Also test that .g.bcf and .g.bcf.gz are disabled.

droazen

👍

* prevent users from requesting g.vcf.gz in Spark * this is currently broken, see #4274 * add a check to HaplotypeCallerSpark and VariantSparkSink and throw a clear exception in this case * added test for GVCF writing in VariantSparkSink which previously didn't exist * added new UserException.UnimplementedFeature class * closes #4275

lbergelson assigned droazen Jan 26, 2018

lbergelson requested a review from droazen January 26, 2018 21:26

lbergelson added 2 commits January 29, 2018 13:32

fix for test

bb0a5ba

lbergelson force-pushed the lb_fix_spark_g_vcf_gz branch from 27e285b to bb0a5ba Compare January 29, 2018 19:05

droazen suggested changes Jan 29, 2018

View reviewed changes

droazen assigned lbergelson and unassigned droazen Jan 29, 2018

responding to comments

a86f570

droazen approved these changes Jan 30, 2018

View reviewed changes

droazen merged commit d6e7635 into master Jan 30, 2018

droazen deleted the lb_fix_spark_g_vcf_gz branch January 30, 2018 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent users from requesting g.vcf.gz in Spark #4277

prevent users from requesting g.vcf.gz in Spark #4277

lbergelson commented Jan 26, 2018

codecov-io commented Jan 29, 2018 •

edited

Loading

droazen left a comment

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen Jan 29, 2018

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen Jan 29, 2018

lbergelson Jan 29, 2018

droazen left a comment

prevent users from requesting g.vcf.gz in Spark #4277

prevent users from requesting g.vcf.gz in Spark #4277

Conversation

lbergelson commented Jan 26, 2018

codecov-io commented Jan 29, 2018 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 29, 2018 •

edited

Loading