Move to GenomicsDB 1.2.0 #6305

nalinigans · 2019-12-07T03:55:11Z

Move to GenomicsDB 1.2.0. This version uses a 64-bit htslib to workaround overflow issues when computed annotation sizes exceed the 32-bit integer space as requested by @ldgauthier.

The htslib and GenomicsDB fixes by @kgururaj also enable read/write of VCFs where the values of INFO fields can be 64-bits. But, if the input VCF (produced by HaplotypeCaller) contains INFO fields with values larger than what can fit in 32 bits, the vid must list those fields as type int64 (currently, it will be int by default). However, no changes are needed in the vid if the input values fit within 32 bits but the result of the combine operation [e.g. sum] is larger.

Note that 64-bit integers are currently only supported for INFO fields (no support for FORMAT fields).

…flow issues with computed annotation sizes outside the 32-bit integer space.

kgururaj · 2019-12-09T13:49:26Z

I believe this release also fixes #6158 and #6275 (thanks @mlathara).

nalinigans · 2019-12-09T17:20:45Z

GenomicsDB 1.2.0 also supports suppression of "No valid combination operation" warnings - see #2689 . This will however require changes in gatk(maybe hardcoded initially via GenomicsDBUtils) to indicate to GenomicsDB that a set of fields have no combinations. I will open another PR with an example.

cmnbroad · 2019-12-09T17:35:52Z

src/main/java/org/broadinstitute/hellbender/engine/FeatureDataSource.java

@@ -396,7 +397,7 @@ final void printCacheStats() {
            final GenomicsDBExportConfiguration.ExportConfiguration exportConfigurationBuilder =
                    createExportConfiguration(workspace, callsetJson, vidmapJson, vcfHeader, genomicsDBOptions);
            return new GenomicsDBFeatureReader<>(exportConfigurationBuilder,
-                    new GenomicsDBBCFCodec(),
+                    new VCFCodec(),


Wondering about the performance implications of changing to VCF format ? Also, if we no longer need the GenomicsDBBCFCodec codec, that class (and it's tests) can be deleted.

Also, there are some other tests where GenomicsDBBCFCodec is still used (GenomicsDBImportIntegrationTest) - I'm surprised those don't seem to be failing.

GenomicsDBImportIntegrationTest has been ported already to use VCFCodec in the change set for this PR. But, I can parameterize the tests to test both BCFCodec and VCFCodec if we decide to keep BCFCodec for performance reasons.

Regarding performance, we will have to benchmark GenomicsDB queries as @droazen mentions. We can probably add a FeatureReader option to choose between BCFCodec and VCFCodec for GenomicsDB if performance turns out to be an issue. If BCFCodec is chosen, GenomicsDB gracefully handles a number of overflow scenarios already.

Note that -

BCF and BCFCodec do not support 64-bit fields.

VCFCodec does not fully decode the VCF streams by default, it uses the LazyParser whereas BCFCodec always fully decodes the BCF streams. Hopefully, this keeps the VCFCodec somewhat performant even if fields have to be explicitly decoded.

GenomicsDBImport already uses VCFCodec to read vcfs, so no change there.

Ah, ok, I see. So then we just need to delete the GenomicsDBBCFCodec class (unless we keep an option for BCF, in which case it will depend on the version of BCF that uses). The GenomicsDBBCFCodec is only necessary if the BCF header is stamped with version 2.2.

I did a benchmark run with about 5K samples more than a year back - negligible difference in performance between VCF and BCF serialization/deseralization. It would be good to re-check again with your data.

droazen · 2019-12-09T17:38:39Z

src/main/java/org/broadinstitute/hellbender/engine/FeatureDataSource.java

@@ -396,7 +397,7 @@ final void printCacheStats() {
            final GenomicsDBExportConfiguration.ExportConfiguration exportConfigurationBuilder =
                    createExportConfiguration(workspace, callsetJson, vidmapJson, vcfHeader, genomicsDBOptions);
            return new GenomicsDBFeatureReader<>(exportConfigurationBuilder,
-                    new GenomicsDBBCFCodec(),
+                    new VCFCodec(),


As discussed in person, we should benchmark this branch to make sure that the switch to VCFCodec did not cause a major performance regression.

nalinigans · 2019-12-12T18:42:59Z

@ldgauthier, does this branch fix the overflow with combined annotation for RAW_MQandDP in the GenomicsDB you encountered for the large cohort? Thanks.

ldgauthier · 2019-12-12T20:55:57Z

@nalinigans I can test on my use case specifically, but it's going to take me some time to dig up all the data again.

… default codec will be BCF2Codec. Also, support using max-genotype-counts/max-alternate-alleles arg options from GenotypeVCFs/GnarlyGenotyper to be passed to GenomicsDB - see #6377

nalinigans · 2020-02-07T18:16:01Z

@droazen, I have introduced --genomicsdb-use-vcf-codec argument defaulting to false. This will allow the user to choose between VCFCodec and BCF2Codec for GenomicsDB streams.

--genomicsdb-use-vcf-codec:Boolean
                              Use VCF Codec Streaming for data from GenomicsDB  Default value: true. Possible values:
                              {true, false}

Also, hooked --max-genotype-count arg from GenotypeGVCFs/GnarlyGenotyper to GenomicsDB Export Configuration and introduced --genomicsdb-max-genotype-count to be used by tools like SelectVariants that do not use the Genotype ArgumentCollection. This will help with Issue #6275.

--genomicsdb-max-genotype-count:Integer
                              Maximum number of genotypes to consider at any site  Default value: 1024.

nalinigans · 2020-02-07T18:22:47Z

@ldgauthier, you mentioned that it would take some time for you to dig into this. But, does this branch fix the overflow with combined annotation for RAW_MQandDP in the GenomicsDB you encountered for the large cohort? Please note that you have to explicitly set the useVCFCodec arg/option to true to test.

cmnbroad

This is a WIP - submitting partial comments.

cmnbroad · 2020-02-07T19:37:04Z

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBArgumentCollection.java

+import org.broadinstitute.barclay.argparser.Argument;
+import org.broadinstitute.hellbender.tools.walkers.genotyper.GenotypeCalculationArgumentCollection;
+
+import java.io.Serializable;


Dangling unused import..

Fixed this.

cmnbroad · 2020-02-07T19:37:18Z

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBArgumentCollection.java

@@ -0,0 +1,18 @@
+package org.broadinstitute.hellbender.tools.genomicsdb;
+
+import htsjdk.variant.variantcontext.GenotypeLikelihoods;


This is unused.

Fixed this.

cmnbroad · 2020-02-07T19:44:41Z

...in/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBBaseArgumentCollection.java

+      fullName = USE_VCF_CODEC_LONG_NAME,
+      doc = "Use VCF Codec Streaming for data from GenomicsDB",
+      optional = true)
+  public boolean useVCFCodec = DEFAULT_USE_VCF_CODEC;


We generally try to have boolean command line arguments default to false, with true being used to opt in to non-default behavior. If we're going to default to VCF, then this arg should probably be "genomicsdb-use-bcf-codec" with default of false.

Actually, we need the default be BCF as we are not sure about the performance implications of VCF as yet. Changed the DEFAULT_USE_VCF_CODEC to false.

Ok. But it looks like the GenomicsDBImportIntegrationTest tests are still using the VCF codec, which now isn't the one used by default. If we're going to keep BCF as the default, those tests should probably be reverted back to BCF, and there really should be at least one new test that uses the opt-in VCF codec, preferably with 64 bit values.

Have changed the GenomicsDB/GnarlyGenotyper tests to run with both codecs. But, I don't have the bandwidth to introduce new 64-bit VCFs to test VCF Codec. We do have tests in GenomicsDB for the 64 bit VCFs - see GenomicsDB PR and vcfs like bigint0.vcf.gz for the GenomicsDB CI tests and they can be ported to work with GATK test references. I can look into this later or perhaps it is easy enough for someone else from the team to do it.

cmnbroad · 2020-02-07T19:45:14Z

...in/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBBaseArgumentCollection.java

+  @Advanced
+  @Argument(
+      fullName = USE_VCF_CODEC_LONG_NAME,
+      doc = "Use VCF Codec Streaming for data from GenomicsDB",


This should state what the alternative is (that is toggles between vcf/bcf) for the internal interchange format.

cmnbroad · 2020-02-07T19:50:16Z

...in/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBBaseArgumentCollection.java

+  /**
+   * Currently there is no support for 64-bit fields in BCFCodec. Specifying this option will allow
+   * for 64-bit width positions and INFO fields and for computed annotation sizes to exceed the 32-bit
+   * integer space while encoding/decoding with GenomicsDB.


See my comments below; if we reverse the toggle polarity as I suggest, we should update this to reflect the reason for opting in to BCF (though I've lost track of what that is - is it just that we're just keeping it as a failsafe in case there is a perf issue ?).

cmnbroad · 2020-02-07T21:58:22Z

...in/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBBaseArgumentCollection.java

+
+import java.io.Serializable;
+
+public class GenomicsDBBaseArgumentCollection implements Serializable {


Is there a better name we can use to make the logical distinction between the the two arg collections clearer ?

cmnbroad · 2020-02-07T22:03:58Z

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBOptions.java

+        this.callGenotypes = genomicsdbArgs.callGenotypes;
+        this.maxDiploidAltAllelesThatCanBeGenotyped = genomicsdbArgs.maxDiploidAltAllelesThatCanBeGenotyped;
+        this.maxGenotypeCount = genotypeCalcArgs.MAX_GENOTYPE_COUNT;
+        this.useVCFCodec = genomicsdbArgs.useVCFCodec;


Rather than having constructor overloads like this, add getters to argument collection classes (its basically the public interface of GenomicsDBOptions, minus the reference). Then the default argument values could be defined in one place (whichever collection winds up being the "base" collection class), and some of these aliases and constructor overloads could be eliminated.

@nalinigans This suggestion can be deferred and made in a subsequent PR if you'd prefer.

Yes, let's defer this to another PR. Thanks.

cmnbroad · 2020-02-07T23:14:20Z

...est/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImportIntegrationTest.java

+     * Genotype.getAnyAttribute() returns String or Integer based on the VC being decoded fully, so use this routine instead to
+     * guarantee that it is an int.
+     */
+    private static int getAttributeAsInt(Genotype genotype, String key) {


You should be able to use VariantContextGetters. getAttributeAsInt() instead of adding this.

cmnbroad · 2020-02-07T23:20:55Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/GnarlyGenotyperIntegrationTest.java

+     * @param v2 List of expected variant contexts
+     */
+    private static void assertEqualVariants(
+            final List<VariantContext> v1, final List<VariantContext> v2) {


Pleas rename to actualVariants and expectedVariants.

Also, might as well put this right in VariantContextTestUtils adjacent to the method its calling.

cmnbroad · 2020-02-07T23:21:44Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/GnarlyGenotyperIntegrationTest.java

+        Utils.nonNull(v2, "v2");
+        Assert.assertEquals(v1.size(), v2.size());
+        for (int i = 0; i < v1.size(); i++) {
+            VariantContextTestUtils.assertVariantContextsAreEqual(v1.get(i), v2.get(i), new ArrayList<String>(), new ArrayList<String>());


Can use the diamond operator for these empty lists, or better yet, just use Collections.emptyList().

Fixed using Collections.emptyList(). Thanks.

…tring>

cmnbroad

@nalinigans A few more comments and questions. I may not be understanding where some of these changes are headed, so please let me know if I'm misunderstanding.

cmnbroad · 2020-02-11T14:31:58Z

...in/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBBaseArgumentCollection.java

+      fullName = USE_VCF_CODEC_LONG_NAME,
+      doc = "Use VCF Codec Streaming for data from GenomicsDB",
+      optional = true)
+  public boolean useVCFCodec = DEFAULT_USE_VCF_CODEC;


Ok. But it looks like the GenomicsDBImportIntegrationTest tests are still using the VCF codec, which now isn't the one used by default. If we're going to keep BCF as the default, those tests should probably be reverted back to BCF, and there really should be at least one new test that uses the opt-in VCF codec, preferably with 64 bit values.

cmnbroad · 2020-02-11T14:33:43Z

.../java/org/broadinstitute/hellbender/tools/walkers/gnarlyGenotyper/GnarlyGenotyperEngine.java

+   * @param g genotype for a sample
+   * @return int array for the given genotype
+   */
+  private static int[] getSBFieldAsIntArray(Genotype g) {


It looks like there is already code that does all this in StrandBiasTest.getStrandCounts.

I just refactored the existing code with existing patterns of throwing exceptions in GnarlyGenotyper. I will leave to @ldgauthier to clean this up further. Thanks.

cmnbroad · 2020-02-11T15:04:06Z

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

@@ -420,6 +423,9 @@
            doc="Suppress reference path in output for test result differencing")
    private boolean suppressReferencePath = false;

+    @ArgumentCollection
+    private GenomicsDBArgumentCollection genomicsdbArgs = new GenomicsDBArgumentCollection();


Has any thought been given to how this new argument interacts with the existing genotype subsetting arguments SelectVariants already has ? If not, I'm thinking this change will likely cause confusion, and that we should take this out until those are resolved and some tests are added.

There is no existing argument that clashes with the existing subsetting arguments. However, if you would rather take this out, please let me know.

SV has several arguments that do various kinds of genotype subsetting/downsampling/filtering. The argument is marked as @Advanced, so maybe its ok. But if this argument does what I think it does, it may not be obvious to a user that this subsetting is done independently before SV ever sees the data.

This needs more thought. Have removed all support for GenomicsDB args in SelectVariants for now and have filed #6456 to generally get the tools hook up to GenomicsDBExportConfiguration better.

cmnbroad · 2020-02-11T15:32:08Z

src/main/java/org/broadinstitute/hellbender/tools/walkers/gnarlyGenotyper/GnarlyGenotyper.java

@@ -169,7 +172,9 @@ public boolean requiresReference() {
    @Override
    protected GenomicsDBOptions getGenomicsDBOptions() {
        if (genomicsDBOptions == null) {
-            genomicsDBOptions = new GenomicsDBOptions(referenceArguments.getReferencePath(), CALL_GENOTYPES, PIPELINE_MAX_ALT_COUNT);
+            genomicsdbArgs.callGenotypes = CALL_GENOTYPES;
+            genomicsdbArgs.maxDiploidAltAllelesThatCanBeGenotyped = PIPELINE_MAX_ALT_COUNT;


Its a bit strange to mutate values in an argument collection like this (I know they aren't annotated as @Argument, but presumably someday they will be exposed ?). Can you verify that the only net change here in this part of the PR is that we're exposing the vcfCodec switch to GnarlyGenotyper (?).

Yes, the net change here allows GnarlyGenotyper to access the vcfCodec arg. But, have also hooked --max-genotype-count arg from GenotypeGVCFs/GnarlyGenotyper to be passed to GenomicsDBOptions to help with Issue #6275.

…is the default

…ange

nalinigans · 2020-02-13T18:37:04Z

I have merged to the latest master, but Travis builds are still failing not related to the changes in this PR. @droazen, @cmnbroad, can someone take a look and let me know what should be done? Thanks.

cmnbroad · 2020-02-13T20:58:28Z

@nalinigans thanks for the updates. All builds are failing for everyone at the moment, so its not just this branch. We're working on a fix. I'll take a look at your changes.

cmnbroad

@nalinigans Thanks for the changes. A couple more minor comments (also, is it possible to add at least one test, maybe to Select Variants, for the --genomicsdb-max-genotype-count arg, which is exposed in several tools now ?) Also, build is fixed now, so if you rebase on master the tests should pass again.

cmnbroad · 2020-02-14T21:15:48Z

src/main/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBArgumentCollection.java

+  private static final String MAX_GENOTYPE_COUNT_LONG_NAME = "genomicsdb-max-genotype-count";
+
+  @Advanced
+  @Argument(fullName=MAX_GENOTYPE_COUNT_LONG_NAME, doc="Maximum number of genotypes to consider at any site", optional=true)


Since this is only used outside outside of the genotyping tools, I think it would make sense to remove the word "consider" from the doc string, and also reinforce that this only applies when the underlying store is GenomicsDB. Maybe something like "Maximum number of genotypes to retrieve when the underlying store is GenomicsDB".

This argument was being used only by SelectVariants. Note that the existing GenomicsDBArgumentCollection is removed and GenomicsDBBaseArgumentCollection is renamed to GenomicsDBArgumentCollection.

cmnbroad · 2020-02-14T21:24:50Z

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

@@ -420,6 +423,9 @@
            doc="Suppress reference path in output for test result differencing")
    private boolean suppressReferencePath = false;

+    @ArgumentCollection
+    private GenomicsDBArgumentCollection genomicsdbArgs = new GenomicsDBArgumentCollection();


SV has several arguments that do various kinds of genotype subsetting/downsampling/filtering. The argument is marked as @Advanced, so maybe its ok. But if this argument does what I think it does, it may not be obvious to a user that this subsetting is done independently before SV ever sees the data.

...est/java/org/broadinstitute/hellbender/tools/genomicsdb/GenomicsDBImportIntegrationTest.java

…ndle this in a separate PR

cmnbroad

@droazen you're on the reviewer list too, so not sure if you want to review as well before merge, but I'm approving this as is so we can get this in the next release.

lbergelson

👍

Move to GenomicsDB 1.2.0 that uses a 64-bit htslib to workaround over…

db054ab

…flow issues with computed annotation sizes outside the 32-bit integer space.

kgururaj added the GenomicsDB label Dec 9, 2019

cmnbroad reviewed Dec 9, 2019

View reviewed changes

droazen reviewed Dec 9, 2019

View reviewed changes

droazen assigned cmnbroad Dec 13, 2019

nalinigans mentioned this pull request Jan 12, 2020

GenomicsDBImport not completing for mixed ploidy samples #6275

Closed

nalinigans and others added 3 commits February 1, 2020 12:26

Merge branch 'master' into genomicsdb_120_1

190943e

Support a use VCFCodec argument option for tools with GenomicsDB. The…

136e4c5

… default codec will be BCF2Codec. Also, support using max-genotype-counts/max-alternate-alleles arg options from GenotypeVCFs/GnarlyGenotyper to be passed to GenomicsDB - see #6377

Revert to using MaxDiploidAltAllele for GenomicsDBOptions

c71efcb

nalinigans requested review from droazen and cmnbroad February 7, 2020 18:19

cmnbroad reviewed Feb 10, 2020

View reviewed changes

nalinigans added 4 commits February 10, 2020 11:21

Merge wth master

cb05016

Fixes based on review comments

17dfd3f

Fixes based on review comments

8a35a29

Fixes based on review comments - change Collections to Collections.<S…

6029c5a

…tring>

cmnbroad requested changes Feb 11, 2020

View reviewed changes

nalinigans added 3 commits February 11, 2020 10:11

Revert GenomicsDBImportIntegrationTest.java to use BCF2Codec as that …

09da199

…is the default

Test GenomicsDB export using vcf and bcf2 codecs for internal interch…

b4a86cf

…ange

Merge branch 'master' into genomicsdb_120_1

2a97f85

nalinigans requested a review from cmnbroad February 13, 2020 18:45

cmnbroad reviewed Feb 14, 2020

View reviewed changes

nalinigans added 2 commits February 14, 2020 16:32

Remove support GenomicsDBArgumentCollection for SelectVariants and ha…

1008b24

…ndle this in a separate PR

Merge branch 'master' into genomicsdb_120_1

b5f774f

nalinigans requested a review from cmnbroad February 15, 2020 04:25

cmnbroad approved these changes Feb 20, 2020

View reviewed changes

lbergelson approved these changes Feb 28, 2020

View reviewed changes

droazen merged commit 7f9b849 into master Feb 28, 2020

droazen deleted the genomicsdb_120_1 branch February 28, 2020 18:01

nalinigans mentioned this pull request Mar 25, 2020

Example code to mark fields that do not have a combination operation #6514

Open

cmnbroad mentioned this pull request May 18, 2020

Subclass BCFCodec for use when reading from GenomicsDB #5839

Closed

		@@ -0,0 +1,18 @@
		package org.broadinstitute.hellbender.tools.genomicsdb;

		import htsjdk.variant.variantcontext.GenotypeLikelihoods;


		import java.io.Serializable;

		public class GenomicsDBBaseArgumentCollection implements Serializable {

Move to GenomicsDB 1.2.0 #6305

Move to GenomicsDB 1.2.0 #6305

Conversation

nalinigans commented Dec 7, 2019 • edited Loading

kgururaj commented Dec 9, 2019

nalinigans commented Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

cmnbroad Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

nalinigans Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

cmnbroad Dec 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalinigans commented Dec 12, 2019

ldgauthier commented Dec 12, 2019

nalinigans commented Feb 7, 2020 • edited Loading

nalinigans commented Feb 7, 2020

cmnbroad 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

nalinigans Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalinigans Feb 13, 2020 • edited Loading

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

cmnbroad Feb 10, 2020 • edited Loading

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

cmnbroad 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

nalinigans commented Feb 13, 2020 • edited Loading

cmnbroad commented Feb 13, 2020

cmnbroad 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

cmnbroad left a comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

nalinigans commented Dec 7, 2019 •

edited

Loading

nalinigans commented Dec 9, 2019 •

edited

Loading

cmnbroad Dec 9, 2019 •

edited

Loading

nalinigans Dec 9, 2019 •

edited

Loading

cmnbroad Dec 9, 2019 •

edited

Loading

nalinigans commented Feb 7, 2020 •

edited

Loading

nalinigans Feb 10, 2020 •

edited

Loading

nalinigans Feb 13, 2020 •

edited

Loading

cmnbroad Feb 10, 2020 •

edited

Loading

nalinigans commented Feb 13, 2020 •

edited

Loading