Multi ranges #94
Multi ranges #94
Conversation
|
I see that this is in the roadmap for |
|
Hm, yes, this extension has some drawbacks though, first and foremost, it is impossible to detect whether the server actually provides support for multipart responses. I want to merge this soonish. |
Common interface for all sources: - all sources are now called with "slices" - allow signals to abort the reading process
Fixing usage in readers
Adding LRU cache dependency
Missing imports Wrong header function call
Adding abort tests
| const groups = this.groupBlocks(this.blockIdsToFetch); | ||
|
|
||
| // start requesting slices of data | ||
| const groupRequests = this.source.fetch(groups, signal); |
There was a problem hiding this comment.
I think we need handling analogous here to
https://github.com/geotiffjs/geotiff.js/pull/182/files#diff-908b5c4d8c419c1ab5cd8a06a28e9e47a8e1d1b9b49c8309198ab1e663d54f2fR234
and
for the AbortSignal because multiple fetch calls can request the same block and then independently abort them. Without the handling, it seems like an application can lock up like this because they are requesting blocks with one fetch that another fetch already cancelled but never deleted from the blockRequests cache. Thus the fetch that wants the block thinks it is possible to get it but it is in fact not because the request was never satisfied on account of the other fetch cancelling it. I am more than happy to make a PR into this PR to handle that.
There was a problem hiding this comment.
Sorry if this is confusing! Can try to give a better explanation :)
There was a problem hiding this comment.
No, I think I understand the issue, thanks for bringing that up! It basically means that we have some proper error handling at the block level. But I think, this is just a matter of try/catch 🤔
What is the app you are using? Maybe I can use that to debug the issue.
There was a problem hiding this comment.
@ilan-gold Could you try again with 07ed101? I think this should solve the issue.
There was a problem hiding this comment.
@constantinius I'll try it now - I need to get on European-time so we can get through this faster.......need that vaccine....
In any case, I am not sure I have ever shared this with you (I think I have but maybe not?) but we are using geotiff.js extensively in https://github.com/hms-dbmi/viv. If you run the dev instructions, after npm start you'll be using geotiff.js if your browser image_url parameter has the .tiff ending. I would recommend http://localhost:8080/?image_url=https://viv-demo.storage.googleapis.com/Vanderbilt-Spraggins-Kidney-MxIF.ome.tif because it is not LZW compressed (we are currently partly on my branch to support that we are using Trevor's LZW library: see #172).
You'll also need to change what branch you're using in the package.json but other than that, you should be able to edit the node_modules/geotiff.js/src directly and get live updates.
There was a problem hiding this comment.
@ilan-gold Thanks for the response!
Unfortunately I did not get the viv client to work, I think I'm missing some dependency:
$ npm start
> @hms-dbmi/[email protected] start
> concurrently "npm run build:w" "cd avivator && npm start"
sh: concurrently: command not found
npm ERR! code 127
npm ERR! path /Users/fabian/dev/viv
npm ERR! command failed
npm ERR! command sh -c concurrently "npm run build:w" "cd avivator && npm start"
npm ERR! A complete log of this run can be found in:
npm ERR! /Users/fabian/.npm/_logs/2021-02-22T17_30_31_194Z-debug.logI think I understand the issue you described, and the way I'm trying to ship around it is to first request the initial set of blocks or use the ones already being requested. If any of the already being requested failed, and a signal was the cause and the signal is not the current one then I will restart just those blocks and wait for them.
This i done in dd3b36f.
Do you think this is a viable strategy? I'm a bit hesitant to juggle around signals too much, apart from checking equality and status, but not combining signals and the like. Please let me know your thoughts on that.
There was a problem hiding this comment.
I did get a bit further with viv and now get this error:
[1] ERROR in ./src/utils.js 2:0-54
[1] Module not found: Error: Can't resolve 'geotiff' in '/Users/fabian/dev/viv/avivator/src'
[1] @ ./src/index.js 8:0-41 43:19-33
[1]
[1] ERROR in ../dist/index.js 11:0-44
[1] Module not found: Error: Can't resolve 'geotiff' in '/Users/fabian/dev/viv/dist'
[1] @ ./src/Avivator.js 8:0-87 98:19-34 337:82-98 353:40-62
[1] @ ./src/index.js 7:0-34 47:40-48 56:38-46
[1]
[1] webpack 5.3.2 compiled with 2 errors in 10071 ms
[1] ℹ ｢wdm｣: Failed to compile.
There was a problem hiding this comment.
@constantinius I had a bit of trouble installing the branch too, which I had never had trouble with in the past. Only the cli npm install https://github.com/geotiffjs/geotiff.js#07ed1013 seemed to work for me in the top-level of the repo. Another option would just be to copy and paste the contents of src from this branch into the node_modules/geotiff/src since Viv uses src directly. I'll test out your recent changes now.
There was a problem hiding this comment.
(Let me know if you need more help, I'm happy to assist - if it seems like a bug is causing this, feel free to open an issue)
We have some dev docs: https://github.com/hms-dbmi/viv#development
There was a problem hiding this comment.
I do think debugging in viv is helpful since it really stretches this functionality to its limit.
aborted via a different signal
|
I know this PR is not perfect yet, but it is good enough for most cases. I will merge this now, as I finally want to ship 1.0. Maybe you'll find the time to test this with the new version, and re-open an issue if the error still persists. |
Implementing fetching of multiple ranges at once using the range header with more than one range.
This PR is WiP.