Skip to content
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.

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

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

Open
manzt opened this issue Nov 22, 2020 · 4 comments
Open

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

manzt opened this issue Nov 22, 2020 · 4 comments

Comments

@manzt
Copy link
Member

manzt commented Nov 22, 2020

In hms-dbmi/vizarr#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
Copy link
Member Author

manzt commented Nov 22, 2020

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
Copy link
Collaborator

ilan-gold commented Nov 23, 2020

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
Copy link
Member Author

manzt commented Nov 23, 2020

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants