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

feat/batch creation #2665

Merged
merged 84 commits into from
Feb 23, 2025
Merged

feat/batch creation #2665

merged 84 commits into from
Feb 23, 2025

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Jan 7, 2025

This PR adds a few routines for creating a collection of arrays and groups (i.e., a dict with path-like keys and ArrayMetadata / GroupMetadata values) in storage concurrently.

  • create_hierarchy takes a dict representation of a hierarchy, parses that dict to ensure that there are no implicit groups (creating group metadata documents as needed), then invokes create_nodes and yields the results
  • create_nodes concurrently writes metadata documents to storage, and yields the created AsyncArray / AsyncGroup instances.

I still need to wire up concurrency limits, and test them.

TODO:

  • Add unit tests and/or doctests in docstrings
  • Add docstrings and API docs for any new/modified user-facing classes and functions
  • New/modified features documented in docs/tutorial.rst
  • Changes documented in docs/release.rst
  • GitHub Actions have all passed
  • Test coverage is 100% (Codecov passes)

@d-v-b d-v-b requested review from jhamman and dcherian January 7, 2025 13:27
@normanrz normanrz added this to the After 3.0.0 milestone Jan 7, 2025
@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
@d-v-b
Copy link
Contributor Author

d-v-b commented Jan 10, 2025

this is now working, so I would appreciate some feedback on the design.

The basic design is the same as what I outlined earlier in this PR: there are two new functions that take a dict[path, GroupMetadata | ArrayMetadata] like {'a': GroupMetadata(zarr_format=3), 'a/b': ArrayMetadata(...)} and concurrently persist those metadata documents to storage, resulting in a hierarchy on disk that looks like the dict.

approach

basically the same as concurrent group members listing, except we don't need any recursion. I'm scheduling writes and using as_completed to yield Arrays / Groups when they are available.

new functions

  • create_nodes is low-level and doesn't do any checking of its input, so it will happily create invalid hierarchies, e.g. nesting groups inside arrays, or mixing v2 and v3 metadata, and it won't create intermediate groups, either.

  • create_hierarchy is higher level, it parses the input, checking it for invalid hierarchies, and inserting implicit groups as needed.

  • Group.create_hierarchy is a new method on the Group / AsyncGroup classes that takes a hierarchy dict and creates the nodes specified in that dict at locations relative to the path of the group instance. the return value is dict[str, AsyncGroup | AsyncArray], but I guess it also doesn't have tor return anything, or it could be an async iterator, so that you can interact with the nodes as they are formed. This is flexible right now, but I think the iterator idea is nice.

  • _from_flat (names welcome) is a new function that creates a group entirely from a hierarchy dict + a store. that dict must specify a root group, otherwise an exception is raised. We could revise this to create a root group if one is not specified. Open to suggestions here.

Implicit groups

Partial hierarchies like {'a': GroupMetadata(), 'a/b/c': ArrayMetadata(...)} implicitly denote a group at a/b. When creating such a hierarchy, if we find an existing group at a/b, then we don't need to create a new one. So in the context of modeling a hierarchy, implicit groups are a little special -- by not specifying the properties of the group, the user / application is tolerant of any group being there. So I introduced a subclass of GroupMetadata called _ImplicitGroupMetadata, which can be inserted into a hierarchy dict to explicitly denote groups that don't need to be written if one already exists. _ImplicitGroupMetadata is just like GroupMetadata except it errors if you try to set any parameter except zarr_format.

streaming v2 vs v3 node creation

creating v3 arrays / groups requires writing 1 metadata document, but v2 requires 2. To get the most concurrency I await the write of each metadata document separately, which means that foo/.zattrs might resolve before foo/.zarray. So in the v2 case I only yield up an array / group when both documents were written.

Overlap with metadata consolidation logic

there's a lot of similarity between the stuff in this PR and routines used for consolidated metadata. it would be great to find ways to factor out some of the overlap areas

still to do:

  • write some more tests (checking that implicit groups don't get written if a group already exists)
  • handle overwriting. I think the plan here is, if overwrite is False, then we do a check before any writing to ensure that there are no conflicts between the proposed hierarchy and the stuff that actually exists in storage. this check will involve more IO.

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 13, 2025

I vote 3rd option. Let me just make whatever keys I want to populate, relative to the root of the store.

that works for me. And what should the returned keys be for d = dict(group.create_hierarchy(...))? Relative to the path of the group, or the root of the store? IMO they should be relative to the path of the group (i.e., the same keys come out as went in)

@TomNicholas
Copy link
Member

IMO they should be relative to the path of the group

That sounds fine as it's clear that the group.create_hierarchy API is already looking at one specific group.

@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 18, 2025

  • removed the path and allow_root keyword arguments to create_hierarchy. the keys are now paths relative to the root of the store. for the Group.create_hierarchy method, the keys are relative to the path to the group, which ensures that only sub-arrays and sub-groups of that group can be created.
  • created a new core/sync_group.py module, which contains the synchronous wrappers around async core/group.py functions. this allows us to have two functions named create_nodes, where one is async, and the other is sync, but neither is public. We will eventually want a more structured layout but that's for a later PR.
  • usage examples added to docstrings for create_hierarchy
  • examples added to docs, in both the quickstart and group pages

in the interest of a narrow scope, I've limited the public api to just create_hierarchy.

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Nice. The public API create_hierarchy looks nice to me.

@d-v-b d-v-b enabled auto-merge (squash) February 21, 2025 15:30
@d-v-b d-v-b disabled auto-merge February 21, 2025 15:39
@d-v-b
Copy link
Contributor Author

d-v-b commented Feb 21, 2025

test failure is unrelated to this PR (looks like an fsspec thing)

@d-v-b d-v-b enabled auto-merge (squash) February 23, 2025 17:30
@d-v-b d-v-b merged commit 8d2fb47 into zarr-developers:main Feb 23, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New features or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants