Feat/store paths #2272

d-v-b · 2024-09-30T02:27:42Z

Adds a path attribute to all the store classes. Key operations like get, set, list, list_prefix etc are now scoped to the subtree rooted by the path attribute of the store, which is how RemoteStore works in the v3 branch. This change is motivated by the desire for a consistent, scalable store API.

Key changes:

renamed LocalStore.root to LocalStore.path, and changed the type of the attribute from pathlib.Path to str
added a path attribute to MemoryStore
changed the semantics of the path attribute of ZipStore, and introduced a new attribute file_path that does what ZipStore.path used to do (point to the location of the Zipfile in the parent file system).

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)

…nto feat/store-paths

TomAugspurger · 2024-09-30T16:02:21Z

~~Just a note, this fixes a failure in some xarray tests that write an array at the top level of the store, open a group at the root and then get the array~~

Edit: whoops, never mind. I missed that this only affects zarr_format=2 when testing. I'll make a new issue.

jhamman

Thanks for working on this @d-v-b. I'm not 100% convinced the with_mode approach is going to be substantially cleaner than our StorePath right now but I'm curious to see how this plays out.

jhamman · 2024-10-02T21:14:55Z

src/zarr/abc/store.py

        for req in requests:
            yield (req[0], await self.get(*req))

+    def with_path(self, path: str) -> Self:


Suggested change

def with_path(self, path: str) -> Self:

@abstractmethod

def with_path(self, path: str) -> Self:

Or do you think think we can do this in a generic way that applies to all stores?

I don't think we can make it generic unfortunately, because each store class might have its own extra attributes. maybe we could use __getstate__ to get around this?

src/zarr/store/local.py

jhamman · 2024-10-02T21:27:06Z

src/zarr/store/memory.py

        self,
        store_dict: MutableMapping[str, Buffer] | None = None,
        *,
+        path: str = "",


this store highlights the fact that not all stores have a meaningful path.

I'm not sure I agree -- if store_dict really is an arbitrary mutable mapping, there's no guarantee that we want all IO operations to be scoped to the top-level key space, and that's exactly what the path attribute is for.

jhamman · 2024-10-02T21:29:10Z

src/zarr/store/zip.py

+        file_path: Path | str,
        *,
+        path: str = "",


This feels quite confusing. IIUC, you are allowing the caller to specify a path within the zipfile as opposed to putting the root of the store at the root of the zip, is that right?

exactly! this is required to make ZipStore congruent to RemoteStore

though this may have some impact (simplifying) zarr-developers/zarr-specs#311

…at/store-paths

…of the path attribute

…at/store-paths

d-v-b · 2024-10-20T20:18:31Z

worth mentioning some changes to the reprs:

In main, MemoryStore and GPUMemoryStore reprs are <store class name>://<id of ._store_dict>. This PR changes that to <store class name>://<id of ._store_dict>/<store.path>
in main, LocalStore repr is file://<store.root>, which is allegedly wrong for a file url. In this PR, it's file:///<store.path>
ZipStore is a bit weird, because it's a nested store: we need a wrapping file system to get to the zip file, and then inside the zip file we have another file system. In this PR i'm using the syntax introduced by @jbms for this purpose, which is basically a sequence of URLs separated by the | character, e.g. file:///<store.file_path>|zip://<store.path> (Jeremy please correct me if I'm wrong here).

jbms · 2025-04-16T22:53:04Z

worth mentioning some changes to the reprs:

In main, MemoryStore and GPUMemoryStore reprs are <store class name>://<id of ._store_dict>. This PR changes that to <store class name>://<id of ._store_dict>/<store.path>

in main, LocalStore repr is file://<store.root>, which is allegedly wrong for a file url. In this PR, it's file:///<store.path>

ZipStore is a bit weird, because it's a nested store: we need a wrapping file system to get to the zip file, and then inside the zip file we have another file system. In this PR i'm using the syntax introduced by @jbms for this purpose, which is basically a sequence of URLs separated by the | character, e.g. file:///<store.file_path>|zip://<store.path> (Jeremy please correct me if I'm wrong here).

I didn't see this before, but note that neuroglancer (and soon tensorstore) use the syntax:

base-url|zip:path/within/zip

not

base-url|zip://path/within/zip

Regarding file: --- currently tensorstore always uses file://PATH where PATH is typically an absolute path (i.e. beginning with /, such that you have file:///tmp/dataset/) but tensorstore also allows a relative path as a non-standard extension.

According to https://en.wikipedia.org/wiki/File_URI_scheme it is supposed to be either file:///tmp/dataset/ or file:/tmp/dataset/ but not file://tmp/dataset/ --- the optional extra two slashes at the beginning are for indicating a "hostname", which as far as I am aware is always empty, i.e. unused.

I should probably fix tensorstore to accept either form. For both tensorstore and zarr-python allowing file:RELATIVE or file://RELATIVE also makes sense, though I suppose with file://RELATIVE there is a risk that the user intended to specify an absolute path but forgot a slash.

jbms · 2025-04-16T22:55:46Z

src/zarr/storage/zip.py

    def __str__(self) -> str:
-        return f"zip://{self.path}"
+        # lets try https://github.com/zarr-developers/zeps/pull/48/files
+        return f"file:///{self.file_path}|zip://{self.path}"


I think you have too many slashes after file: because file_path also starts with a slash, right?

Also you should percent-encode both file_path and path for consistency with my ZEP proposal and tensorstore, and to avoid ambiguity since | is a legal path character except on Windows.

thanks for catching this!

d-v-b added 5 commits September 27, 2024 16:13

add path attribute to stores; migrate localstore to the new protocol

736ef8a

add path kwarg to memory test fixture

8189cc7

Merge branch 'v3' of https://github.com/zarr-developers/zarr-python i…

9696222

…nto feat/store-paths

add path to stores

fa13860

Merge branch 'v3' of https://github.com/zarr-developers/zarr-python i…

c5586ce

…nto feat/store-paths

TomAugspurger mentioned this pull request Sep 30, 2024

Zarr Python 3 tracking issue pydata/xarray#9515

Closed

4 tasks

jhamman reviewed Oct 2, 2024

View reviewed changes

ghost approved these changes Oct 4, 2024

View reviewed changes

jhamman added the V3 label Oct 11, 2024

jhamman changed the base branch from v3 to main October 14, 2024 20:52

d-v-b added 19 commits October 17, 2024 14:20

Merge branch 'main' of github.com:zarr-developers/zarr-python into fe…

b4b9c28

…at/store-paths

add _path attribute to localstore that caches a pathlib.Path version …

c6c8843

…of the path attribute

remove some old roots

b5dd50c

make store test class get and set async

d7fbbdc

remove an old root

0b1c454

resolve get before _getting in zipstore tests

d378242

Merge branch 'main' into feat/store-paths

669e258

add path kwarg to gpu memorystore test fixture

9d0d04b

Merge branch 'main' of github.com:zarr-developers/zarr-python into fe…

471740b

…at/store-paths

zipstore store_kwargs propagates path

9a83b86

storetests store_kwargs defaults to not implemented

84c545f

memory store clear is scoped to the path

bd912ef

use relative path for zipfile listing

0c7ab00

use path in with_mode for memorystore

d04934d

call resolve_path at the top of store routines for memorystore

9e65afb

use path for gpumemorystore

a5b186b

use relative key in test_list_dir

aab4138

fix warning test

346d291

parameterize over path in memorystore tests

e68a341

d-v-b added 2 commits October 20, 2024 19:54

Merge branch 'main' of github.com:zarr-developers/zarr-python into fe…

eb93c24

…at/store-paths

lint docstring

59ac9ed

d-v-b marked this pull request as ready for review October 20, 2024 19:56

d-v-b added 3 commits October 20, 2024 22:03

put path in memorystore and gpu memorystore reprs

fc4a780

update localstore repr

a92cd4a

update zipstore repr

1bd1714

d-v-b requested review from jhamman and joshmoore October 20, 2024 22:05

change default path for remotestore to ''

808e9ff

dstansby removed the V3 label Dec 12, 2024

dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025

d-v-b force-pushed the feat/store-paths branch from b5ef33c to 808e9ff Compare April 16, 2025 22:38

jbms reviewed Apr 16, 2025

View reviewed changes

	def with_path(self, path: str) -> Self:
	@abstractmethod
	def with_path(self, path: str) -> Self:

Uh oh!

Feat/store paths #2272

Are you sure you want to change the base?

Feat/store paths #2272

Conversation

d-v-b commented Sep 30, 2024

Uh oh!

TomAugspurger commented Sep 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhamman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-v-b commented Oct 20, 2024

Uh oh!

jbms commented Apr 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

TomAugspurger commented Sep 30, 2024 •

edited

Loading