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

Version counters #109

Merged

Conversation

keller-mark
Copy link
Collaborator

I'm not sure if this is the best way to implement this (and especially the typings), but the general idea is that the root of the store can (optionally) maintain two counters: v2_count and v3_count, which represent the number of times the store has successfully opened v2 vs. v3 metadata. Then, we can use these counters to determine which version to prioritize in the subsequent open() calls.

The goal is to avoid extra v3 zarr.json network requests and console errors when requesting v2 data, but without restricting by using open.v2 or open.v3. This is particularly important in higlass-zarr-datafetchers https://github.com/higlass/higlass-zarr-datafetchers/tree/keller-mark-patch-1 where many requests are being made on zoom/pan interactions.

Screenshot 2023-09-23 at 11 33 05 AM

@manzt
Copy link
Owner

manzt commented Sep 23, 2023

I really like the idea, but in terms of implementation I'm not sure it's something that should live on the store. I think this might be a nice use case for a WeakMap, where we could keep context so long as the store lives in memory.

// open.ts
function create_version_counter() {
  let version_counts = new WeakMap<Readable, { v2: number, v3: number}>();
  function get_counts(store: Readable) {
    let counts = version_counts.get(store) ?? { v2: 0, v3: 0 };
    version_counts.set(store, counts);
    return counts;
  }
  return {
    increment(store: Readable, version: "v2" | "v3") {
      get_counts(store)[version] += 1;
    },
    version_max(store: Readable): "v2" | "v3" {
      let counts = get_counts(store);
      return counts.v3 > counts.v2 ? "v3" :  "v2";
    }
  }
}
let VERSION_COUNTER = create_version_counter();

@keller-mark
Copy link
Collaborator Author

Ah that is much nicer! I have re-implemented using your code snippet

Copy link
Owner

@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.

Looks great. Just a comment on when we should increment the counts.

@@ -43,6 +62,7 @@ async function open_v2<Store extends Readable>(
let loc = "store" in location ? location : new Location(location);
let attrs = {};
if (options.attrs ?? true) attrs = await load_attrs(loc);
VERSION_COUNTER.increment(loc.store, "v2");
Copy link
Owner

Choose a reason for hiding this comment

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

For v2, maybe we only increment if a node has successfully been found? this within open_array_v2 and open_group_v2 but only if they are successful?

async function open_array_v2<Store extends Readable>(
	location: Location<Store>,
	attrs: Attributes,
) {
	let { path } = location.resolve(".zarray");
	let meta = await location.store.get(path);
	if (!meta) {
		throw new NodeNotFoundError(path);
	}
++	VERSION_COUNTER.increment(loc.store, "v2");
	return new Array(
		location.store,
		location.path,
		v2_to_v3_array_metadata(json_decode_object(meta), attrs),
	);
}

Or we add some wrappers functions:

async function open_v2_with_counter<Store extends Readable>(...args: Parameters<typeof open_v2<Store>>) {
	let node = await open_v2(...args);
	VERSION_COUNTER.increment(loc.store, "v2");
	return node;
}

@manzt manzt merged commit 3ec6538 into manzt:main Sep 25, 2023
2 checks passed
@github-actions github-actions bot mentioned this pull request Sep 25, 2023
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.

2 participants