hms-dbmi / viv

Library for multiscale visualization of high-resolution multiplexed bioimaging data on the web. Directly renders Zarr and OME-TIFF.
http://avivator.gehlenborglab.org
MIT License
282 stars 45 forks source link

More careful check for zarr store that supports abort controller #299

Open manzt opened 3 years ago

manzt commented 3 years ago

In https://github.com/hms-dbmi/vizarr/pull/43, we upgraded viv and it broke the imjoy support (in part), due to the addition of the AbortController argument in loader.getTile for zarr images.

This is because we pass an extra argument to store.getItem than before (the signal), and the python store exposed via imjoy-rpc expects 1 argument, throwing and error when getItem() is called with two arguments. I think a fix on our end to isolate this behavior would be to add a hidden property to our custom HTTPStore (e.g. HTTPStore._viv) that we can check for in getTile, but ideally functions exposed to JavaScript from python would "work" like JavaScript functions (where you can supply as many arguments as you want).

const { store } = source;
buffer = await (store._is_viv_httpstore ? store.getItem(key, { signal }) : store.getItem(key));

maybe @oeway has some thoughts here.

manzt commented 3 years ago

This change would have the additional added benefit of making our HTTPStore totally tree-shakable. As of now it's required to for the instanceof HTTPStore check, so even if you don't you an HTTPStore in your application, it will be added to your final bundle.

This way we just check for the object property store._is_viv_httpstore, which will be undefined for any object that isn't our custom store.

ilan-gold commented 3 years ago

I'm fine with this. Another option might be to somehow make the signal part of the store using a Map that is updated per request or something and then call it the signal from the map given the tile request internally. That way you can check if it has something like .signalCache property instead of something specific to us (this might make it more palatable to bring into zarr.js since the API would not change). Just throwing ideas out, I don't have any particular preference.

manzt commented 3 years ago

make the signal part of the store using a Map that is updated per request or something and then call it the signal from the map given the tile request internally.

I think I follow, but I'm hesitant to expose anything generic/public unless there is a compelling use-case beyond ours. The AbortSignal is specific to an HTTPStore that uses fetch internally, and other stores don't rely on fetch.

this might make it more palatable to bring into zarr.js since the API would not change.

I don't think this makes sense for Zarr.js because end users don't manage indexing; this is handled internally and when you call z.getRaw([slice(4,10), null]), where all chunks are requested for a slice of the array and assembled into a single strided view. In that sense, it makes sense to me to just extend the getItem with an extra options parameter for a custom store, since this is totally valid in JavaScript and doesn't break the use of the store within ZarrArray.get.

read: If you are dealing with low level requests (calling the store manually), like we are in Viv, then you can rely on certain custom properties for a specific store, but Zarr.js won't rely on anything that's not a shared interface for all stores.

ilan-gold commented 3 years ago

This sounds good then, an extra property seems like it would work.