New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

URI and Path specifier classes for input/output resources. #34

Merged

lbergelson merged 3 commits into samtools:master from cmnbroad:cn_uri_spec

Feb 13, 2019

Collaborator

cmnbroad commented Oct 29, 2018

Not sure if there are better names or package locations.


          URI and Path specifier classes for input/output resources.

fa89394

Collaborator Author

cmnbroad commented Oct 29, 2018 •

edited

Loading

Arghh... the tests fail on Windows because I hardcoded "/" in my test path names. Are we really supporting Windows ?

cmnbroad commented

View reviewed changes

core/src/test/java/org/htsjdk/core/utils/PathSpecifierUnitTest.java Outdated

+                              // URIs with an embedded fragemnt delimiter ("#"); if the file scheme is included, the rest of the path
+                              // must already be escaped; ifno file scheme is included, the path will be escaped by the URI class
+                              {"../data/utils/testDirWith#InName/testTextFile.txt", "Test file."},
+                              {"file:///" + getCWD() + "../data/utils/testDirWith%23InName/testTextFile.txt", "Test file."},

Collaborator Author

cmnbroad Oct 29, 2018 •

edited

Loading

Need to resolve how to reference these paths without "..". Need to resolve #19.

Member

magicDGS Oct 29, 2018

I think that you can use Paths.get(getCWD() + "../data/utils/testDirWith%23InName/testTextFile.txt").toUri().toString(), if the problem is with Windows. I can test if you want locally and let you know.

Collaborator Author

cmnbroad Oct 31, 2018

There are several problems here; the original one about where/how we should store test data, and the Windows one where the path separator is different. The latter has two parts 1) how we specify paths in tests 2) How we test Windows paths (UNC paths, dos drive specifiers, encoded names with embedded spaces etc).

Also, I know you're not yet convinced we need these classes, but for these tests in particular, I want to make sure the raw strings are passed directly to the classes being tested, not strings that have been pre-processed by Paths and URI, since I'm trying to test that the classes themselves do this correctly.

Collaborator Author

cmnbroad Oct 31, 2018

Also, there is the issue that Windows paths (with forward slashes) don't appear to be valid URIs ?

Member

magicDGS commented Oct 29, 2018

@cmnbroad - I would like to support Windows, also because of the hardcoded "/" that are all around htsjdk2. One of the major points of supporting Windows is that, as we are using java, it will be nice to be multi platform (and I have a project in mind for a desktop app that I will test mostly in Windows). There is no much to do to support windows if we don't use hardcoded "/", as far as we are using java.nio.Path.

By the way, I will start the review now.

magicDGS requested changes

View reviewed changes

Member

magicDGS left a comment

I am a bit against the class in the first place because this looks more like a way to distinguish when a passed String should be considered a path or something else that should be handled with a different reader (record-provider) implementation. I think that handle URI-specific thinks like this should be done at the reader-factory level; we can provide a factory for GenomicDB that takes the URI with the scheme and handle a Path as if it has it (as a different artifact or in a different repository). Then GATK or other framework that requires to provide a common implementation for handling resources could do their own URI class, which I think that it's out-of-scope for htsjdk3 itself.

core/src/main/java/org/htsjdk/core/api/PathURI.java Outdated

		@@ -0,0 +1,86 @@
		package org.htsjdk.core.api;

Member

magicDGS Oct 29, 2018

I would say that a better place will be org.htsjdk.core.api.io, to keep things more organized.

Collaborator Author

cmnbroad Nov 14, 2018

Done.

core/src/main/java/org/htsjdk/core/api/PathURI.java Outdated

+              /**
+               * Interface representing htsjdk-next input/output resources as URIs.
+               */
+              public interface PathURI {

Member

magicDGS Oct 29, 2018

As this will represent a resource, what's about IOResource? I'm not sure if it is better, but I don't like the PathURI name...

Collaborator Author

cmnbroad Oct 31, 2018 •

edited

Loading

Agreed, I don't love the names. IOResource might be an improvement - we'd still need a name for the default implementation class (currently called PathSpecifier for lack of a better name.

Collaborator Author

cmnbroad Nov 14, 2018

Changed to IOResource; still not sure about PathSpecifier.

core/src/main/java/org/htsjdk/core/api/PathURI.java Outdated

+                  /**
+                   * @return true if this URI has a scheme that has an installed NIO file system provider. This does not
+                   * guarantee the URI can be converted into a {@code java.nio.file.Path}, since the URI can be syntactically
+                   * valid, and specify a valid file system provider, but still fail to be semantically meaningful.

Member

magicDGS Oct 29, 2018

I prefer to have properly formatted javadocs. Most of the information should be at the begining, and the @return tag should contain just the information about the return value (in this case, {@code true} if the URI has a installed NIO filesystem provider; {@code false} otherwise).

Collaborator Author

cmnbroad Nov 14, 2018

Done.

core/src/main/java/org/htsjdk/core/api/PathURI.java Outdated

+                   * guarantee the URI can be converted into a {@code java.nio.file.Path}, since the URI can be syntactically
+                   * valid, and specify a valid file system provider, but still fail to be semantically meaningful.
+                   */
+                  boolean isNIO();

Member

magicDGS Oct 29, 2018

Similarly to the new disq, I don't like this isNio name in the method. I prefer something like hasFileSystem or throw a meaningful exception if a Path could not be retrieved to be catched by client code...

Collaborator Author

cmnbroad Oct 31, 2018

Clients can ignore this and do exactly as you describe (just call toPath, which will throw).

core/src/main/java/org/htsjdk/core/api/PathURI.java Outdated

+                   *
+                   *  hdfs://namenode/to/file
+                   *
+                   * The current implementation returns false for these cases (toPath will fail, getInvalidPathReason

Member

magicDGS Oct 29, 2018

There is no current implementation.

Collaborator Author

cmnbroad Nov 14, 2018

Fixed.

core/src/main/java/org/htsjdk/core/utils/PathSpecifier.java Outdated

+                  @Override
+                  public OutputStream getOutputStream() {
+                      if (!isPath()) {
+                          throw new HtsjdkException(getToPathFailureReason());

Member

magicDGS Oct 29, 2018

No raw HtsjdkException

Collaborator Author

cmnbroad Oct 31, 2018

Yeah - perhaps we need a separate discussion about how we want to partition the exception namespace. #36.

core/src/main/java/org/htsjdk/core/utils/PathSpecifier.java Outdated

+                      try {
+                          return Files.newOutputStream(resourcePath);
+                      } catch (IOException e) {
+                          throw new HtsjdkException(String.format("Could not open output stream for %s (as URI %s)", getRawInputString(), getURIString()), e);

Member

magicDGS Oct 29, 2018

No raw HtsjdkException

core/src/test/java/org/htsjdk/core/utils/PathSpecifierUnitTest.java

		@@ -0,0 +1,254 @@
		package org.htsjdk.core.utils;

		//TODO: NAMING: is there any useful distinction between Unit/Integration tests in this repo ?

Member

magicDGS Oct 29, 2018

I think that we should set this naming conventions, as we did not though about that yet.

core/src/test/java/org/htsjdk/core/utils/PathSpecifierUnitTest.java Outdated

+                              {"file:project/gvcf-pcr/23232_1#1/1.g.vcf.gz",  "file:project/gvcf-pcr/23232_1#1/1.g.vcf.gz", true, false},
+                              {"file:/project/gvcf-pcr/23232_1#1/1.g.vcf.gz", "file:/project/gvcf-pcr/23232_1#1/1.g.vcf.gz", true, false},
+                              {"gendb://somegdb",             "gendb://somegdb", false, false},

Member

magicDGS Oct 29, 2018

I guess that this is the main reason to have the class in the first place (see the overall comment).

Collaborator Author

cmnbroad Oct 31, 2018

See my overall comment describing the reasons for proposing these classes.

data/utils/testDirWith#InName/testTextFile.txt

		@@ -0,0 +1,2 @@
		Test file.

Member

magicDGS Oct 29, 2018

We should also decide on the naming conventions for test data. Also, as I commented before, it will be nice that the test-resources are located in the src to be packaged in the test artifact; otherwise, test won't run if the test jar is isolated.

magicDGS reviewed

View reviewed changes

build.gradle

@@ @@ -30,6 +30,9 @@ subprojects { @@
                   dependencies {
                       testCompile "org.testng:testng:6.14.3"
+                      testCompile "com.google.jimfs:jimfs:1.1"
+                      compile 'commons-io:commons-io:2.5'

Member

magicDGS Oct 29, 2018

I forgot to mention: is this needed in this PR?

Collaborator Author

cmnbroad Oct 31, 2018

Good catch - its left over from previous changes and can be removed.

cmnbroad mentioned this pull request

Resolve how to handle issues with supporting Windows #35

Open

Collaborator Author

cmnbroad commented Oct 30, 2018

@magicDGS thanks for the review comments - I'll go through these individually, but I created #35 to discuss issues related to Windows since I think there are likely some other issues to resolve.

droazen requested a review from lbergelson

October 30, 2018 15:16

droazen assigned lbergelson

Collaborator Author

cmnbroad commented Oct 31, 2018 •

edited

Loading

I think there is value in having an interface thats more strongly typed than String, and more general than File or even Path, for specifying input/output resources in the new interfaces. Using URI as the specifier syntax is compatible with regular old local file paths, and allows for extensibility via query params and custom schemes. The classes proposed in this PR provide both an interface and a default concrete implementation that can handle most cases (local files and other NIO provider-based file systems), but which can also be extended via overrides to allow consumers to provide custom resolution to streams.

cmnbroad added 2 commits

November 13, 2018 15:21


          Changes for Windows and test refactoring.

e60e358


          Review comments.

46f93bc

magicDGS mentioned this pull request

Add docs project for technical documentation #42

Open

Member

magicDGS commented Nov 14, 2018

I have a proposal for the IOResource and how to provide other implementations by downstream projects. I don't know the rest of @samtools/htsjdk-next-maintainers, but I prefer to keep it simpler (e.g., only java.nio.Path implementation) and allow better extensibility. The implementation in this PR looks a bit cumbersome.

In #42 I included a proposal; maybe we can discuss on that PR or just here. I did not add to the design a method for the getting the raw/string representation, but actually that could make sense to be able to retrieve it. I will copy-paste the generated HTML from #42 here to show what I propose.

Member

magicDGS commented Nov 14, 2018

Sorry, I could only get a temporary PDF because I had some problems with the computer. But it should be enough for the discussion: IOResourceProvider.pdf

Member

lbergelson commented Nov 16, 2018

@magicDGS I'm sorry I haven't gotten a chance to comment on this properly yet.

I think having a fairly lightweight interface that is essentially a more convenient URI is a good thing to have. My thought is that we have essentially 2 different things that need to be referred to by uri's of some sort.

is a file system like thing that can be opened and read as a block of bytes. I think we should support these with pluggable nio Path providers.
is something isn't readable as a block of bytes, these are things like SRI accessions and GenomicsDB. We will want another layer of plugins that can understand these.
I think we want an interface below these that can be plugged into either layer as an identifier. Your proposal seems closer to what I'm imaging as the layer above this interface, the reader layer which will support both pluggable file systems and pluggable readers. Particularly, I think the concept of companion files is a really important one that has been poorly implemented in htsjdk currently, but I think we still need a path representation for a single file.

magicDGS mentioned this pull request

Change README to AsciiDoc #41

Closed

Member

magicDGS commented Nov 29, 2018

@lbergelson - I am designing (like in my other comment, in my mind by now due to lack of time for personal reasons) the whole interface for IO in a similar way as #42 (that was just a proof-of-concept on how to write and propose this kind of things in a live document in GitHub, which will be nice for several reasons that we can discuss in Slack):

Low-level IO: as the one that I described in Add docs project for technical documentation #42. This is the layer which contains how to read byte-data from different URIs or path. As I explain there, the main idea is to have a factory which provide different kind of resources. We can provide the one for java.nio.Path in the core, and then have other projects (in this repo or somewhere else) to provide on top of them a resource for GenomicsDB URIs or SRA accessions (or even configuration files with bundled data - resource/index/etc). For the purpose of an user, this is the layer that we don't expect to be implemented outside this couple of examples (or a new resource format, I don't know, based on HDF5 or something that crazy).
Low-level record: by now, just an Record marker interface (I am thinking how to abstract record metadata properly for advance usage). This will be the base class for concrete records, such reads or variants. I am against using the Locatable as a record, because it might be used for other purposes too (e.g., FASTA-records, which are not locating themselves on themselves). We expect that this is just used as a marker but not really containing too much logic - it is the bridge between the low-level IO and the high-level IO to faciltate its implementation. This includes the codec interface, that requires only the concrete implementation.
High-level IO: layer between the low-level IO and the low-level record. That will be like a record provider, including factories and factory-builders. This will be the main extensible point for API users, that could implement their own way to translate the low-level IO into records: from the byte-stream, parse the FASTQ/BED/SAM/BAM/CRAM or whatever format.
High-level record. This are the specialized records that should be implemented, including the codecs.

The idea behind this design is:

An user implementing a new way of reading byte-data, should only implement a new resource that pass a stream to the codecs that is understandable (unless specific parsing is required).
An user interested in a new kind of file should only implement the record/codec and register it. They should not care about the byte data at all and from where it comes, just for parsing (except special cases).
Both could be combined for more complex data sources.

yfarjoun reviewed

View reviewed changes

core/src/main/java/org/htsjdk/core/utils/PathSpecifier.java

+              public class PathSpecifier implements IOResource, Serializable {
+                  private static final long serialVersionUID = 1L;
+                  private final String    rawInputString;     // raw input string provided by th user; may or may not have a scheme

yfarjoun Jan 16, 2019

Suggested change

      
                private final String    rawInputString;     // raw input string provided by th user; may or may not have a scheme
          
                private final String    rawInputString;     // raw input string provided by the user; may or may not have a scheme

yfarjoun reviewed

View reviewed changes

core/src/main/java/org/htsjdk/core/utils/PathSpecifier.java

+                  private final String    rawInputString;     // raw input string provided by th user; may or may not have a scheme
+                  private final URI       uri;                // working URI; always has a scheme (assume "file" if not provided)
+                  private transient Path  cachedPath;         // cache the Path associated with this URI if its "Path-able"

yfarjoun Jan 16, 2019

Suggested change

      
                private transient Path  cachedPath;         // cache the Path associated with this URI if its "Path-able"
          
                private transient Path  cachedPath;         // cache the Path associated with this URI if it's "Path-able"

lbergelson approved these changes

View reviewed changes

Member

lbergelson left a comment

In the name of progress I approve this PR.

magicDGS mentioned this pull request

Proposal implementation of IOResource #45

Open

lbergelson merged commit 5924ef3 into samtools:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet