TableReader and TableWriter take in a Path #5785

jean-philippe-martin · 2019-03-11T21:30:44Z

All subclasses do the same.

This involves updating all callers to pass a Path instead of a File.

Added a test in TableUtilsUnitTest that writes a table to an in-memory Path to demonstrate we're not using files underneath (it works). Added a similar test in TableReaderUnitTest that reads from an in-memory Path.

Fixes #5747

codecov-io · 2019-03-11T22:15:07Z

Codecov Report

Merging #5785 into master will increase coverage by 0.027%.
The diff coverage is 92.737%.

@@               Coverage Diff               @@
##              master     #5785       +/-   ##
===============================================
+ Coverage     87.003%   87.029%   +0.027%     
- Complexity     32091     32104       +13     
===============================================
  Files           1975      1972        -3     
  Lines         147184    147187        +3     
  Branches       16228     16201       -27     
===============================================
+ Hits          128054    128096       +42     
+ Misses         13225     13184       -41     
- Partials        5905      5907        +2

Impacted Files	Coverage Δ	Complexity Δ
...er/tools/walkers/mutect/filtering/FilterStats.java	`95.455% <ø> (ø)`	`9 <0> (ø)`	⬇️
...r/tools/walkers/readorientation/AltSiteRecord.java	`93.846% <ø> (ø)`	`11 <0> (ø)`	⬇️
...r/tools/walkers/readorientation/ArtifactPrior.java	`100% <ø> (ø)`	`9 <0> (ø)`	⬇️
...rg/broadinstitute/hellbender/utils/io/IOUtils.java	`72.615% <0%> (-0.224%)`	`98 <1> (+1)`
...oadinstitute/hellbender/utils/hmm/HMMUnitTest.java	`80.877% <100%> (ø)`	`102 <0> (ø)`	⬇️
...oadinstitute/hellbender/utils/tsv/TableReader.java	`80.612% <100%> (+0.404%)`	`39 <1> (ø)`	⬇️
...lkers/mutect/filtering/Mutect2FilteringEngine.java	`97.938% <100%> (ø)`	`42 <1> (ø)`	⬇️
...walkers/readorientation/AltSiteRecordUnitTest.java	`100% <100%> (ø)`	`10 <4> (ø)`	⬇️
...e/hellbender/tools/walkers/mutect/MutectStats.java	`88.889% <100%> (+0.654%)`	`5 <2> (ø)`	⬇️
...ers/readorientation/LearnReadOrientationModel.java	`96.078% <100%> (+0.039%)`	`30 <0> (ø)`	⬇️
... and 90 more

samuelklee

Just a few minor comments on the CNV code from me, I'll leave the rest up to another reviewer. Thanks for doing this!

samuelklee · 2019-03-11T23:10:54Z

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

+            try {
+                final LineReader lineReader = new BufferedLineReader(Files.newInputStream(path));
+                return new SAMTextHeaderCodec().decode(lineReader, getSource());
+            } catch (IOException ex) {


We typically make the final explicit (probably for no practical reason other than style).

Thank you for the compliment! I'm guessing you mean the IOException variable? Done.

samuelklee · 2019-03-11T23:11:30Z

src/main/java/org/broadinstitute/hellbender/utils/mcmc/ParameterReader.java

@@ -1,5 +1,6 @@
 package org.broadinstitute.hellbender.utils.mcmc;

+import java.nio.file.Path;


OK to delete this and ParameterWriter, I believe they are dead code.

The compiler agrees with your assessment. Deleted!

jean-philippe-martin · 2019-03-12T16:40:43Z

Checks passed, most excellent!

All subclasses do the same. This involves updating all callers to pass a Path instead of a File. Fixes #5747

Also, update the newly-written code to use Path instead of File. We're in a race of sorts, it seems.

jean-philippe-martin · 2019-03-12T17:06:12Z

Conflict resolved.

@samuelklee if you don't mind my asking, could we please try to have a prompt review cycle?

This change touches a lot of files, some of which are under active development (as evidenced by the fact we got a conflict already). We're in a race between people writing new code that uses File and my converting it to use Path so it will work on non-local filesystems.

samuelklee · 2019-03-12T17:15:09Z

Great, just spoke to @lbergelson @jamesemery and someone from engine team will try to get this turned around quick. Thanks again!

A unit test with Jimfs, so it's fast but still checks that we're not converting the Path to a File along the way.

jean-philippe-martin · 2019-03-12T17:19:03Z

Great, thank you! I'm no longer adding code, you can have a look.

jean-philippe-martin · 2019-03-13T17:38:41Z

It looks like the regression test failed due to some connectivity blip:

pip._vendor.requests.packages.urllib3.exceptions.ProtocolError: ("Connection broken:
ConnectionResetError(104, 'Connection reset by peer')", 
ConnectionResetError(104, 'Connection reset by peer'))

In the hope it's transient, could you please restart it?

droazen · 2019-03-13T17:45:05Z

@jean-philippe-martin Restarted

jean-philippe-martin · 2019-03-13T20:01:42Z

Thank you @droazen ! It passed this time, yay!

jean-philippe-martin · 2019-03-15T16:49:13Z

@cmnbroad if you could please review this code (if you haven't started already)? I'd like to get it in before one of the many touched files gets modified (or more subclasses get created and follow the bad example of using a File instead of Path).

cmnbroad

Looks pretty good - a few minor changes requested.

...broadinstitute/hellbender/tools/copynumber/formats/collections/AbstractRecordCollection.java

cmnbroad · 2019-03-18T16:04:41Z

...broadinstitute/hellbender/tools/exome/orientationbiasvariantfilter/OrientationBiasUtils.java

@@ -186,16 +187,17 @@ public static void writeOrientationBiasSummaryTable(final List<Pair<String, Tran
                                                                                            final Function<DataLine, T> dataLineToSummaryFunction) {
        Utils.nonNull(inputFile);
        IOUtils.canReadFile(inputFile);
+        Path inputPath = IOUtils.fileToPath(inputFile);


Good catch, done.

src/test/java/org/broadinstitute/hellbender/utils/hmm/HMMUnitTest.java

src/test/java/org/broadinstitute/hellbender/utils/tsv/TableReaderUnitTest.java

cmnbroad · 2019-03-18T20:22:19Z

src/test/java/org/broadinstitute/hellbender/utils/tsv/TableReaderUnitTest.java

+        return testFile;
+    }
+
+    private Path createTestInputInMemory(final java.nio.file.FileSystem ramfs, final String... lines) throws IOException {


Rather than adding a new "create" method with a FileSystem arg, add createTestInputOnPath with a Path arg, and have createTestInput pass a Path to that method. Then the jimfs test above can create the jimfs file directly and pass the pathto createTestInputOnPath. All the jimfs code will be confined to that one test, and fillTestInput can go away:

Suggested change

private Path createTestInputInMemory(final java.nio.file.FileSystem ramfs, final String... lines) throws IOException {

private Path createTestInput(final String... lines) throws IOException {

final Path testFile = createTempPath("test", ".tab");

return createTestInputOnPath(testFile, lines);

}

private Path createTestInputOnPath(Path testPath, final String... lines) throws IOException {

try (final PrintWriter testWriter = new PrintWriter(Files.newBufferedWriter(testPath));) {

for (final String line : lines) {

testWriter.println(line);

}

}

return testPath;

}

OK, done! I didn't merge your suggestion (because that would undo the try-with-resources I just put in), but I copied it by hand and you should see it match in spirit.

src/test/java/org/broadinstitute/hellbender/utils/tsv/TableWriterUnitTest.java

cmnbroad · 2019-03-18T20:34:05Z

src/test/java/org/broadinstitute/hellbender/utils/tsv/TableUtilsUnitTest.java

+
+    /**
+     * Check that TableWriter can accept paths that have no corresponding File
+     * on the local disk.


Nit perhaps, but can you update this comment as it seems misleading. The purpose of the test is to show that paths on installed NIO file system providers work; the fact that its using an in-memory file system with no on disk representation seems incidental.

No problem. The intent was to say "check that this code doesn't try to convert the Path back to a File" (which you could do if the Path represented a file), I changed the text to be hopefully clearer.

jean-philippe-martin · 2019-03-18T22:27:20Z

Thank you @cmnbroad for the excellent review! I've applied all the changes, let's see how Travis likes them.

cmnbroad

Thanks for taking this one on @jean-philippe-martin.

samuelklee reviewed Mar 11, 2019

View reviewed changes

jean-philippe-martin added 3 commits March 12, 2019 09:55

TableReader and TableWriter take in a Path

247a8a4

All subclasses do the same. This involves updating all callers to pass a Path instead of a File. Fixes #5747

Delete dead code, minor style change

29333db

Merge new changes

e4f290c

Also, update the newly-written code to use Path instead of File. We're in a race of sorts, it seems.

jean-philippe-martin force-pushed the tables_use_path branch from 967f4a2 to e4f290c Compare March 12, 2019 17:04

Add NIO test for TableReader

e275927

A unit test with Jimfs, so it's fast but still checks that we're not converting the Path to a File along the way.

droazen requested a review from cmnbroad March 13, 2019 17:23

droazen assigned cmnbroad Mar 13, 2019

cmnbroad requested changes Mar 18, 2019

View reviewed changes

Apply reviewer feedback

02f30b7

cmnbroad approved these changes Mar 19, 2019

View reviewed changes

cmnbroad merged commit 19a42e3 into master Mar 19, 2019

cmnbroad deleted the tables_use_path branch March 19, 2019 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TableReader and TableWriter take in a Path #5785

TableReader and TableWriter take in a Path #5785

jean-philippe-martin commented Mar 11, 2019 •

edited

Loading

codecov-io commented Mar 11, 2019 •

edited

Loading

samuelklee left a comment •

edited

Loading

samuelklee Mar 11, 2019

jean-philippe-martin Mar 11, 2019

samuelklee Mar 11, 2019

jean-philippe-martin Mar 11, 2019

jean-philippe-martin commented Mar 12, 2019

jean-philippe-martin commented Mar 12, 2019

samuelklee commented Mar 12, 2019

jean-philippe-martin commented Mar 12, 2019

jean-philippe-martin commented Mar 13, 2019

droazen commented Mar 13, 2019

jean-philippe-martin commented Mar 13, 2019

jean-philippe-martin commented Mar 15, 2019

cmnbroad left a comment

cmnbroad Mar 18, 2019

jean-philippe-martin Mar 18, 2019

cmnbroad Mar 18, 2019

jean-philippe-martin Mar 18, 2019

cmnbroad Mar 18, 2019

jean-philippe-martin Mar 18, 2019

jean-philippe-martin commented Mar 18, 2019

cmnbroad left a comment

		@@ -1,5 +1,6 @@
		package org.broadinstitute.hellbender.utils.mcmc;

		import java.nio.file.Path;

-    private Path createTestInputInMemory(final java.nio.file.FileSystem ramfs, final String... lines) throws IOException {
+    private Path createTestInput(final String... lines) throws IOException {
+        final Path testFile = createTempPath("test", ".tab");
+        return createTestInputOnPath(testFile, lines);
+    }
+    private Path createTestInputOnPath(Path testPath, final String... lines) throws IOException {
+        try (final PrintWriter testWriter = new PrintWriter(Files.newBufferedWriter(testPath));) {
+            for (final String line : lines) {
+                testWriter.println(line);
+            }
+        }
+        return testPath;
+    }

TableReader and TableWriter take in a Path #5785

TableReader and TableWriter take in a Path #5785

Conversation

jean-philippe-martin commented Mar 11, 2019 • edited Loading

codecov-io commented Mar 11, 2019 • edited Loading

Codecov Report

samuelklee 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

jean-philippe-martin commented Mar 12, 2019

jean-philippe-martin commented Mar 12, 2019

samuelklee commented Mar 12, 2019

jean-philippe-martin commented Mar 12, 2019

jean-philippe-martin commented Mar 13, 2019

droazen commented Mar 13, 2019

jean-philippe-martin commented Mar 13, 2019

jean-philippe-martin commented Mar 15, 2019

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

jean-philippe-martin commented Mar 18, 2019

cmnbroad left a comment

Choose a reason for hiding this comment

jean-philippe-martin commented Mar 11, 2019 •

edited

Loading

codecov-io commented Mar 11, 2019 •

edited

Loading

samuelklee left a comment •

edited

Loading