Skip to content

Add ObjectStoreRegistry to support default stores #549

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

Merged
merged 31 commits into from
Apr 18, 2025

Conversation

maxrjones
Copy link
Member

This PR makes it possible to have an empty dict in ManifestStores._stores and automatically create an S3Store or LocalStore from a filepath.

Part of #498

  • Closes #xxxx
  • Tests added
  • Tests passing
  • Full type hint coverage
  • Changes are documented in docs/releases.rst
  • New functions/methods are listed in api.rst
  • New functionality has documentation

store: ObjectStore,
store: ObjectStore | None = None,
group: str | None = None,
) -> ManifestStore: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

@maxrjones Can you include the drop_variables arg in each of these overloads. It needs to be passed down to the _construct_manifest_group call (see https://github.com/zarr-developers/VirtualiZarr/pull/542/files#diff-e89ebbf07b078f47cd46e9b829b2513b7f56a8193af6d09cf045e2669c61a1dbR170)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather address this comment separately if that's alright to keep this PR isolated to supporting default store instances

@maxrjones
Copy link
Member Author

Thanks for the reviews, all. FYI I want to revise this to follow the same design as https://docs.rs/datafusion/latest/datafusion/datasource/object_store/trait.ObjectStoreRegistry.html. So a ManifestStore would have a ObjectStoreRegistry which would implement the register_store and get_store methods. But I've been a bit busy with other tasks and probably won't be able to return to this PR until tomorrow afternoon.

@maxrjones maxrjones marked this pull request as draft April 15, 2025 21:47
@maxrjones maxrjones marked this pull request as ready for review April 17, 2025 23:33
@maxrjones
Copy link
Member Author

I think this PR is ready for a round of reviews if anyone has some extra time 🙏

Copy link
Member

@TomNicholas TomNicholas 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 doing this @maxrjones !

FYI I want to revise this to follow the same design as https://docs.rs/datafusion/latest/datafusion/datasource/object_store/trait.ObjectStoreRegistry.html

Cool idea. Nice to make that connection.

The main thing that's missing from this PR is documentation of how to instantiate ManifestStore in different scenarios. That could either be done now (perhaps as examples in the ManifestStore.__init__ docstring) or it could be left until later. We are building up technical debt in this refactor in the sense that we will need to go back through and document how everything now works.

@maxrjones
Copy link
Member Author

The main thing that's missing from this PR is documentation of how to instantiate ManifestStore in different scenarios. That could either be done now (perhaps as examples in the ManifestStore.init docstring) or it could be left until later. We are building up technical debt in this refactor in the sense that we will need to go back through and document how everything now works.

sounds good, I can do this now.

Comment on lines -159 to +160
prefix: str,
store: ObjectStore,
store: ObjectStore | None = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

@sharkinsspatial with using the scheme + bucket for the ObjectStoreRegistry keys, we no longer need the prefix argument for _create_manifest_store since it can be inferred from the filepath. 🙇 to @kylebarron for this suggested approach

@maxrjones
Copy link
Member Author

@TomNicholas I'm actually going to use your approval to merge this an add the additional docs separately as a new "How to build a reader" section of the contrib guide.

@maxrjones maxrjones merged commit 259ddeb into zarr-developers:develop Apr 18, 2025
10 checks passed
@maxrjones maxrjones deleted the default-store branch April 18, 2025 18:06
@TomNicholas
Copy link
Member

add the additional docs separately as a new "How to build a reader" section of the contrib guide.

This is actually tracked already in #452 FYI

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.

5 participants