VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

cmnbroad · 2017-03-29T15:25:12Z

This is the first of 3 PRs for adding support for VCF4.3 and BCF 2.2; the first PR is mostly a refactoring of VCFHeader and the VCFHeaderLine class hierarchy in order to fix a bunch of bugs, and to provide the foundation for supporting new VCF versions.

There are a number of TODO comments embedded in these changes that are there either to:

underscore differences from prior behavior
ask a question about how some case should be handled
provide an explanation to help the reviewer understand the changes
note other changes that need to be fixed independent of this PR.

Fixes:

VCFHeader class design is unsafe #250 VCFHeader class design is unsafe
VCFHeader drops sequence dictionary attributes #730 VCFHeader drops sequence dictionary attributes
VCFHeader should not strip version lines #251 VCFHeader should not strip fileformat/version lines
VCFHeader class should allow multiple "other metadata" lines with the same key to be added #277 VCFHeader should allow multiple other metadata lines (with the same key)
VCF header lines get dropped by VCFHeader.addMetaDataLine() #500 VCFHeader lines dropped by addMetaDataLine()
VCF4.3 constants and header lines #575 (partial fix) VCF 4.3 constants and header lines (ALT, META, SAMPLE, PEDIGREE)
VCF4Parser.parseLine throws exception on optional Source and Version tags in INFO header #517 VCF4Parser.parse throws on Source and Version tags in INFO lines
VCFContigHeaderLine throwing Exception although the spec doesn't require it #389 VCFContigHeaderLine throws if no length tag is present

Changes to VCFHeader:

Always maintains a VCFHeaderVersion, and validates any header lines against the validation rules for that version.
Delegates management of the header line list to the VCFMetaDataLines class, which uses a single LinkedHashMap of header lines. The VCFMetaDataLines class is also used to merge headers.
No longer removes/restores file format lines from the header line list.

Changes to the VCFHeaderLine hierarchy:

All structured header lines classes maintain and round-trips all tags
All header lines are now immutable
All VCFHeaderLine classes have a validateForVersion method that validates the line against a particular version (this is called by VCFHeader when the line is added to a header, or when a header version is changed)
Replaced VCFSimpleHeaderLine/VCFIDHeaderLine with VCFStructuredHeaderLine, which is the base class for all “ID” header lines. Its no longer necessary to use “instanceof” tests to determine header line type. The VCFHeaderLine base class now has an “isStructuredHeaderLine” and a “getID” method; getID is valid iff isStructuredHeaderLine returns true.

VcfUtils.smartMergeHeaders delegates to VCFHeader, which in turn sometimes delegates to VCFHeaderLine classes to resolve conflicts when merging headers

Non-backward compatible API changes:

Removed VCFSimpleHeaderLine class, VCFIDHeaderLine interface
Removed the SupportedLineType enum from VCFCompoundHeaderLine
Removed public sameLineTypeAndName and equalExcludingDescription methods from VCFCompoundHeaderLine
Removed the VCFContigHeaderLine constructor that allowed the caller to specify a key
Also deprecated VCFUtils.makeContigHeaderLine

Notes: These changes have been run against the Picard and GATK test suites. All of the tests pass, though some require minor changes; these are mostly tests that do line-by-line comparisons against expected results files that have duplicate ID header lines. These now fail because the new implementation no longer retains/roundtrips the duplicate lines. One tool (LiftOverVcf) requires a minor code change because the version validation framework when it attempts to set a header version on a header that contains a conflicting file format line (which would previously have been stripped out by VCFHeader).

Outstanding question: There are several places where code nw either logs or throws on input that is not conformant for the version; was previously silently dropped; was previously silently retained when it shouldn't have been, or has correct behavior that differs from the previous implementation. For now, the throwing/logging is based on “strict reporting” mode setting that is controlled by a global variable. We need to determine the right level and mechanism for opt-out/opt-in should be for these cases.

codecov-io · 2017-03-29T15:37:13Z

Codecov Report

Merging #835 into master will increase coverage by 0.13%.
The diff coverage is 76.281%.

@@              Coverage Diff               @@
##              master      #835      +/-   ##
==============================================
+ Coverage     64.882%   65.011%   +0.13%     
- Complexity      7198      7308     +110     
==============================================
  Files            527       533       +6     
  Lines          31781     32056     +275     
  Branches        5424      5442      +18     
==============================================
+ Hits           20620     20840     +220     
- Misses          9016      9060      +44     
- Partials        2145      2156      +11

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/htsjdk/variant/bcf2/BCF2Codec.java	`74.847% <ø> (ø)`	`31 <0> (ø)`	⬇️
...c/main/java/htsjdk/variant/vcf/VCFRecordCodec.java	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
src/main/java/htsjdk/variant/bcf2/BCF2Utils.java	`71.845% <100%> (-2.194%)`	`47 <0> (-1)`
...n/java/htsjdk/variant/vcf/VCFFormatHeaderLine.java	`100% <100%> (+30%)`	`7 <6> (+3)`	⬆️
...ava/htsjdk/variant/vcf/VCFStandardHeaderLines.java	`76.389% <100%> (+0.333%)`	`11 <5> (ø)`	⬇️
src/main/java/htsjdk/variant/vcf/VCFConstants.java	`91.667% <100%> (+91.667%)`	`1 <0> (+1)`	⬆️
src/main/java/htsjdk/variant/vcf/VCFCodec.java	`23.077% <23.077%> (-42.832%)`	`5 <5> (-8)`
src/main/java/htsjdk/utils/Utils.java	`50% <50%> (ø)`	`3 <3> (?)`
src/main/java/htsjdk/variant/vcf/VCF3Codec.java	`40% <50%> (-13.659%)`	`4 <2> (-3)`
src/main/java/htsjdk/variant/vcf/VCFUtils.java	`11.905% <55.556%> (-12.801%)`	`4 <4> (-2)`
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a75d02...1efa91e. Read the comment docs.

…or VCF4.3/BCF2.2 and bug fixes.

cmnbroad · 2017-07-24T14:10:20Z

This branch also fixes #934.

magicDGS

Some comments (not in-depth reviewed)

magicDGS · 2017-07-24T14:22:11Z

src/main/java/htsjdk/variant/bcf2/BCF2Codec.java

@@ -150,6 +150,10 @@ public FeatureCodecHeader readHeader( final PositionalBufferedStream inputStream

            if ( bcfVersion.getMajorVersion() != ALLOWED_MAJOR_VERSION )
                error("BCF2Codec can only process BCF2 files, this file has major version " + bcfVersion.getMajorVersion());
+
+            // TODO: fixing this breaks GATK GenomicsDB integration/tests


I personally don't like that a block for fixing a bug is based on a downstream project. I will rather vote for fixing it here and fix the tests in GATK when it is updated.

Agreed. It needs to be fixed, but that will go into the next PR. This is one several places in this PR where I added notes like this for things like this.

magicDGS · 2017-07-24T14:26:51Z

src/main/java/htsjdk/variant/bcf2/BCF2Codec.java

@@ -206,6 +210,10 @@ public FeatureCodecHeader readHeader( final PositionalBufferedStream inputStream

    @Override
    public boolean canDecode( final String path ) {
+        // TODO: this is broken in a couple of ways:
+        // First, the version check is too permissive - it accepts any minor version, including BCF 2.2,


For the version check, I suggest to extract the checking code in readHeader to a private method returning an Optional<String> with the error message/code; if isPresent() returns true, cannot be decode. In the case of the readHeader(), if the optional is present it is thrown with the error message.

magicDGS · 2017-07-24T14:34:04Z

src/main/java/htsjdk/variant/bcf2/BCF2Codec.java

+        // TODO: this is broken in a couple of ways:
+        // First, the version check is too permissive - it accepts any minor version, including BCF 2.2,
+        // which it shouldn't. Second, it doesn't recognize that BCF can be block gzipped, so it rejects
+        // those files because the header never matches, but only because the stream isn't decompressed.


A block-gzipped file cannot be read by using any of the codecs implemented in htsjdk unless it has a gz or bgz extension, even if this method returns true for them. Maybe AbstractFeatureReader should use BlockCompressedInputStream.isValidFile() to open properly this kind of files and not base the code on the extension...But I guess that it is part of other PR.

magicDGS · 2017-07-24T14:37:30Z

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

@@ -141,7 +146,7 @@ public void writeHeader(final VCFHeader header) {
    }

    public static String getVersionLine() {
-        return VERSION_LINE;
+        return DEFAULT_VERSION_LINE;
    }

    public static VCFHeader writeHeader(VCFHeader header,


Because you are already here, what's about adding the writeHeader method to the encoder to have a better encapsulation? This will fit the encoder interface that I'm working in my tribble-writing PR (#822)...

There are already a lot of changes in this PR, so I'd prefer to keep this focused for now.
(I like the the general idea of #822, but that one also will need some discussion).

magicDGS · 2017-07-24T14:39:58Z

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

+                }
+            }
+            if (VCFUtils.getVerboseVCFLogging()) {
+                logger.warn(message);


This warning does not says anything about the compatibility checked in the previous if clause. Maybe it will be nice to add this to the warning to do not scare users writing compatible versions...

Not quite sure I get what you're saying. Are you saying to only log this message if the versions are incompatible ?

I mean that the message could include information about the compatibility...

magicDGS · 2017-07-24T16:26:16Z

src/main/java/htsjdk/variant/vcf/VCFHeaderLineCount.java

 public enum VCFHeaderLineCount {
    INTEGER, A, R, G, UNBOUNDED;
+
+    // A default int value used to represent an integral count value (not a count *type*) when the
+    // actual count is derived and not a fixed integer (i.e., when isFixedCount()==false)


I will make this a javadoc and not a comment, because it is public.

Yes, good point.

magicDGS · 2017-07-24T16:36:09Z

src/main/java/htsjdk/variant/vcf/VCFMetaDataLines.java

+import java.util.*;
+import java.util.stream.Collectors;
+
+//TODO: should this class be public so consumers can use in place of Set<VCFHeaderLine>


Is any other change planned for the next PRs? If not, maybe it is worthy to make it public in this PR to make people familiar with the API from the beginning...

magicDGS · 2017-07-24T16:37:03Z

src/main/java/htsjdk/variant/vcf/VCFMetaDataLines.java

+    // Allow duplicate non-ID keys, unless fileformat, reference ?
+
+    // TODO: Should we reject attempts to add two contig header lines with the same contigIndex ?
+    // TODO: GATK VcfUtilsUnitTest.createHeaderLines test creates headers with contig lines with identical (0) indices


Again, I think that it is not a good idea to block htsjdk development for failing tests in downstream projects...

Again, these are just notes for myself, and for discussion. The GATK comment is only there to reinforce the idea that this would be a breaking change.

magicDGS · 2017-07-24T16:41:43Z

src/main/java/htsjdk/variant/vcf/VCFMetaDataLines.java

+                // TODO: duplicate ID lines, which will have to be modified in order for the tests to pass.
+                //
+                // TODO: Is it sufficient to log a warning for this case (duplicate key) ? Should we add code
+                // TODO: here to throw only for v4.3+ ? Only if VCFUtils.getStrictVCFVersionValidation is set ? Or both ?


After reading through the whole PR, I start to think that VCF requires a ValidationStringency enum similar to SAM/BAM parsing to use in different parts, with STRICT value by default (maybe SILENT until the 3 PRs are in to keep the actual behaviour).

magicDGS · 2017-07-24T16:44:03Z

src/main/java/htsjdk/variant/vcf/VCFMetaDataLines.java

+
+    //TODO: Is this useful ? It returns the first match for the given key, no matter how many
+    //TODO: there are. Should we deprecate it (and add a new one that returns a collection)
+    //TODO: or just change this one to return a collection ?


I vote for a breaking change to return a Collection, because anyway this PR will introduce lots of backwards incompatible changes...

yfarjoun · 2019-02-04T19:29:40Z

How about we close this PR and move the code to htsjdk3?

yfarjoun · 2019-03-25T18:45:03Z

Closing due complexity.

author will work with htsjdk-next-beta to include these great features.

cmnbroad assigned droazen Mar 29, 2017

cmnbroad force-pushed the cn_vcf_43 branch 5 times, most recently from 7cf14e6 to 9408a7a Compare April 6, 2017 19:45

VCFHeader, VCFCodec and VCFHeaderLine refactoring to enable support f…

1efa91e

…or VCF4.3/BCF2.2 and bug fixes.

cmnbroad force-pushed the cn_vcf_43 branch from 9408a7a to 1efa91e Compare April 9, 2017 14:21

This was referenced Apr 14, 2017

Redesign HTSJDK3 vcf package to support codec/header versioning broadinstitute/gatk#2601

Open

picard SortVcf and MergeVcfs: do not reorder INFO column contens and header rows broadinstitute/picard#760

Open

cmnbroad mentioned this pull request Jul 7, 2017

equals in VCFInfoHeaderLine is using description.equals(other.description) ? #929

Open

cmnbroad mentioned this pull request Jul 24, 2017

Performance issues with VCF header parsing #934

Closed

magicDGS reviewed Jul 24, 2017

View reviewed changes

cmnbroad mentioned this pull request Aug 16, 2017

Fix performance issue when creating a VCF header with a large number of contigs #968

Merged

5 tasks

cmnbroad mentioned this pull request Nov 15, 2017

GenomicsDB outputs AD header line that doesn't match HTSJDK broadinstitute/gatk#3839

Closed

cmnbroad mentioned this pull request Jan 24, 2018

SelectVariants appears to produce some incongruities in header lines broadinstitute/gatk#4252

Open

cmnbroad mentioned this pull request Mar 4, 2019

Maintain reference md5s from bam to gvcf broadinstitute/gatk#5746

Open

yfarjoun closed this Mar 25, 2019

brainstorm mentioned this pull request Jul 11, 2019

VCF4.2 PEDIGREE header parsing broken #1400

Closed

mjhipp mentioned this pull request Jan 28, 2021

Allow multiple VCFSimpleHeaderLine of same type #1531

Closed

5 tasks

michaelgatzen mentioned this pull request Feb 4, 2021

Fixed SelectVariants generating duplicate header lines broadinstitute/gatk#7069

Merged

daviesrob mentioned this pull request Apr 27, 2021

VCF v4.3 ##META header line syntax samtools/hts-specs#558

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

cmnbroad commented Mar 29, 2017

codecov-io commented Mar 29, 2017 •

edited

Loading

cmnbroad commented Jul 24, 2017

magicDGS left a comment

magicDGS Jul 24, 2017

cmnbroad Jul 26, 2017

magicDGS Jul 24, 2017

magicDGS Jul 24, 2017

magicDGS Jul 24, 2017

cmnbroad Jul 26, 2017

magicDGS Jul 24, 2017

cmnbroad Jul 26, 2017

magicDGS Jul 26, 2017

magicDGS Jul 24, 2017

cmnbroad Jul 26, 2017

magicDGS Jul 24, 2017

magicDGS Jul 24, 2017

cmnbroad Jul 26, 2017

magicDGS Jul 24, 2017

magicDGS Jul 24, 2017

yfarjoun commented Feb 4, 2019

yfarjoun commented Mar 25, 2019

VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

VCFHeader, VCFCodec and VCFHeaderLine refactoring for VCF4.3/BCF2.2 #835

Conversation

cmnbroad commented Mar 29, 2017

codecov-io commented Mar 29, 2017 • edited Loading

Codecov Report

cmnbroad commented Jul 24, 2017

magicDGS 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Feb 4, 2019

yfarjoun commented Mar 25, 2019

codecov-io commented Mar 29, 2017 •

edited

Loading