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

Use Proxy store for nested chunks #85

Merged
merged 6 commits into from
Apr 20, 2021
Merged

Conversation

will-moore
Copy link
Collaborator

Code adapted from gzuidhof/zarr.js#88 (comment)

In progress:

  • Need to check whether data is nested (move logic to ome.ts and only for OME-Zarr version 0.2)
  • No Typescript types yet...etc

Code adapted from gzuidhof/zarr.js#88 (comment)
No Typescript types yet
src/utils.ts Outdated Show resolved Hide resolved
Copy link
Member

@manzt manzt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small comment. In general, it should be ok to reuse the original open function since nested only effects chunk keys and some stores used in vizarr will not be nested.

Instead we should try to feature detect when "nested" is needed and just update the underlying "store" for the zarr.Group.

src/utils.ts Outdated
Comment on lines 37 to 54
function nested(store) {
const get = (target, key) => {
if (key === 'getItem' || key === 'setItem' || key === 'containsItem') {
return (path, ...args) => {
if (path.endsWith('.zarray') || path.endsWith('.zattrs') || path.endsWith('.zgroup')) {
return target[key](path, ...args);
}
const prefix = path.split('/');
const chunkKey = prefix.pop();
const newPath = [...prefix, chunkKey.replaceAll('.', '/')].join('/');
return target[key](newPath, ...args);
}
}
return Reflect.get(target, key);
};
return new Proxy(store, { get });
}
const nested_store = nested(store);
Copy link
Member

@manzt manzt Mar 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually move this to be a utility function (outside of open), since this would break vizarr for folks who don't have a nested store (e.g. imjoy usage, Jupyter notebooks).

My thought is that in io.ts, we can check for OME-Zarr version and replace the underlying store with a proxy if needed.

if (node instanceof ZarrGroup) {
    const attrs = (await node.attrs.asObject()) as Ome.Attrs;

  // not sure where the v0.2 meta data is located, but something similar to this.
   if (attrs.version === "0.2") {
       node.store = nested(node.store); // replaces underlying store with proxied store.
    }

    if ('plate' in attrs) {
      return loadPlate(config, node, attrs.plate);
    }

    if ('well' in attrs) {
      return loadWell(config, node, attrs.well);
    }

/* ... */

@manzt
Copy link
Member

manzt commented Mar 30, 2021

BTW, I wrote a small CLI today to launch multiple instances of vizarr in different tabs with various data URLs. I found myself opening the images manually from the OME-Blog to test changes (which could get tedious). It's written in Typescript and made to execute in deno, so the script can be used/installed simply via a gist.

Install

$ deno install --allow-read --allow-net --allow-run https://gist.githubusercontent.com/manzt/f24b30b4f59c3d419ecf1e37dd35ee86/raw/b6e567b4c32090f286e5af71ec445a280a89d7e8/launch-vizarr.ts

List all images in configuration

$ launch-vizarr --list --config https://raw.githubusercontent.com/ome/blog/master/_data/zarr_data_2020-12-01.json 
# ┌───────┬────────┬────────────┬─────────────────────────┬───────────┬───────┬────────┬──────────────┬─────────────────────┬───────────────┐
# │ (idx) │   id   │  image_id  │          name           │   study   │ wells │ fields │ acquisitions │     dimensions      │ first_version │
# ├───────┼────────┼────────────┼─────────────────────────┼───────────┼───────┼────────┼──────────────┼─────────────────────┼───────────────┤
# │   0   │ "2551" │ "1229801"  │     "JL_120731_S6A"     │ "idr0001" │ "96"  │  "6"   │     "6"      │ "1376x1040x16x2x1"  │     "0.1"     │
# │   1   │ "422"  │  "179693"  │     "plate1_1_013"      │ "idr0002" │ "96"  │  "1"   │     "1"      │ "1344x1024x1x2x329" │     "0.1"     │
# │   2   │ "1751" │  "692149"  │         "P101"          │ "idr0004" │ "69"  │  "1"   │     "1"      │  "672x510x10x2x1"   │     "0.1"     │
# │   3   │ "5966" │ "3230447"  │ "41744_illum_corrected" │ "idr0033" │ "384" │  "9"   │     "1"      │  "1080x1080x1x5x1"  │     "0.1"     │
# │   4   │ "7825" │ "10567921" │      "ESP0025722"       │ "idr0094" │ "96"  │  "9"   │     "1"      │  "1080x1080x1x1x1"  │     "0.1"     │
# └───────┴────────┴────────────┴─────────────────────────┴───────────┴───────┴────────┴──────────────┴─────────────────────┴───────────────┘

Open 2 browser tabes for plates 0 & 4 in a development version of vizarr

$ launch-vizarr --vizarr http://localhost:8080 --idx 0,4 --config https://raw.githubusercontent.com/ome/blog/master/_data/zarr_data_2020-12-01.json 

Both of these JSON files are recognized from the OME-Blog: https://github.com/ome/blog/tree/master/_data, more in the README: https://gist.github.com/manzt/f24b30b4f59c3d419ecf1e37dd35ee86

@will-moore
Copy link
Collaborator Author

Thanks for the pointers. Should be looking better now, although there may be nicer ways of pleasing the TypeScript Gods with ...opts etc

@will-moore will-moore marked this pull request as ready for review March 31, 2021 15:51
Copy link
Member

@manzt manzt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, Will. I made a few comments. Slight preference to move check / handling of nesting outside of each load* function and into one place in io.ts.

src/utils.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/ome.ts Outdated Show resolved Hide resolved
@manzt
Copy link
Member

manzt commented Mar 31, 2021

Also noticing that linting/formatting GH Actions only occur on my push but not for PRs. Would you be able to run npm run format? I'll fix the GH Actions for the future.

@manzt
Copy link
Member

manzt commented Apr 8, 2021

@will-moore this looks like it's almost there (nesting logic duplicated now in ome.ts and io.ts)! Do you need anything from me to help push this through?

@will-moore
Copy link
Collaborator Author

Sorry, missed that. Fixed now. Thanks.

@will-moore
Copy link
Collaborator Author

will-moore commented Apr 9, 2021

@manzt
Copy link
Member

manzt commented Apr 12, 2021

Great, thank you!

Seeing zarr-developers/zarr-python#715, I'm thinking we will eventually need to create a more sophisticated check for isNested as to whether to create this proxy. If/when bioformats2raw adopts the dimension_separator field, we could run into an issue where we create a proxy from the OME-Zarr metadata but Zarr.js also tries to handle nesting since it is now (or will be) a part of the spec gzuidhof/zarr.js#89.

Essentially, vizarr has to handle two OME-Zarr "0.2" variants which I think could get complicated. cc: @joshmoore

@manzt
Copy link
Member

manzt commented Apr 12, 2021

I assume this could also be an issue in python, where key_separator is manually set for the FSStore depending on the version of the OME metadata, but eventually zarr-python would handle the key transformation automatically from the actual array.

@manzt manzt closed this Apr 20, 2021
@manzt manzt reopened this Apr 20, 2021
@manzt
Copy link
Member

manzt commented Apr 20, 2021

@joshmoore @will-moore

I'm going to merge this for now, but I'm curious if any have comments on a plan for how ome-zarr-py will handle the nested store.

@manzt manzt merged commit 921d60a into hms-dbmi:master Apr 20, 2021
@joshmoore
Copy link

👍 ome/ome-zarr-py#75 implements a workaround while we wait on dimension_separator to be in place (zarr-developers/zarr-python#716). While coding it up, I did feel more and more like the workaround was out of place and the low-level array code should be handling all of this. In that case, the ngff spec should really say, "depends on zarr spec v2 as of zarr-python 2.7.1" but that's hard to detect.

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

Successfully merging this pull request may close these issues.

3 participants