NIO output support for ApplyBQSR #4424

jean-philippe-martin · 2018-02-16T21:26:54Z

Add path support, and test.

jean-philippe-martin · 2018-02-16T22:52:45Z

I confirmed this can output to GCS like so:

$ ./gatk ApplyBQSR \
  -I src/test/resources/org/broadinstitute/hellbender/tools/BQSR/HiSeq.1mb.1RG.2k_lines.alternate.bam \
  --bqsr-recal-file src/test/resources/org/broadinstitute/hellbender/tools/BQSR/HiSeq.20mb.1RG.table.gz \
  -O gs://$BUCKET/test-output/applybqsr.bam

codecov-io · 2018-02-16T23:37:45Z

Codecov Report

Merging #4424 into master will increase coverage by 1.114%.
The diff coverage is 71.053%.

@@              Coverage Diff               @@
##             master     #4424       +/-   ##
==============================================
+ Coverage     79.04%   80.154%   +1.114%     
- Complexity    16447     19883     +3436     
==============================================
  Files          1047      1092       +45     
  Lines         59189     72137    +12948     
  Branches       9672     12253     +2581     
==============================================
+ Hits          46783     57821    +11038     
- Misses         8645      9988     +1343     
- Partials       3761      4328      +567

Impacted Files	Coverage Δ	Complexity Δ
...roadinstitute/hellbender/utils/read/ReadUtils.java	`85.039% <100%> (+5.089%)`	`214 <3> (+30)`	⬆️
...itute/hellbender/tools/walkers/bqsr/ApplyBQSR.java	`91.667% <100%> (ø)`	`6 <1> (ø)`	⬇️
...ava/org/broadinstitute/hellbender/utils/Utils.java	`80.435% <33.333%> (-1.092%)`	`140 <2> (+1)`
...itute/hellbender/utils/test/SamAssertionUtils.java	`72.821% <77.273%> (-2.46%)`	`46 <11> (+6)`
...titute/hellbender/utils/logging/OneShotLogger.java	`78.571% <0%> (-21.429%)`	`3% <0%> (ø)`
...ellbender/engine/filters/VariantFilterLibrary.java	`33.333% <0%> (-16.667%)`	`1% <0%> (ø)`
...stitute/hellbender/engine/ReferenceDataSource.java	`70% <0%> (-10%)`	`7% <0%> (+3%)`
...stitute/hellbender/engine/ReferenceFileSource.java	`65.217% <0%> (-7.51%)`	`8% <0%> (+4%)`
...titute/hellbender/utils/test/ArgumentsBuilder.java	`93.151% <0%> (-6.849%)`	`31% <0%> (+12%)`
...ools/spark/sv/evidence/IntervalCoverageFinder.java	`82.222% <0%> (-6.239%)`	`19% <0%> (+11%)`
... and 205 more

jean-philippe-martin · 2018-02-17T01:27:54Z

Part of bug #2422

lbergelson · 2018-02-23T19:35:02Z

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

+        }
+        if (Files.isDirectory(path)) {
+            throw new IOException("File '" + fname + "' exists but is a directory");
+        }


Should this check Files.isRegularFile() as well? It seems wrong to try to take the md5 of a pipe or something like that.

Agreed, done.

lbergelson · 2018-02-23T19:35:57Z

src/main/java/org/broadinstitute/hellbender/utils/read/ReadUtils.java

-            return SamStreams.isCRAMFile(bis);
+    public static boolean hasCRAMFileContents(final Path putativeCRAMPath) {
+        try (final InputStream fileStream = Files.newInputStream(putativeCRAMPath)) {
+            try (final BufferedInputStream bis = new BufferedInputStream(fileStream)) {


I think it's better to have 2 resources declared in the same try() block rather than having 2 nested try. I would switch it back to how it was.

It doesn't compile that way, complains about an IOException. Suggestions welcome, though.

We tried it, and it does actually work as a single try-with-resources

droazen · 2018-03-12T19:06:48Z

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

@@ -531,6 +534,17 @@ public static String calcMD5(final byte[] bytes) {
    public static String calculateFileMD5( final File file ) throws IOException{
        return Utils.calcMD5(FileUtils.readFileToByteArray(file));
    }
+    public static String calculatePathMD5(final Path path) throws IOException{


Add javadoc for the new overload of calculatePathMD5()

Also, can you make sure that these new method overloads are covered by unit tests?

I added Javadoc and made sure that the first calls the second. Since the first is tested, that also covers the second automatically.

lbergelson · 2018-03-12T19:09:37Z

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

@@ -531,6 +534,17 @@ public static String calcMD5(final byte[] bytes) {
    public static String calculateFileMD5( final File file ) throws IOException{
        return Utils.calcMD5(FileUtils.readFileToByteArray(file));


seems like if we can we should make all the old methods delegate to the new ones

Agreed, done.

droazen · 2018-03-12T19:11:05Z

src/main/java/org/broadinstitute/hellbender/utils/test/SamAssertionUtils.java

@@ -161,14 +171,46 @@ public static String samsEqualStringent(final File actualSam, final File expecte
        return compareReads(actualSam, expectedSam, validation, reference);
    }

+    public static String samsEqualStringent(final Path actualSam, final Path expectedSam, final ValidationStringency validation, final Path reference) throws IOException {


Can you make the file overload of this method call into the Path version?

Also add javadoc

Certainly! Done.

droazen · 2018-03-12T19:13:25Z

src/main/java/org/broadinstitute/hellbender/utils/test/SamAssertionUtils.java

@@ -215,6 +257,32 @@ private static String equalHeadersIgnoreCOandPG(final File actualSam, final File
            return msg;
        }
    }
+    private static String equalHeadersIgnoreCOandPG(final Path actualSam, final Path expectedSam, final ValidationStringency validation, final Path reference) throws IOException {


Can you make the file overload call into this one?

Oh actually I meant to delete it; it's private and has no callers.

droazen · 2018-03-12T19:14:37Z

src/main/java/org/broadinstitute/hellbender/utils/test/SamAssertionUtils.java

+            assertCRAMContents(putativeCRAMFile.toPath());
+        }
+    }
+    public static void assertCRAMContentsIfCRAM(final Path putativeCRAMPath) {


javadoc for this method

Done; I also changed the File version to more cleanly call the Path version.

droazen · 2018-03-12T19:17:33Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java

-        if (params.reference != null) {
-            refFile = new File(params.reference);
-            args.add("-R"); args.add(refFile.getAbsolutePath());
+        try (FileSystem jimfs = Jimfs.newFileSystem(Configuration.unix())) {


Can you keep the existing test code as-is, and make the Jimfs-based tests a separate test method? It would also be good to have one test case that writes to a live GCS bucket (marked as groups = {bucket})

Re-instated the old (File-based) test. Remains to add a GCS test.

GCS test added. For now it runs all the cases, but since the other tests already establish that ApplyBQSR does the right thing and we just want to make sure it can write to GCS, it'd make sense to only run a subset of the cases on the cloud. If you let me know which ones you want, I can cull the rest.

droazen · 2018-03-12T19:20:49Z

src/test/java/org/broadinstitute/hellbender/utils/test/SamAssertionUtilsUnitTest.java

    }

    @Test(dataProvider = "testCRAMContentsFail", expectedExceptions=AssertionError.class)
    public void testAssertCRAMContentsFail(File putativeCRAMFile) {
-        SamAssertionUtils.assertCRAMContents(putativeCRAMFile);
+        SamAssertionUtils.assertCRAMContents(putativeCRAMFile.toPath());


Test the file-based overloads as well

There's no file-based overload of assertCRAMContents anymore.

droazen · 2018-03-12T19:22:08Z

@jean-philippe-martin Review complete, back to you

jean-philippe-martin · 2018-03-15T12:36:17Z

@droazen all review comments applied, back to you!

droazen · 2018-04-09T19:26:39Z

src/main/java/org/broadinstitute/hellbender/utils/test/SamAssertionUtils.java

+
+    /**
+     * Validate/assert that the contents are CRAM if the extension is .cram
+     */    public static void assertCRAMContentsIfCRAM(final Path putativeCRAMPath) {


Indentation here is off

Good catch, thank you! Fixed.

droazen · 2018-04-09T19:27:15Z

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

+     * @return file's MD5 in String form
+     * @throws IOException if the file could not be read
+     */
+    public static String calculatePathMD5(final Path path) throws IOException{


Can you add a warning to the javadoc for this method that it slurps the entire file into memory, and should not be used for large files.

droazen · 2018-04-09T19:27:20Z

src/main/java/org/broadinstitute/hellbender/utils/Utils.java

@@ -529,7 +532,29 @@ public static String calcMD5(final byte[] bytes) {
     * @throws IOException if the file could not be read
     */
    public static String calculateFileMD5( final File file ) throws IOException{


Can you add a warning to the javadoc for this method that it slurps the entire file into memory, and should not be used for large files.

droazen · 2018-04-09T19:31:46Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java

+    }
+
+    @Test(dataProvider = "ApplyBQSRTest")
+    public void testApplyBQSRPath(ABQSRTest params) throws IOException {


This test doesn't need to be run on the full ApplyBQSRTest DataProvider -- can you create a separate DataProvider with ONE of the test cases from the ApplyBQSRTest provider, and use that here?

Absolutely! Done!

droazen · 2018-04-09T19:37:07Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java


-        runCommandLine(args);
+    @Test(dataProvider = "ApplyBQSRTest", groups={"cloud", "bucket"})


This test should be just in the bucket group, not the cloud group, since execution is local

Good catch, thank you! Fixed!

droazen · 2018-04-09T19:39:05Z

src/test/java/org/broadinstitute/hellbender/tools/walkers/bqsr/ApplyBQSRIntegrationTest.java

+            SamAssertionUtils.assertSamsEqual(outPath, new File(params.expectedFile).toPath(), refPath);
+        } finally {
+            if (Files.exists(outPath)) {
+                Files.delete(outPath);


Doesn't BucketUtils.getTempFilePath() already mark this for deletion on JVM exit? (I guess this doesn't hurt though)

Oh yes, you're right! I removed the delete and the try/finally since they are redundant. I also added a comment to help in case others also forget.

droazen

Final review complete -- a few minor remaining TODOs, then we can merge this.

jean-philippe-martin · 2018-04-16T16:59:44Z

Thank you very much @droazen! This should take care of all the comments, so once the checks are green I'll press "squash and merge." Then I'll move on to the next tool to update!

* ApplyBQSR can output BAMs to GCS Add path support, and test.

jean-philippe-martin added 2 commits February 16, 2018 13:18

ApplyBQSR can output BAMs to GCS

2536af7

Add path support, and test.

allow null reference in SamAssertionUtils.compareReads

b7c0c00

jean-philippe-martin changed the title ~~ApplyBQSR can output BAMs to GCS~~ NIO output support for ApplyBQSR Feb 17, 2018

jean-philippe-martin requested a review from droazen February 22, 2018 22:39

droazen self-assigned this Feb 23, 2018

lbergelson reviewed Mar 12, 2018

View reviewed changes

droazen reviewed Mar 12, 2018

View reviewed changes

lbergelson reviewed Mar 12, 2018

View reviewed changes

droazen reviewed Mar 12, 2018

View reviewed changes

droazen assigned jean-philippe-martin and unassigned droazen Mar 12, 2018

jean-philippe-martin added 2 commits March 14, 2018 10:29

reviewer comments

9d0403b

randomize name of temp file

ed3632a

jean-philippe-martin assigned droazen and unassigned jean-philippe-martin Mar 15, 2018

droazen reviewed Apr 9, 2018

View reviewed changes

droazen suggested changes Apr 9, 2018

View reviewed changes

Address reviewer comments

077e85f

jean-philippe-martin merged commit 984e783 into master Apr 16, 2018

jean-philippe-martin deleted the jp_applybqsr_nio branch April 16, 2018 22:38

cwhelan pushed a commit to cwhelan/gatk-linked-reads that referenced this pull request May 25, 2018

NIO output support for ApplyBQSR (broadinstitute#4424)

ca9b387

* ApplyBQSR can output BAMs to GCS Add path support, and test.

		@@ -531,6 +534,17 @@ public static String calcMD5(final byte[] bytes) {
		public static String calculateFileMD5( final File file ) throws IOException{
		return Utils.calcMD5(FileUtils.readFileToByteArray(file));


		runCommandLine(args);
		@Test(dataProvider = "ApplyBQSRTest", groups={"cloud", "bucket"})

NIO output support for ApplyBQSR #4424

NIO output support for ApplyBQSR #4424

Conversation

jean-philippe-martin commented Feb 16, 2018

jean-philippe-martin commented Feb 16, 2018

codecov-io commented Feb 16, 2018 • edited Loading

Codecov Report

jean-philippe-martin commented Feb 17, 2018

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 Mar 12, 2018 • 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

droazen commented Mar 12, 2018

jean-philippe-martin commented Mar 15, 2018

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 Apr 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen left a comment

Choose a reason for hiding this comment

jean-philippe-martin commented Apr 16, 2018

codecov-io commented Feb 16, 2018 •

edited

Loading

droazen Mar 12, 2018 •

edited

Loading

droazen Apr 9, 2018 •

edited

Loading