PrintBGZFBlockInformation: a tool to dump information about blocks in a BGZF file #4239

droazen · 2018-01-23T21:51:06Z

No description provided.

droazen · 2018-01-23T21:51:44Z

@jamesemery please review (after using the tool to help diagnose #4224)

codecov-io · 2018-01-25T18:46:31Z

Codecov Report

Merging #4239 into master will increase coverage by 0.004%.
The diff coverage is 84%.

@@               Coverage Diff               @@
##              master     #4239       +/-   ##
===============================================
+ Coverage     87.048%   87.052%   +0.004%     
- Complexity     31445     31479       +34     
===============================================
  Files           1921      1923        +2     
  Lines         144977    145146      +169     
  Branches       16062     16081       +19     
===============================================
+ Hits          126199    126352      +153     
- Misses         12935     12944        +9     
- Partials        5843      5850        +7

Impacted Files	Coverage Δ	Complexity Δ
...te/hellbender/tools/PrintBGZFBlockInformation.java	`79.798% <79.798%> (ø)`	`21 <21> (?)`
...ools/PrintBGZFBlockInformationIntegrationTest.java	`92.157% <92.157%> (ø)`	`7 <7> (?)`
...institute/hellbender/engine/VariantWalkerBase.java	`100% <0%> (ø)`	`14% <0%> (+1%)`	⬆️
...ender/utils/haplotype/HaplotypeBAMDestination.java	`100% <0%> (ø)`	`4% <0%> (ø)`	⬇️
...roadinstitute/hellbender/engine/VariantWalker.java	`93.333% <0%> (ø)`	`12% <0%> (ø)`	⬇️
...nstitute/hellbender/engine/VariantLocusWalker.java	`89.091% <0%> (ø)`	`17% <0%> (ø)`	⬇️
...nstitute/hellbender/engine/MultiVariantWalker.java	`100% <0%> (ø)`	`16% <0%> (ø)`	⬇️
...aplotypecaller/HaplotypeCallerIntegrationTest.java	`88.262% <0%> (+0.027%)`	`84% <0%> (ø)`	⬇️
... and 6 more

jamesemery

Mostly minor changes, I might insist though that for this to be a useful tool in diagnosing problems with (valid).vcf.gz files which have null blocks before the final block that there should at least be a check for this functionality, as the code looks like it would stop reading blocks after that point.

Also probably making a better error message for empty file

jamesemery · 2018-01-24T15:57:20Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+    @Argument(fullName = "bgzf-file", doc = "The BGZF-format file for which to print block information", optional = false)
+    private String bgzfPathString;
+
+    @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME, shortName = StandardArgumentDefinitions.OUTPUT_SHORT_NAME, doc = "File to which to write block information (if not specified, prints to standard output", optional = true)


missing parenthesis

jamesemery · 2018-01-24T16:00:43Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+
+        if ( ! AbstractFeatureReader.hasBlockCompressedExtension(bgzfPathString) ) {
+            throw new UserException("File " + bgzfPathString + " does not end in a recognized BGZF file extension (" +
+                    AbstractFeatureReader.BLOCK_COMPRESSED_EXTENSIONS + ")");


minor nit, but you should probably use a string joiner on this set as I suspect it will mangle the formatting.

Maybe a UserException.BadInput instead to be more specific?

jamesemery · 2018-01-24T18:46:29Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+    @Argument(fullName = "bgzf-file", doc = "The BGZF-format file for which to print block information", optional = false)
+    private String bgzfPathString;
+
+    @Argument(fullName = StandardArgumentDefinitions.OUTPUT_LONG_NAME, shortName = StandardArgumentDefinitions.OUTPUT_SHORT_NAME, doc = "File to which to write block information (if not specified, prints to standard output", optional = true)


Missing ')'

jamesemery · 2018-01-25T19:21:51Z

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

+public class PrintBGZFBlockInformationIntegrationTest extends CommandLineProgramTest {
+
+    @Test
+    public void testNormalInput() throws IOException {


You should probably add a test asserting that this behaves rationally for bad input (eg. ending on a non-empty block or by having null blocks interspersed with occupied blocks in the file)

I'll add a test with a corrupt BGZF file.

jamesemery · 2018-01-25T19:33:39Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+        }
+    }
+
+    private BGZFBlockMetadata processNextBlock( InputStream stream, String streamSource) throws IOException {


This looks like it was pulled from htsjdk, this should maybe exist in some common utility method somewhere exposed in htsjdk ideally.

Ideally, yes, but beyond the scope of this PR.

jamesemery · 2018-01-25T19:35:08Z

...nder/tools/diagnostics/expected_PrintBGZFBlockInformationIntegrationTest_testNormalInput.out

@@ -0,0 +1,1712 @@
+Block at file offset 0


Perhaps these output files should have a header pointing to the file they are summarizing to avoid confusion?

Good idea, added.

jamesemery · 2018-01-25T19:37:14Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+@ExperimentalFeature
+@CommandLineProgramProperties(
+        summary = "Print information about the compressed blocks in a BGZF format file",
+        oneLineSummary = "Print information about the compressed blocks in a BGZF format file",


Maybe expand this comment to more explicitly summarize the compressed and uncompressed size.

Not necessary I think -- the tool-level docs have that information.

jamesemery · 2018-01-25T20:26:33Z

Also, this seems to misbehave when run on a GZIP file that is NOT a block gzipped file. I get the following result which appears to be flawed in a number of ways:

Block at file offset 0
	- compressed size: 25442
	- uncompressed size: 2114545489

15:24:38.967 INFO  PrintBGZFBlockInformation - Shutting down engine
[January 25, 2018 3:24:38 PM EST] org.broadinstitute.hellbender.tools.diagnostics.PrintBGZFBlockInformation done. Elapsed time: 0.00 minutes.
Runtime.totalMemory()=190840832
***********************************************************************

A USER ERROR has occurred: Error while parsing BGZF file. Error message was: testBrokenFile/gnomADaccuracyTest.SynDip.unBlocked.vcf.gz has invalid uncompressedLength: -758824605

***********************************************************************
Set the system property GATK_STACKTRACE_ON_USER_EXCEPTION (--java-options '-DGATK_STACKTRACE_ON_USER_EXCEPTION=true') to print the stack trace.

magicDGS

Some minor comments for documentation and simplification.

magicDGS · 2018-01-26T09:35:31Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+ * A diagnostic tool that prints information about the compressed blocks in a BGZF format file,
+ * such as a .vcf.gz file.
+ *
+ * The output looks like this:


For properly formatted in the docgen, this requires the <p> HTML tag (I guess).

magicDGS · 2018-01-26T09:35:51Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+ *
+ * The output looks like this:
+ *
+ * Block at file offset 0


This probably can be in a <code> tag.

magicDGS · 2018-01-26T09:36:12Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+ * ...
+ * etc.
+ *
+ * The output can be redirected to a file using the -O option.


Also <p>.

magicDGS · 2018-01-26T09:37:37Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+        programGroup = DiagnosticsAndQCProgramGroup.class
+)
+public class PrintBGZFBlockInformation extends CommandLineProgram {
+    private final Logger logger = LogManager.getLogger(PrintBGZFBlockInformation.class);


CommandLineProgram already has a protected logger initialized for this.getClass().

Good point -- removed

magicDGS · 2018-01-26T09:39:21Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+        bgzfPath = IOUtils.getPath(bgzfPathString);
+
+        if ( ! Files.exists(bgzfPath) ) {
+            throw new UserException("File " + bgzfPathString + " does not exist");


Maybe better a UserException.CouldNotReadInputFile to narrow down?

Changed to UserException.CouldNotReadInputFile

magicDGS · 2018-01-26T09:40:04Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+
+        if ( ! AbstractFeatureReader.hasBlockCompressedExtension(bgzfPathString) ) {
+            throw new UserException("File " + bgzfPathString + " does not end in a recognized BGZF file extension (" +
+                    AbstractFeatureReader.BLOCK_COMPRESSED_EXTENSIONS + ")");


Maybe a UserException.BadInput instead to be more specific?

magicDGS · 2018-01-26T09:41:47Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+        try ( InputStream bgzfInputStream = Files.newInputStream(bgzfPath) ) {
+            BGZFBlockMetadata blockInfo = null;
+            while ( (blockInfo = processNextBlock(bgzfInputStream, bgzfPathString)) != null ) {
+                outStream.println(String.format("Block at file offset %d", blockInfo.blockOffset));


Better outStream.printf() for removing the String.format().

magicDGS · 2018-01-26T09:42:10Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+                outStream.println();
+            }
+        } catch ( IOException e ) {
+            throw new UserException("Error while parsing BGZF file. Error message was: " + e.getMessage(), e);


Better a UserException.CouldNotReadInputFile, no?

Yes, fixed.

cmnbroad · 2018-02-09T20:03:39Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+@CommandLineProgramProperties(
+        summary = "Print information about the compressed blocks in a BGZF format file",
+        oneLineSummary = "Print information about the compressed blocks in a BGZF format file",
+        programGroup = DiagnosticsAndQCProgramGroup.class


I think this would be out-of-place in DiagnosticsAndQCProgramGroup, which currently contains things like metrics collectors, amongst other things. Other utilities like this are in OtherProgramGroup.

Moved to OtherProgramGroup

… a BGZF file

droazen · 2019-01-11T23:13:28Z

@jamesemery I added a pretty comprehensive set of tests for various kinds of corrupt BGZF files (as well as a regular GZIP file), and patched the tool to report something sensible in these cases. You can see the new tool output in the *.out files included in the branch. Needs another review from you before we can merge.

jamesemery

Do with these comments what you will, the tests are good now and this was the primary issue with this branch before so once you have acknowledged/responded 👍

jamesemery · 2019-01-14T19:01:51Z

src/main/java/org/broadinstitute/hellbender/tools/diagnostics/PrintBGZFBlockInformation.java

+        }
+
+        // Emit a warning at the end if we encountered any terminator blocks before the final block:
+        if ( sawNonFinalTerminatorBlock ) {


I would almost think that this should also print out the location of the first one found, just so its use friendly and doesn't confuse anybody about there being two empty blocks in a row at the end.

Either that or clearly separate this from the very similarly formatted output line that appears right before it in the output.

Done -- I added block numbers, and reference them in the error messages.

jamesemery · 2019-01-14T19:08:21Z

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

+        IntegrationTestSpec.assertEqualTextFiles(actualOutput, expectedOutput);
+    }
+
+    @Test(expectedExceptions= UserException.CouldNotReadInputFile.class)


Add a brief comment explaining what this test means.

Added explanatory comments to all tests

jamesemery

This output looks clearer, 👍

droazen requested a review from jamesemery January 23, 2018 21:51

droazen assigned jamesemery Jan 23, 2018

droazen force-pushed the dr_bgzf_block_info_tool branch from 0e696e6 to 5d13c2b Compare January 25, 2018 17:57

jamesemery requested changes Jan 25, 2018

View reviewed changes

jamesemery assigned droazen and unassigned jamesemery Jan 25, 2018

magicDGS reviewed Jan 26, 2018

View reviewed changes

cmnbroad reviewed Feb 9, 2018

View reviewed changes

droazen added 2 commits January 11, 2019 15:46

PrintBGZFBlockInformation: a tool to dump information about blocks in…

4b90d90

… a BGZF file

Update tool and address comments

c57a265

droazen force-pushed the dr_bgzf_block_info_tool branch from 5d13c2b to c57a265 Compare January 11, 2019 23:09

droazen assigned jamesemery Jan 11, 2019

jamesemery approved these changes Jan 14, 2019

View reviewed changes

Address more review comments

35ce7cd

jamesemery approved these changes Jan 14, 2019

View reviewed changes

droazen added 2 commits January 15, 2019 10:48

Change tool package

f9e3288

Update example in tool docs

bef06f9

droazen merged commit 2d0a4d1 into master Jan 15, 2019

droazen deleted the dr_bgzf_block_info_tool branch January 15, 2019 18:23

PrintBGZFBlockInformation: a tool to dump information about blocks in a BGZF file #4239

PrintBGZFBlockInformation: a tool to dump information about blocks in a BGZF file #4239

Conversation

droazen commented Jan 23, 2018

droazen commented Jan 23, 2018

codecov-io commented Jan 25, 2018 • edited Loading

Codecov Report

jamesemery left a comment • 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

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

jamesemery commented Jan 25, 2018 • edited Loading

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

Choose a reason for hiding this comment

droazen commented Jan 11, 2019

jamesemery 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

jamesemery left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 25, 2018 •

edited

Loading

jamesemery left a comment •

edited

Loading

jamesemery commented Jan 25, 2018 •

edited

Loading