Make VariantContextWriterBuilder warn when indexing-on-the-fly is enabled for stream output #1328

jamesemery · 2019-03-15T20:06:29Z

This is the first stage in resolving broadinstitute/gatk#5779. Currently in when we provide a stream (or any non-regular file to resolve) to the VariantContextWriterBuilder we set .setOutputFileType(OutputType.VCF). This sidesteps the code in the switch statement designed to this case and throw an exception if indexing on the fly is enabled. It turns out to be a pain point for the users because indexing on the fly is enabled by default. I propose that we push the responsibility of deciding if the output is a stream up to gatk, and then make the stream code in VariantContextWriterBuilder consistent with the BAM writer, which only warns and disables indexing if it suspects the file might be a stream.

…stream outputs

jamesemery · 2019-03-15T20:19:49Z

@cmnbroad

pshapiro4broad

Looks OK, a few minor comments.

pshapiro4broad · 2019-03-25T17:49:39Z

src/test/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilderUnitTest.java

@@ -347,6 +347,25 @@ public void testIndexingOnTheFly() {
    }
  }

+  @Test
+  public void testIndexingOnTheFlyForPathStream() throws IOException {
+    final VariantContextWriterBuilder builder = new VariantContextWriterBuilder()


Indent should be 4 spaces, not 2. I realize the rest of the file is using 2, but IMO we should move to a common convention for all files, starting with new/updated code.

I can reindent the file pretty easily if you would like? It seems orthogonal to this branch however.

I would just fix the lines that you're adding/modifying.

pshapiro4broad · 2019-03-25T17:51:09Z

src/test/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilderUnitTest.java

+    try (FileSystem fs = Jimfs.newFileSystem("test", Configuration.unix())) {
+      final Path vcfPath = IOUtil.getPath("/dev/null");
+      final Path vcfIdxPath = Tribble.indexPath(vcfPath);
+      // We explicitly setOutputFileType here to mimic gatk behavior where it pretends files that have no extension


Incomplete sentence, or extra that?

pshapiro4broad · 2019-03-25T17:53:17Z

src/main/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilder.java

@@ -107,6 +104,7 @@
    public static final EnumSet<Options> DEFAULT_OPTIONS = EnumSet.of(Options.INDEX_ON_THE_FLY);
    public static final EnumSet<Options> NO_OPTIONS = EnumSet.noneOf(Options.class);
    private static final OpenOption[] EMPTY_OPEN_OPTION_ARRAY = new OpenOption[0];
+    private final static Log log = Log.getInstance(VariantContextWriter.class);


incorrect modifier order

Suggested change

private final static Log log = Log.getInstance(VariantContextWriter.class);

private static final Log log = Log.getInstance(VariantContextWriter.class);

src/main/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilder.java

pshapiro4broad · 2019-03-25T17:57:53Z

src/main/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilder.java

-import htsjdk.samtools.util.IOUtil;
-import htsjdk.samtools.util.Md5CalculatingOutputStream;
-import htsjdk.samtools.util.RuntimeIOException;
+import htsjdk.samtools.util.*;


IMO it's better to avoid wildcard imports as it makes it harder to see what the code is doing when you're not using an IDE. You can change the IDE settings so it doesn't create wildcard imports.

yfarjoun · 2019-03-25T18:23:34Z

src/test/java/htsjdk/variant/variantcontext/writer/VariantContextWriterBuilderUnitTest.java

+      final Path vcfPath = IOUtil.getPath("/dev/null");
+      final Path vcfIdxPath = Tribble.indexPath(vcfPath);
+      // We explicitly setOutputFileType here to mimic gatk behavior where it pretends files that have no extension
+      try (final VariantContextWriter writer = builder.setOutputPath(vcfPath).setOutputFileType(OutputType.VCF_STREAM).build()) {


will this also work if you do not set the output type to a stream?

No it won't. In that case it will end up in the same place where we are as of right now, where an output stream location will end up "UNSPECIFIED" and throw an exception unless the user explicitly defines what type of output it is.

pshapiro4broad

Looks good!

…proach

codecov-io · 2019-06-05T16:22:03Z

Codecov Report

Merging #1328 into master will increase coverage by 0.17%.
The diff coverage is 40%.

@@              Coverage Diff               @@
##              master     #1328      +/-   ##
==============================================
+ Coverage     67.822%   67.992%   +0.17%     
- Complexity      8258      8344      +86     
==============================================
  Files            562       570       +8     
  Lines          33663     33826     +163     
  Branches        5644      5655      +11     
==============================================
+ Hits           22831     22999     +168     
+ Misses          8658      8645      -13     
- Partials        2174      2182       +8

Impacted Files	Coverage Δ	Complexity Δ
...antcontext/writer/VariantContextWriterBuilder.java	`80.625% <40%> (-2.067%)`	`59 <0> (-1)`
...s/cram/encoding/external/ExternalLongEncoding.java	`57.143% <0%> (-9.524%)`	`2% <0%> (-1%)`
src/main/java/htsjdk/variant/bcf2/BCFVersion.java	`88.889% <0%> (-5.848%)`	`11% <0%> (+4%)`
...amtools/reference/FastaReferenceWriterBuilder.java	`79.518% <0%> (-4.118%)`	`38% <0%> (+13%)`
...java/htsjdk/samtools/cram/structure/Container.java	`88.043% <0%> (-2.433%)`	`27% <0%> (+5%)`
...c/main/java/htsjdk/samtools/util/IntervalList.java	`72.18% <0%> (-2.329%)`	`64% <0%> (-11%)`
...samtools/cram/build/CramSpanContainerIterator.java	`71.053% <0%> (-2.118%)`	`9% <0%> (ø)`
...tsjdk/variant/variantcontext/writer/VCFWriter.java	`81.72% <0%> (-1.234%)`	`19% <0%> (+1%)`
...jdk/samtools/cram/build/CramContainerIterator.java	`88.889% <0%> (-1.111%)`	`10% <0%> (ø)`
src/main/java/htsjdk/samtools/util/Interval.java	`63.333% <0%> (-0.952%)`	`28% <0%> (+1%)`
... and 51 more

jamesemery added 2 commits March 15, 2019 15:43

downgraded exception to warning when index-on-the-fly is enabled for …

3b1f550

…stream outputs

Resolving a template for invokign streams in gatk

bd5ff18

pshapiro4broad reviewed Mar 25, 2019

View reviewed changes

yfarjoun reviewed Mar 25, 2019

View reviewed changes

jamesemery force-pushed the je_fixDevNullVcfs branch from 77438b3 to bd5ff18 Compare March 25, 2019 18:43

responded to comments

80d2c31

yfarjoun assigned pshapiro4broad Jun 5, 2019

pshapiro4broad approved these changes Jun 5, 2019

View reviewed changes

jamesemery added 2 commits June 5, 2019 11:57

removing the tests that are no longer relevant given the change in ap…

e4cf1d6

…proach

also updating a line

b20eb19

yfarjoun merged commit f13d075 into samtools:master Jun 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make VariantContextWriterBuilder warn when indexing-on-the-fly is enabled for stream output #1328

Make VariantContextWriterBuilder warn when indexing-on-the-fly is enabled for stream output #1328

jamesemery commented Mar 15, 2019

jamesemery commented Mar 15, 2019

pshapiro4broad left a comment

pshapiro4broad Mar 25, 2019

jamesemery Mar 25, 2019

pshapiro4broad Jun 5, 2019

pshapiro4broad Mar 25, 2019

pshapiro4broad Mar 25, 2019

pshapiro4broad Mar 25, 2019

yfarjoun Mar 25, 2019

jamesemery Mar 25, 2019

pshapiro4broad left a comment

codecov-io commented Jun 5, 2019 •

edited

Loading

	private final static Log log = Log.getInstance(VariantContextWriter.class);
	private static final Log log = Log.getInstance(VariantContextWriter.class);

Make VariantContextWriterBuilder warn when indexing-on-the-fly is enabled for stream output #1328

Make VariantContextWriterBuilder warn when indexing-on-the-fly is enabled for stream output #1328

Conversation

jamesemery commented Mar 15, 2019

jamesemery commented Mar 15, 2019

pshapiro4broad 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

pshapiro4broad left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 5, 2019 • edited Loading

Codecov Report

codecov-io commented Jun 5, 2019 •

edited

Loading