Bug with passing max_alternate_alleles to GenomicsDB #7576
Conversation
There was a problem hiding this comment.
@nalinigans I have a few comments for you and also some vague complaints shouted into the void.
| this.useBCFCodec = genomicsdbArgs.useBCFCodec; | ||
| this.sharedPosixFSOptimizations = genomicsdbArgs.sharedPosixFSOptimizations; | ||
| this.useGcsHdfsConnector = genomicsdbArgs.useGcsHdfsConnector; | ||
| if (genotypeCalcArgs != null) { | ||
| this.maxDiploidAltAllelesThatCanBeGenotyped = genotypeCalcArgs.MAX_ALTERNATE_ALLELES; |
There was a problem hiding this comment.
This isn't your doing, but it's super confusing that this is in ALL_CAPS because it made me assume it was a constant and not the value that was set.
| @@ -25,18 +25,24 @@ public GenomicsDBOptions(final Path reference) { | |||
| } | |||
|
|
|||
| public GenomicsDBOptions(final Path reference, GenomicsDBArgumentCollection genomicsdbArgs) { | |||
| this(reference, genomicsdbArgs, new GenotypeCalculationArgumentCollection(), false); | |||
| this(reference, genomicsdbArgs, new GenotypeCalculationArgumentCollection()); | |||
| } | |||
|
|
|||
| public GenomicsDBOptions(final Path reference, final GenomicsDBArgumentCollection genomicsdbArgs, | |||
There was a problem hiding this comment.
Bleh, GenomicsDBOptions and GenomicsDBArgumentCollection are confusingly similar. We should probably revisit that at some point.
There was a problem hiding this comment.
Yes, we should. As of now, GenomicsDBOptions is a combination of GenomicsDBArgumentCollection and GenotypeCalculationArgumentCollection.
| @@ -173,8 +173,9 @@ public boolean requiresReference() { | |||
| @Override | |||
| protected GenomicsDBOptions getGenomicsDBOptions() { | |||
| if (genomicsDBOptions == null) { | |||
| genomicsdbArgs.maxDiploidAltAllelesThatCanBeGenotyped = PIPELINE_MAX_ALT_COUNT; | |||
| genomicsDBOptions = new GenomicsDBOptions(referenceArguments.getReferencePath(), genomicsdbArgs, genotypeArgs, CALL_GENOTYPES); | |||
| genomicsdbArgs.callGenotypes = CALL_GENOTYPES; | |||
There was a problem hiding this comment.
I think this is a minor bug. Previously I think it would force call if either --call-genotypes was set OR CALL_GENOTYPES was true. Now it only takes CALL_GENOTYPES into account.
There was a problem hiding this comment.
Since CALL_GENOTYPES is hardcoded to true, I wanted to explicitly override the arguments and be consistent with what is done for maxDiploidAltAllelesThatCanBeGenotyped.
| try { | ||
| File output = runGenotypeGVCFS(genomicsDBUri, expected, args, reference); | ||
| Assert.assertTrue(output.exists()); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Can we catch a specific exception here instead of anything, otherwise it's possible the test is just passing because of a misspelled argument or something.
| args.add("2"); // Too small max_alternate_alleles arg to GenomicsDB, should fail | ||
| try { | ||
| File output = runGenotypeGVCFS(genomicsDBUri, expected, args, reference); | ||
| Assert.assertTrue(output.exists()); |
There was a problem hiding this comment.
If we're counting on this to fail instead of assertTrue(output.exists()) we should probably have a Assert.fail("Should have thrown an exception")
This was unearthed by #7542 and is plumbed correctly in this PR.
Note that we need to still address the broader issue of hooking arguments to GenomicsDB - #6456 . GenomicsDB exposes a whole set of export arguments all added in response of gatk requests, some of them are hardcoded by certain tools(e.g GenotypeGVCFs uses --max-alternate-alleles while SelectVariants does not), many are unused.