Enable VcfFileReader and other classes to use Path #1018

yfarjoun · 2017-10-26T08:00:43Z

Description

Enables the use of Path, and therefore NIO implementations such as google-cloud-nio when opening VCF readers. (though the google-cloud-nio library is not added)
Cleans up ununsed test files and some white-space/java8 issues

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
Extended the README / documentation, if necessary
Is not backward compatible (breaks binary or source compatibility)

magicDGS

I disagree on using GCS for the tests, more when there is already another filesystem for testing (jimfs).

magicDGS · 2017-10-27T13:33:23Z

build.gradle

@@ -43,6 +43,8 @@ dependencies {
    testRuntime 'org.pegdown:pegdown:1.4.2' // Necessary for generating HTML reports with ScalaTest
    testCompile "org.testng:testng:6.9.9"
    testCompile "com.google.jimfs:jimfs:1.1"
+    // https://mvnrepository.com/artifact/com.google.cloud/google-cloud-nio
+    testCompile group: 'com.google.cloud', name: 'google-cloud-nio', version: '0.5.0'


I would use jimfs instead of google-cloud for testing a different filesystem. Like that, you can avoid the issues with your GCS costs...

magicDGS · 2017-10-27T13:37:41Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+     * @param p a {@link Path} that is to be read in as text
+     * @return an iterator over the lines in the file
+     */
+    public static IterableOnceIterator<String> readLines(final Path p) {


Could be Files.lines(Path) used instead of creating a new utility method?

magicDGS · 2017-10-27T13:39:57Z

src/test/java/htsjdk/variant/vcf/VCFPathReaderTest.java

+                //testing GCS files:
+
+                // this is almost a vcf, but not quite it's missing the #CHROM line and it has no content...
+                {"gs://htsjdk-testdata/htsjdk/variant/Homo_sapiens_assembly38.tile_db_header.vcf", false, false},


Tests for other FS could use Jimfs: copy the local files into Jimfs, and use the filesystem for the test to create the strings...

yfarjoun · 2017-11-15T16:34:31Z

had to rebase to resolve a conflit. Kept all the original commits

codecov-io · 2017-11-15T17:07:54Z

Codecov Report

Merging #1018 into master will decrease coverage by 0.058%.
The diff coverage is 60.274%.

@@               Coverage Diff               @@
##              master     #1018       +/-   ##
===============================================
- Coverage     66.093%   66.034%   -0.058%     
- Complexity      7558      7592       +34     
===============================================
  Files            532       532               
  Lines          32238     32427      +189     
  Branches        5494      5531       +37     
===============================================
+ Hits           21307     21413      +106     
- Misses          8775      8847       +72     
- Partials        2156      2167       +11

Impacted Files	Coverage Δ	Complexity Δ
...va/htsjdk/tribble/TribbleIndexedFeatureReader.java	`68.889% <ø> (+1.667%)`	`24 <0> (+2)`	⬆️
...rc/main/java/htsjdk/samtools/SamReaderFactory.java	`63.861% <100%> (+0.73%)`	`7 <0> (ø)`	⬇️
...rc/main/java/htsjdk/tribble/util/ParsingUtils.java	`74.346% <100%> (+0.801%)`	`46 <3> (-1)`	⬇️
...rc/main/java/htsjdk/variant/vcf/VCFFileReader.java	`50% <55.556%> (-22.549%)`	`21 <18> (+5)`
src/main/java/htsjdk/samtools/util/IOUtil.java	`38.083% <57.143%> (+2.721%)`	`80 <9> (+9)`	⬆️
.../variant/utils/SAMSequenceDictionaryExtractor.java	`72.5% <61.538%> (-6.987%)`	`2 <2> (+1)`
...samtools/util/AsyncBlockCompressedInputStream.java	`72% <0%> (-4%)`	`12% <0%> (-1%)`
...a/htsjdk/samtools/util/AbstractProgressLogger.java	`38.889% <0%> (-1.111%)`	`9% <0%> (+3%)`
... and 4 more

yfarjoun · 2017-11-15T20:10:32Z

@magicDGS @jacarey I fixed things as per your comments...care to take another glance?

pshapiro4broad · 2017-11-15T20:44:50Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+
+                try {
+                    Files.lines(p).forEach(s -> {
+                        if (!s.trim().isEmpty()) {


instead of calling String.trim() multiple times here use map(), e.g.

Files.lines(p).map(String::trim).forEach(...)

You could also filter there too, e.g. .map(...).filter(s -> !s.isEmpty()).forEach()

pshapiro4broad · 2017-11-15T20:50:14Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+
+        while (!stack.empty()) {
+            final Path p = stack.pop();
+            final String name = p.toUri().toString();


Why call .toUri().toString() here? Seems like p.toString() would produce the same result.

I don't think it does. it drops the schema.

There is no scheme for a Path. Anyway the code is testing the end of the path, so it doesn't matter what the scheme is.

pshapiro4broad · 2017-11-15T20:54:27Z

src/main/java/htsjdk/samtools/util/IOUtil.java

@@ -1064,6 +1076,60 @@ public static String slurp(final InputStream is, final Charset charSet) {
    }

    /**
+     * Go through the files provided and if they have one of the provided file extensions pass the file into the output
+     * otherwise assume that file is a list of filenames and unfold it into the output.


This method also recursively expands files of filenames, if one of the filenames in a file of filenames points to another file of filenames. If this is the intended behavior it should be documented and tested.

is tested, will update message.

pshapiro4broad · 2017-11-15T20:56:34Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+     * Returns true if the given path appears to be a BCF file.
+     */
+    public static boolean isBCF(final Path path) {
+        return path.toUri().getRawPath().endsWith(".bcf");


Why use .toUri() here and below? In this case you could write path.toString().endsWith(".bcf")

because I wanted to remove the possible junk after a ? in a URL.

In what case does a Path have a query string? A Path is not a URI and a query string would be invalid, as far as I know.

I think that path could have a query string...but I could be wrong.

pshapiro4broad · 2017-11-15T20:57:59Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+            if (includeFiltered || !vc.isFiltered()) {
+                String name = vc.getID();
+                final Integer intervalEnd = vc.getCommonInfo().getAttributeAsInt("END", vc.getEnd());
+                if (".".equals(name) || name == null)


Need { } around if body. Also IMO it's more natural to write if (name == null || name.equals("."))

pshapiro4broad · 2017-11-15T20:59:32Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+        try {
+            return reader.iterator();
+        } catch (final IOException ioe) {
+            throw new TribbleException("Could not create an iterator from a feature reader.", ioe);


Why is this code using TribbleException if it's not part of htsjdk.tribble? If you want an unchecked IO Exception you can use UncheckedIOException.

this is a copy of the VCFFileReader...I made very few changes....I think unless we want a serious rewrite of htsjdk (which is out of scope for this PR) I'd vote for keeping the current (slightly annoying) behavior.

pshapiro4broad · 2017-11-15T21:00:53Z

src/test/java/htsjdk/samtools/util/IoUtilTest.java

-    private static final File FIVE_SPACES_THEN_A_NEWLINE_THEN_FIVE_SPACES_FILE = new File("src/test/resources/htsjdk/samtools/io/5newline5.txt");
+
+    private static final File TEST_DATA_DIR = new File ("src/test/resources/htsjdk/samtools/io/");
+    private static final File SLURP_TEST_FILE = new File(TEST_DATA_DIR,"slurptest.txt");


missing space after ,, this occurs many places in this checkin

pshapiro4broad · 2017-11-15T21:01:13Z

src/test/java/htsjdk/samtools/util/IoUtilTest.java

-    private static final File EMPTY_FILE = new File("src/test/resources/htsjdk/samtools/io/empty.txt");
-    private static final File FIVE_SPACES_THEN_A_NEWLINE_THEN_FIVE_SPACES_FILE = new File("src/test/resources/htsjdk/samtools/io/5newline5.txt");
+
+    private static final File TEST_DATA_DIR = new File ("src/test/resources/htsjdk/samtools/io/");


Would be better to use Path here if possible.

OK...though it makes things a bit more cumbersome

Oh. nice. thanks.

pshapiro4broad · 2017-11-15T21:01:40Z

src/test/java/htsjdk/samtools/util/IoUtilTest.java

+    @DataProvider
+    public Object[][] fofnData() throws IOException {
+        Path fofnPath1 = inMemoryfileSystem.getPath("Level1.fofn");
+        Files.copy(new File(TEST_DATA_DIR.getAbsolutePath(),"Level1.fofn").toPath(), fofnPath1);


This should be TEST_DATA_DIR.getPath().resolve("Level1.fofn")

TIL. thanks.

magicDGS

Quick comments from the phone. I will do. a proper review in one week if this is still open!

magicDGS · 2017-11-16T13:57:13Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

            throws IOException {

        final InputStream inputStream;
        if (path.startsWith("http:") || path.startsWith("https:") || path.startsWith("ftp:")) {
            inputStream = getURLHelper(new URL(path)).openInputStream();
-        } else if (IOUtil.hasScheme(path)) {
+        } else if (IOUtil.hasScheme(path) && !IOUtil.getScheme(path).equals("file")) {


Maybe get the scheme before, and use it for checking http, ftp, etc...

magicDGS · 2017-11-16T13:59:08Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

            inputStream = new SeekablePathStream(IOUtil.getPath(path), wrapper);
        } else {
-            File file = new File(path);
+            File file = IOUtil.getScheme(path) != null && IOUtil.getScheme(path).equals("file") ? new File(path.substring("file:".length())) : new File(path);


This branch can be used with the SeekablePathStream too, no?

the problem, I think, is that a file can represent a named pipe, which is not seekable.

magicDGS · 2017-11-16T14:02:13Z

src/main/java/htsjdk/variant/vcf/VCFFileReader.java

@@ -16,74 +16,84 @@
 import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
+import java.nio.file.Path;

 /**
 * Simplified interface for reading from VCF/BCF files.
 */
 public class VCFFileReader implements Closeable, Iterable<VariantContext> {


deprecate in favor of the Path version?

pshapiro4broad · 2017-11-16T15:33:09Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

@@ -100,16 +101,16 @@ public static InputStream openInputStream(String path)
     * @return
     * @throws IOException
     */
-    public static InputStream openInputStream(String path, Function<SeekableByteChannel, SeekableByteChannel> wrapper)
+    public static InputStream openInputStream(final String path, final Function<SeekableByteChannel, SeekableByteChannel> wrapper)


Argument path should be named uri since it's not a path but a URI. Making the caller create a URI would be helpful too.

The javadoc for this could be improved too, it should have full sentences for its description text. And omitting the text for a @return or @throws is not helpful; either remove the @ form from the javadoc or add the missing documentation.

hmmm. I don't think that changing the API is justified here....I'll develop the docs though

pshapiro4broad · 2017-11-16T15:39:21Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+     */
+    public static SAMSequenceDictionary getSequenceDictionary(final Path path) {
+        final SAMSequenceDictionary dict = new VCFPathReader(path, false).getFileHeader().getSequenceDictionary();
+        CloserUtil.close(path);


What is this trying to do? Path is not closeable and has no close() method.

It looks like CloserUtil might predate Closeable and should really be cleaned up to only accept closeable objects.

not sure what I was thinking there....thanks.

pshapiro4broad · 2017-11-16T15:53:12Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+     */
+    public VCFPathReader(final Path path, final boolean requireIndex) {
+        // Note how we deal with type safety here, just casting to (FeatureCodec)
+        // in the call to getFeatureReader is not enough for Java 8.


This is because the compiler can't infer the common type that both branches of the ternary op produces and the function accepts. If you cast using the parameterized type you can inline this expression, e.g. (FeatureCodec<VariantContext, ?>) (isBCF(path) ? new BCF2Codec() : new VCFCodec()) works.

It looks like isBCF() is only called in two places and in both places it's used to determine which codec to return. If so you can avoid the code duplication and and replace with isBCF() with getCodecForPath(Path). Or keep isBCF() and write:

private static FeatureCodec<VariantContext, ?> getCodecForPath(Path path) { return isBCF(path) ? new BCF2Codec() : new VCFCodec(); }

This also eliminates the need for a cast or introducing a local variable.

pshapiro4broad · 2017-11-16T15:57:25Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+    public static IntervalList fromVcf(final Path path, final boolean includeFiltered) {
+        final VCFPathReader vcfReader = new VCFPathReader(path, false);
+        final IntervalList intervalList = fromVcf(vcfReader, includeFiltered);
+        vcfReader.close();


Using try-with-resources would eliminate the need to call close() here

pshapiro4broad · 2017-11-16T16:00:59Z

src/main/java/htsjdk/variant/vcf/VCFPathReader.java

+     * @return an IntervalList constructed from input vcf
+     */
+
+    public static IntervalList fromVcf(final VCFPathReader vcf) {


In general a static method on object X that also accepts an argument of type X should be converted to a non-static method. E.g. this should be written as:

public IntervalList fromVcf() { return vcf.fromVcf(false); }

Although it's unclear to me if the caller would know that this is going to produce an interval list. Normally you see static fromX() methods to convert object type X to another type. So a better name here might be toIntervalList().

yfarjoun · 2017-11-17T18:41:22Z

back to you @pshapiro4broad

lindenb · 2017-11-17T18:50:08Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

+        if (URL_SCHEMES.contains(scheme)) {
+            inputStream = getURLHelper(new URL(uri)).openInputStream();
+        } else if (scheme.isEmpty()) {
+            File file = new File(uri);
            inputStream = new FileInputStream(file);


when possible use Files.newInputStream instead of new FileInputStream( ( see https://www.cloudbees.com/blog/fileinputstream-fileoutputstream-considered-harmful )

pshapiro4broad · 2017-11-17T19:23:38Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

-            inputStream = new SeekablePathStream(IOUtil.getPath(path), wrapper);
-        } else {
-            File file = new File(path);
+        final Set<String> URL_SCHEMES = new HashSet<>(Arrays.asList("http","ftp","https"));


Should make this a private static to avoid creating every time the method is called.

pshapiro4broad · 2017-11-17T19:41:29Z

src/main/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractor.java

@@ -111,7 +144,10 @@ SAMSequenceDictionary extractDictionary(final File intervalList) {
            applicableExtensions = extensions;
        }

+        /** @deprecated in favor of extractDictionary(final Path file) */


Better to use @link here so it shows up as a hyperlink, e.g.

/** * @deprecated use {@link #extractDictionary(Path)} instead */

pshapiro4broad · 2017-11-17T19:43:08Z

src/main/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractor.java

@@ -111,7 +144,10 @@ SAMSequenceDictionary extractDictionary(final File intervalList) {
            applicableExtensions = extensions;
        }

+        /** @deprecated in favor of extractDictionary(final Path file) */
+        @Deprecated
        abstract SAMSequenceDictionary extractDictionary(final File file);


Wouldn't it be simpler to make this non-abstract and implement it as

return extractDictionary(file.toPath());

?

pshapiro4broad · 2017-11-17T19:44:28Z

src/main/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractor.java

                } finally {
-                    CloserUtil.close(vcfFileReader);
+                    CloserUtil.close(vcfReader);


Can you replace this close() with try-with-resources?

pshapiro4broad · 2017-11-17T19:45:50Z

src/main/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractor.java

        },
        VCF(IOUtil.VCF_EXTENSIONS) {
            @Override
            SAMSequenceDictionary extractDictionary(final File vcf) {
-                VCFFileReader vcfFileReader = null;
+                VCFPathReader vcfReader = null;


Personally I don't think renaming the class is necessary. Even though the API now accepts/requires Path it is still reading a VCF File. The implementation detail of how a file is represented has changed but the functionality hasn't.

pshapiro4broad · 2017-11-17T19:49:22Z

src/main/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractor.java

@@ -124,6 +160,17 @@ static TYPE forFile(final File dictionaryExtractable) {
            throw new SAMException("Cannot figure out type of file " + dictionaryExtractable.getAbsolutePath() + " from extension. Current implementation understands the following types: " + Arrays.toString(TYPE.values()));
        }

+        static TYPE forPath(final Path dictionaryExtractable) {


The enum should really be called Type as uppercase is typically reserved for static final objects.

sorry...that ship has sailed....I can file it under "things to do in htsjdk3"...

yfarjoun · 2017-11-18T18:41:29Z

Tests now pass... @jacarey ? @magicDGS ? @pshapiro4broad ? any more comments?

magicDGS

Some comments below.

In addition, some test data is removed but it does not look that it correspond to this PR. Maybe it should be done in a different one...

magicDGS · 2017-11-21T10:21:20Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

+            inputStream = getURLHelper(new URL(uri)).openInputStream();
+        } else if (scheme.isEmpty()) {
+            File file = new File(uri);
+            inputStream = Files.newInputStream(file.toPath());


Will this handle the case of named-pipes, as @yfarjoun commented in my previous comment? In addition, is there any way to differentiate the pipes from files? If the scheme is empty and it is a File, it feels wrong to me that the method does not return a SeekablePathStream and it does if the scheme is "file:".

unclear.... @lbergelson ?

I assume named pipes can be referred to as either a file or a path, which means that we'd need a different mechanism to check for them. We don't ever do a check for isRegularFile which is I think what we'd need to do.

I'm not sure how much we care if we return a SeekablePathStream for a file, vs a simple input stream. They should have similar performance I hope, and this function only guarantees an input stream. It is weird though to return different types depending on how you write the path though and it probably has some weird knock on effects. That said, it's not a change from how it was set up before is it?

Why did you change from the previous order and using hasScheme? It seems like you added a new local variable and some additional logic and then only use it to do what the simpler check accomplished?

magicDGS · 2017-11-21T10:23:06Z

src/main/java/htsjdk/samtools/SamReaderFactory.java

@@ -142,6 +142,9 @@ public SamReader open(final Path path,
    /** Utility method to open the file get the header and close the file */
    abstract public SAMFileHeader getFileHeader(File samFile);

+    /** Utility method to open the file get the header and close the file */
+    abstract public SAMFileHeader getFileHeader(Path samFile);


Maybe HTSJDK should start deprecating the File versions of the methods in favor of the Path versions. Anyway, for users using File the change is as simple as using File.toPath if there are no differences in the methods...

I think we should probably only start doing that when we have full support of Path....

magicDGS · 2017-11-21T10:27:34Z

src/main/java/htsjdk/variant/vcf/VCFFileReader.java

+     */
+    @Deprecated
+    public VCFFileReader(final File file, final boolean requireIndex) {
+        // Note how we deal with type safety here, just casting to (FeatureCodec)


Maybe it is better to call this(file.toPath(), requireIndex) instead of doing the magic in place (it should result in the same reader).

magicDGS · 2017-11-21T10:27:41Z

src/main/java/htsjdk/variant/vcf/VCFFileReader.java

-                      requireIndex);
+        // Note how we deal with type safety here, just casting to (FeatureCodec)
+        // in the call to getFeatureReader is not enough for Java 8.
+        FeatureCodec<VariantContext, ?> codec = isBCF(file) ? new BCF2Codec() : new VCFCodec();


Same as before.

magicDGS · 2017-11-21T10:28:24Z

src/test/java/htsjdk/samtools/sra/SRALazyRecordTest.java

-    @Test
-    public void testCloneAndEquals() throws Exception {
-        final SRAFileReader reader = new SRAFileReader(DEFAULT_ACCESSION);
+    @Test(dataProvider = "serializationTestData")


Does this correspond to this PR?

no....but I noticed it, so I fixed it...it requires a provider but wasn't given one....

magicDGS · 2017-11-21T10:38:24Z

src/test/java/htsjdk/tribble/TestUtils.java


 /**
 * User: jacob
 * Date: 2012-Dec-13
 */
 public class TestUtils {
    public static String DATA_DIR = "src/test/resources/htsjdk/tribble/";
+
+
+    public static Path getTribbleFileInJimfs(String vcf, String index, FileSystem fileSystem) throws IOException, URISyntaxException {


Add a javadoc explaining that the FileSystem should be a JimfsFileSystem. It took me a while to figure out what was it.

Another option is to provide a destination JimfsPath instead to get the FileSystem from it.

magicDGS · 2017-11-21T10:40:07Z

src/test/java/htsjdk/tribble/TestUtils.java

+        if (index != null) {
+            final Path idxPath = Paths.get(index);
+            final Path idxDestination = Paths.get(AbstractFeatureReader.isTabix(vcf, index) ? Tribble.tabixIndexFile(vcf) : Tribble.indexFile(vcf));
+            Files.copy(idxPath, root.resolve(idxDestination.getFileName().toString()));


You can resolve with the root the vcfPath for the JimfsFileSystem before, and use the Tribble methods for get the idxDestination name without the need to resolve it again against the root.

Thanks. that's a bit simpler.

magicDGS · 2017-11-21T10:40:30Z

src/test/java/htsjdk/variant/utils/SAMSequenceDictionaryExtractorTest.java

@@ -48,7 +48,6 @@
                new Object[]{"Homo_sapiens_assembly18.trimmed.fasta", "Homo_sapiens_assembly18.trimmed.dict"},
                new Object[]{"test2_comp.interval_list", "Homo_sapiens_assembly18.trimmed.dict"},
                new Object[]{"ScreenSamReads.100.input.sam", "test3_comp.interval_list"},
-                new Object[]{"ScreenSamReads.100.input.sam", "test4_comp.interval_list"},


Why is this test case removed?

not sure...putting it back.

magicDGS · 2017-11-21T10:42:32Z

src/test/java/htsjdk/variant/vcf/VCFPathReaderTest.java

+/**
+ * Created by farjoun on 10/12/17.
+ */
+public class VCFPathReaderTest extends HtsjdkTest {


Now that you unified the classes, this should be named VCFFileReaderTest

magicDGS · 2017-11-21T10:42:59Z

src/test/java/htsjdk/variant/vcf/VCFHeaderUnitTest.java

@@ -48,6 +49,7 @@
 import java.io.PrintWriter;
 import java.io.StringReader;
 import java.math.BigInteger;
+import java.nio.file.Path;


Changes in this file are not longer need it.

…n review.

…leReader and deprecating the File versions

- responding to review comments.

yfarjoun · 2017-11-25T18:38:25Z

@magicDGS responded to your comments.

@jacarey @tfenne @lbergelson, this needs a directive from the maintainers I think...

magicDGS · 2017-11-26T07:44:53Z

👍 for my point of view.

yfarjoun · 2017-12-15T21:40:47Z

@lbergelson ?

lbergelson

@yfarjoun Some minor quibbles. Seems fine. There's bunch of now deprecated methods that are now untested, we might want to test both versions until we remove the deprecated ones. We've not been deprecating the file methods until now, but it seems fine to do so, it' just a bit annoying for downstream projects. I'm not sure if it's better to deprecate piecemeal as we add the new versions or all at once when we're ready to pull the plug on files.

lbergelson · 2017-12-18T16:18:57Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+     * Go through the files provided and if they have one of the provided file extensions pass the file to the output
+     * otherwise assume that file is a list of filenames and unfold it into the output (recursively).
+     */
+    public static List<Path> unrollPaths(final Collection<Path> inputs, final String... extensions) {


@yfarjoun Is there a reason we can't replace the file version with a call to the new path code?

well, it returns a List<Path> instead of a List<File>... is that a reason?

You could do the conversion on the way in and out. It's slightly less efficient, but probably worth it to avoid a bunch of lines of code.

fair enough.

lbergelson · 2017-12-18T16:20:21Z

src/main/java/htsjdk/samtools/util/IOUtil.java

@@ -1101,6 +1181,20 @@ public static Path getPath(String uriString) throws IOException {
        }
    }

+    public static List<Path> getPaths(List<String> uriStrings) throws RuntimeException {


this is untested

lbergelson · 2017-12-18T16:20:31Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+            try {
+                return IOUtil.getPath(s);
+            } catch (IOException e) {
+                throw new RuntimeException(e);


RuntimeIOException maybe?

lbergelson · 2017-12-18T16:21:23Z

src/main/java/htsjdk/samtools/util/IOUtil.java

+        }).collect(Collectors.toList());
+    }
+
+    public static List<Path> promoteFiles(List<File> files){


This needs a better name. maybe filesToPaths? It's also untested and undocumented.

lbergelson · 2017-12-18T16:22:46Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

     * @param wrapper to wrap the input stream in, may be used to implement caching or prefetching, etc
-     * @return
-     * @throws IOException
+     * @return An inputStream appropriately created from uri and conditionally wrapped with wrapper (only in certain cases


👍 for updating the doc. missing a closing )

lbergelson · 2017-12-18T16:39:27Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

+            inputStream = getURLHelper(new URL(uri)).openInputStream();
+        } else if (scheme.isEmpty()) {
+            File file = new File(uri);
+            inputStream = Files.newInputStream(file.toPath());


I assume named pipes can be referred to as either a file or a path, which means that we'd need a different mechanism to check for them. We don't ever do a check for isRegularFile which is I think what we'd need to do.

lbergelson · 2017-12-18T16:43:05Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

            throws IOException {

+        // will be empty string if there's no scheme
+        final String scheme = Optional.ofNullable(IOUtil.getScheme(uri)).orElse("");


getScheme doesn't return a null, there's no need for this complex dance. It's also definitely preferable to just have a ternary expression that checks for null instead of creating a new nullable just to check if it's empty.

look at getScheme:

* @return The scheme component of this URI, * or {@code null} if the scheme is undefined */

if I use a ternary, I need to either add a line of code (to save the result of getSceme) or call it twice....I'll do it if you insist, but I thought that this is a better solution.

You're not calling uri.get scheme, you're calling your own version that you added, IOUtil.getScheme() which returns true/false.

Ternary or not doesn't really matter, it's just kind of ugly to create a new wrapper object just to hide the null check. I see your point about needing to call it twice or store it in a temp variable. In any case, it feels like something is weird here. You never actually use the scheme you get, you just check if it's empty. Isn't calling IOUtil.hasScheme, which is what was originally done here correct?

lbergelson · 2017-12-18T16:47:08Z

src/main/java/htsjdk/tribble/util/ParsingUtils.java

+            inputStream = getURLHelper(new URL(uri)).openInputStream();
+        } else if (scheme.isEmpty()) {
+            File file = new File(uri);
+            inputStream = Files.newInputStream(file.toPath());


I'm not sure how much we care if we return a SeekablePathStream for a file, vs a simple input stream. They should have similar performance I hope, and this function only guarantees an input stream. It is weird though to return different types depending on how you write the path though and it probably has some weird knock on effects. That said, it's not a change from how it was set up before is it?

Why did you change from the previous order and using hasScheme? It seems like you added a new local variable and some additional logic and then only use it to do what the simpler check accomplished?

lbergelson · 2017-12-18T16:52:27Z

src/main/java/htsjdk/variant/vcf/VCFFileReader.java

+     *
+     * @deprecated in favor of {@link VCFFileReader#isBCF(Path)}
+     */
+    @Deprecated


We've typically been avoiding deprecating the file methods so far, but it would be nice to move away from them. Probably fine to do so here.

That said, if we deprecate them, we should remove uses of them except for direct tests. Lots of test code is using the File constructors.

That seems like a larger effort than this PR is meant to do...reversing the deprecations...

Perhaps this should be on the roadmap for htsjdk3?

Is there any plan appart of #520 to implement htsjdk3?

yfarjoun · 2017-12-20T16:10:47Z

not sure what you're asking me to do about the isRegularFile...can you elaborate?

yfarjoun · 2017-12-20T21:38:55Z

another look @lbergelson ?

lbergelson · 2017-12-20T21:51:06Z

@yfarjoun A fwe new comments. What I meant with regular file is that we don't ever check for fifos so we're handling them weirdly to begin with, your pr doesn't make it worse as far as I can tell. We should do it correctly, but I think that's a different pr that should hit a lot more places in the codebase.

yfarjoun · 2017-12-21T13:48:21Z

@lbergelson ?

lbergelson

👍

yfarjoun · 2017-12-21T20:50:41Z

🎉

magicDGS reviewed Oct 27, 2017

View reviewed changes

yfarjoun force-pushed the yf_vcf_from_path branch from 26e7194 to 352d92b Compare November 15, 2017 16:33

pshapiro4broad reviewed Nov 15, 2017

View reviewed changes

magicDGS reviewed Nov 16, 2017

View reviewed changes

pshapiro4broad reviewed Nov 16, 2017

View reviewed changes

lindenb reviewed Nov 17, 2017

View reviewed changes

pshapiro4broad reviewed Nov 17, 2017

View reviewed changes

yfarjoun changed the title ~~Yf vcf from path~~ Enable VcfFileReader and other classes to use Path Nov 18, 2017

samtools deleted a comment from jacarey Nov 18, 2017

magicDGS reviewed Nov 21, 2017

View reviewed changes

yfarjoun added 12 commits November 22, 2017 09:49

- removed references to gsutil and replaced with jimfs as requested i…

8164a91

…n review.

- having trouble with paths...trying something

e6a35b7

- oops forgot to add files to git

923e68f

-removed un-needed readline function.

333a002

- removed gs references from fofn

478cc02

- responding to review comments.

07036a2

- responding to more review comments.

a9108e8

- fixing code and added a test with spaces

de11435

- replacing newly added VCFPathReader with Path constructors in VCFFi…

749c5e7

…leReader and deprecating the File versions

- more review comments

38cf4e0

- Fix recursive, non-terminating call in SamSequenceDictionaryExtractor

5ddf106

-optimizing imports

2d26b7a

yfarjoun force-pushed the yf_vcf_from_path branch from e76de8d to 2d26b7a Compare November 22, 2017 14:50

- refactored to remove commit that was put into another PR.

ae8ea3e

- responding to review comments.

- oops

6fb660a

yfarjoun assigned lbergelson Dec 15, 2017

lbergelson requested changes Dec 18, 2017

View reviewed changes

- responding to review comments.

5c7089e

- responding to more review comments.

51e2939

lbergelson approved these changes Dec 21, 2017

View reviewed changes

lbergelson merged commit 80122b9 into master Dec 21, 2017

lbergelson deleted the yf_vcf_from_path branch December 21, 2017 16:24

Enable VcfFileReader and other classes to use Path #1018

Enable VcfFileReader and other classes to use Path #1018

Conversation

yfarjoun commented Oct 26, 2017 • edited Loading

Description

Checklist

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

yfarjoun commented Nov 15, 2017

codecov-io commented Nov 15, 2017 • edited Loading

Codecov Report

yfarjoun commented Nov 15, 2017

pshapiro4broad Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshapiro4broad Nov 16, 2017 • 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

pshapiro4broad Nov 15, 2017 • 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

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

pshapiro4broad Nov 16, 2017 • 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

yfarjoun commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshapiro4broad Nov 17, 2017 • 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

yfarjoun commented Nov 18, 2017

magicDGS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yfarjoun commented Oct 26, 2017 •

edited

Loading

codecov-io commented Nov 15, 2017 •

edited

Loading

pshapiro4broad Nov 15, 2017 •

edited

Loading

pshapiro4broad Nov 16, 2017 •

edited

Loading

pshapiro4broad Nov 15, 2017 •

edited

Loading

pshapiro4broad Nov 16, 2017 •

edited

Loading

pshapiro4broad Nov 17, 2017 •

edited

Loading