Add HtsgetBAMFileReader #1494

andersleung · 2020-07-23T17:12:54Z

Description

Add a new SamReader type that is able to request reads from HTSget sources, as well as classes defining the content of an HTSget request and response. This change allows consumers of the htsjdk library to use HTSget sources like any other type of reads source.

Things to think about before submitting:

Make sure your changes compile and new tests pass locally.
Add new tests or update existing ones:
- A bug fix should include a test that previously would have failed and passes now.
- New features should come with new tests that exercise and validate the new functionality.
Extended the README / documentation, if necessary
Check your code style.
Write a clear commit title and message
- The commit message should describe what changed and is targeted at htsjdk developers
- Breaking changes should be mentioned in the commit message.

codecov-commenter · 2020-07-23T21:09:41Z

Codecov Report

Merging #1494 into master will increase coverage by 0.046%.
The diff coverage is 72.156%.

@@               Coverage Diff               @@
##              master     #1494       +/-   ##
===============================================
+ Coverage     69.279%   69.325%   +0.046%     
- Complexity      8764      8888      +124     
===============================================
  Files            590       601       +11     
  Lines          34755     35433      +678     
  Branches        5800      5901      +101     
===============================================
+ Hits           24078     24564      +486     
- Misses          8386      8538      +152     
- Partials        2291      2331       +40

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/htsjdk/samtools/SamReaderFactory.java	`65.487% <18.750%> (-3.381%)`	`7.000 <0.000> (ø)`
src/main/java/htsjdk/samtools/SamReader.java	`79.348% <25.000%> (-2.470%)`	`0.000 <0.000> (ø)`
.../util/htsget/HtsgetMalformedResponseException.java	`28.571% <28.571%> (ø)`	`1.000 <1.000> (?)`
src/main/java/htsjdk/samtools/BAMFileReader.java	`69.501% <50.000%> (+1.036%)`	`50.000 <1.000> (-2.000)`	⬆️
...rc/main/java/htsjdk/samtools/SamInputResource.java	`65.432% <53.846%> (-2.451%)`	`20.000 <3.000> (+3.000)`	⬇️
...ava/htsjdk/samtools/util/htsget/HtsgetRequest.java	`61.905% <61.905%> (ø)`	`34.000 <34.000> (?)`
...dk/samtools/util/SAMRecordPrefetchingIterator.java	`74.667% <74.667%> (ø)`	`13.000 <13.000> (?)`
...main/java/htsjdk/samtools/HtsgetBAMFileReader.java	`75.799% <75.799%> (ø)`	`24.000 <24.000> (?)`
...sjdk/samtools/util/htsget/HtsgetErrorResponse.java	`80.000% <80.000%> (ø)`	`4.000 <4.000> (?)`
src/main/java/htsjdk/samtools/QueryInterval.java	`71.429% <90.909%> (+4.762%)`	`24.000 <5.000> (+5.000)`
... and 22 more

lbergelson

@andersleung I promised to get this done today but other stuff came up and then it got late. Here's my first round of comments but I'll probably have more tomorrow. It looks generally good so far but I think we want to change some of the handling of the Input resources.

scripts/start-htsget-test-server.sh

src/main/java/htsjdk/samtools/HtsgetBAMFileReader.java

lbergelson · 2020-07-29T20:31:02Z

src/main/java/htsjdk/samtools/HtsgetBAMFileReader.java

+     * @return false, since htsget sources never have indices
+     */
+    @Override
+    public boolean hasIndex() {


This is going to be problematic since this method is used in 2 different ways. The first and most important is to mean "can I query on this reader or do I have to iterate over it". Obviously htsget supports queries without the index. The second way this is used though is "let me check if I can locate the index for this file so I can perform some operation on it. That's very rarely used but it is used internally by the sam reader to guard a check to getIndex.

We might want to add a new method "isQueryable" or something like that to the interface so we can split that concept up.

returning true here will be a lie but will work better with almost everything. we'd have to make getIndex return null then I think.

Either way it's unfortunate.

src/main/java/htsjdk/samtools/HtsgetBAMFileReader.java

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

src/main/java/htsjdk/samtools/SamInputResource.java

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

lbergelson · 2020-07-30T04:50:15Z

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

@@ -523,6 +548,11 @@ void applyTo(final CRAMFileReader underlyingReader, final SamReader reader) {
            void applyTo(final SRAFileReader underlyingReader, final SamReader reader) {
                underlyingReader.enableIndexCaching(true);
            }
+
+            @Override
+            void applyTo(final HtsgetBAMFileReader underlyingReader, final SamReader reader) {


This design is so crufty. It seems like someone started refactoring it to use ReaderImplementation so we don't need these custom handlers here as well but then never actually did it. (Just me venting... nothing to do here...)

lbergelson · 2020-07-30T04:51:14Z

src/main/java/htsjdk/samtools/util/htsget/HtsgetErrorResponse.java

+
+/**
+ * Class allowing deserialization from json htsget error response
+ */


Note to self: stopping here for now.

src/main/java/htsjdk/samtools/util/htsget/HtsgetErrorResponse.java

lbergelson

@andersleung More comments. I think I got everything this time.

src/main/java/htsjdk/samtools/util/htsget/HtsgetErrorResponse.java

src/main/java/htsjdk/samtools/util/htsget/HtsgetRequest.java

src/test/java/htsjdk/samtools/HtsgetBAMFileReaderTest.java

lbergelson · 2020-07-30T20:17:57Z

src/test/java/htsjdk/samtools/HtsgetBAMFileReaderTest.java

+import java.io.IOException;
+import java.net.URI;
+
+public class HtsgetBAMFileReaderTest extends HtsjdkTest {


It would be good to run these tests with a variety of settings on the htsget reader. Particularly I'd like to see them with async on for the htsget reader since it seems plausible that there could be potential issues there. You could add a datprovider that has multiple readers.

The readers also need to be closed at the end.

Typically I'm in favor of not reusing the same objects for multiple tests and generating them fresh each time, it helps avoid test ordering problems or if 1 test puts the objects into a broken state. This should be OK though.

src/test/java/htsjdk/samtools/SamReaderFactoryTest.java

lbergelson · 2020-07-30T20:24:18Z

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

+public class HtsgetResponseUnitTest extends HtsjdkTest {
+    @Test
+    public void testDeserialization() {
+        final String respJson = "{\"htsget\":{\"format\":\"BAM\",\"urls\":[{\"url\":\"data:application/vnd.ga4gh.bam;base64,QkFNAQ==\",\"class\":\"header\"},{\"url\":\"https://htsget.blocksrv.example/sample1234/header\",\"class\":\"header\"},{\"url\":\"https://htsget.blocksrv.example/sample1234/run1.bam\",\"headers\":{\"Authorization\":\"Bearer xxxx\",\"Range\":\"bytes=65536-1003750\"},\"class\":\"body\"},{\"url\":\"https://htsget.blocksrv.example/sample1234/run1.bam\",\"headers\":{\"Authorization\":\"Bearer xxxx\",\"Range\":\"bytes=2744831-9375732\"},\"class\":\"body\"}],\"md5\":\"blah\"}}";


I can't wait until java adds multiline strings...

src/test/java/htsjdk/samtools/SamReaderFactoryTest.java

lbergelson

@andersleung I think this is looking good overall, but I think the prefetcher could use some changes. I think there are a number of race conditions and generally the error handling needs to be improved.

lbergelson · 2020-08-06T15:07:35Z

scripts/htsget-scripts/README.md

@@ -0,0 +1,5 @@
+This folder contains scripts and files necessary for starting a local htsget reference server so that htsget functionality can be tested.


Thank you, this is helpful

src/main/java/htsjdk/samtools/HtsgetBAMFileReader.java

src/main/java/htsjdk/samtools/SamReader.java

src/main/java/htsjdk/samtools/HtsgetBAMFileReader.java

src/main/java/htsjdk/samtools/SamInputResource.java

src/main/java/htsjdk/samtools/util/htsget/SAMRecordPrefetchingIterator.java

src/test/java/htsjdk/samtools/HtsgetBAMFileReaderTest.java

src/main/java/htsjdk/samtools/util/htsget/SAMRecordPrefetchingIterator.java

…ning code

lbergelson

@andersleung A few minor comments. Looks good to me after those are resolved.

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

lbergelson · 2020-08-13T19:47:38Z

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

+ * Note that this implementation is not synchronized. If multiple threads
+ * access an instance concurrently, it must be synchronized externally.
+ */
+public class SAMRecordPrefetchingIterator implements CloseableIterator<SAMRecord> {


This looks good to me. I have two small comments. I don't see any concurrency issues so hopefully that means that there aren't any :)

This is nicer than the last implementation.

lbergelson · 2020-08-13T19:47:49Z

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

+        if (this.backgroundThread == null) return;
+        /*
+         If prefetch thread is interrupted while awake and before acquiring permits, it will either acquire the permits
+         and pass through to the next case, or check interruption status before sleeping then exit immediately


Thanks for the detailed explanation.

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

lbergelson · 2020-08-13T19:59:57Z

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

+
+import java.util.stream.IntStream;
+
+public class SAMRecordPrefetchingIteratorTest extends HtsjdkTest {


Thanks for writing these tests!

lbergelson · 2020-08-13T22:02:03Z

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

@@ -65,6 +67,7 @@ private void prefetch() {
                // InterruptedException is expected if the iterator is being closed
                return;
            } catch (final Throwable t) {
+                t.printStackTrace();


I think this should only be printed in the case of an Error, not in the normal case of an Exception.

lbergelson · 2020-08-14T16:14:23Z

@andersleung 👍

co-authored-by: Florian Reisinger <florian.reisinger@unimelb.edu.au>

andersleung added 7 commits July 23, 2020 13:06

WIP add HtsgetBAMFileReader

b05fae5

sudo docker commands when starting htsget test server

90de6be

Add shebang to script

d14460b

sudo execute script

08fcefa

Make start script executable

6f83e2d

Fix SamReaderFactoryTest

2146521

Fix based on spotBugs, delete useless file

9cf6d7e

andersleung added 2 commits July 27, 2020 16:53

Add further tests

36544de

Add tests

59bf032

lbergelson reviewed Jul 30, 2020

View reviewed changes

src/main/java/htsjdk/samtools/util/htsget/HtsgetErrorResponse.java Show resolved Hide resolved

lbergelson requested changes Jul 30, 2020

View reviewed changes

andersleung added 6 commits August 4, 2020 14:57

Respond to PR comments, add aynchronous prefetching support

da99416

Fix tests, fix SAMRecordPrefetchingIterator

e9329ef

Add isQueryable() method to PrimitiveSamReader and SamReader

f2a91f4

Fix SAMRecordPrefetchingIterator, update tests

deae5df

Remove unused code and imports

3aaaa0a

Remove debug println statement

121de0f

lbergelson requested changes Aug 7, 2020

View reviewed changes

Rework SAMRecordPrefetchingIterator, add tests, clean up resource ope…

042f50b

…ning code

andersleung force-pushed the add-htsget-functionality branch from 58797b4 to 042f50b Compare August 13, 2020 19:15

lbergelson approved these changes Aug 13, 2020

View reviewed changes

Respond to comments

ef128f3

lbergelson reviewed Aug 13, 2020

View reviewed changes

Only print stack trace for Errors

0bacdf8

lbergelson merged commit e803eea into samtools:master Aug 14, 2020

lbergelson added the GA4GH Collaboration with GA4GH label Sep 1, 2020

brainstorm added a commit to umccr/igv that referenced this pull request Sep 20, 2020

WIP htsget client implementation leveraging samtools/htsjdk#1494

2d6a8a6

co-authored-by: Florian Reisinger <florian.reisinger@unimelb.edu.au>

This was referenced Jun 15, 2021

HtsGetVCFReader implementation? #1555

Open

htsget variants middleware igvteam/igv#983

Closed

jmarshall mentioned this pull request Jul 29, 2021

Standardize htsget:// scheme on spec samtools/hts-specs#581

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HtsgetBAMFileReader #1494

Add HtsgetBAMFileReader #1494

andersleung commented Jul 23, 2020

codecov-commenter commented Jul 23, 2020 •

edited

lbergelson left a comment

lbergelson Jul 29, 2020

lbergelson Jul 30, 2020

lbergelson Jul 30, 2020

lbergelson left a comment

lbergelson Jul 30, 2020

lbergelson Jul 30, 2020

lbergelson left a comment

lbergelson Aug 6, 2020

lbergelson left a comment

lbergelson Aug 13, 2020

lbergelson Aug 13, 2020

lbergelson Aug 13, 2020

lbergelson Aug 13, 2020 •

edited

lbergelson commented Aug 14, 2020

		@@ -0,0 +1,5 @@
		This folder contains scripts and files necessary for starting a local htsget reference server so that htsget functionality can be tested.


		import java.util.stream.IntStream;

		public class SAMRecordPrefetchingIteratorTest extends HtsjdkTest {

Add HtsgetBAMFileReader #1494

Add HtsgetBAMFileReader #1494

Conversation

andersleung commented Jul 23, 2020

Description

Things to think about before submitting:

codecov-commenter commented Jul 23, 2020 • edited

Codecov Report

lbergelson 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

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson 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

lbergelson Aug 13, 2020 • edited

Choose a reason for hiding this comment

lbergelson commented Aug 14, 2020

codecov-commenter commented Jul 23, 2020 •

edited

lbergelson Aug 13, 2020 •

edited