-
Notifications
You must be signed in to change notification settings - Fork 92
Support zarr-python 3.x #646
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
Changes from all commits
70abb2c
a870ff7
0d72485
4e9a9f2
4e08f8c
9ad5c8a
7bbfe90
e8c8f54
47b9bd2
8bccefb
91852e8
e2e7bca
7b88b38
5cbe53e
55e2fa8
30fddab
5eeb055
c2a59bf
e8ec00d
75a5cb5
6bd4079
bc92a2c
1ca12e2
172eeb7
101d1f7
6e2e2e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,10 @@ | ||||||||||||||||||||||||||
| # Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from importlib import metadata as _metadata | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from packaging.version import Version as _Version, parse as _parse | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if _parse(_metadata.version("zarr")) >= _Version("3.0.0"): | ||||||||||||||||||||||||||
| from ._zarr_python_3 import * # noqa: F401,F403 | ||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||
| from ._zarr_python_2 import * # type: ignore[assignment] # noqa: F401,F403 | ||||||||||||||||||||||||||
|
Comment on lines
+3
to
+10
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, if we're doing this, I would prefer that we have: This way, a user's scripts don't suddenly break because they run on them on a machine with a different environment.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have two strategies here: try to preserve source-code compatibility or not. So under option 1, we have the potential for some user's code working with either zarr-python 2.x or 3.x, but if and only if the user limits themselves to If you're using any of the other features of We also get the (IMO) nicer name of I don't have a strong preference between the two strategies. My initial implementation would have required users to import the new module name with zarr-python 3.x, but I very slightly preferred trying to preserve source compatibility for that (potentially narrow) subset of use cases where source compatibility is possible to preserve.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess I see that. I'm kind of on the fence because presumably at some point in the future we will only support zarr3 and then there'll be another breakage. What do we think is our best migration strategy, if at some point in the future
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What would cause the second breakage? Moving people back to Here's an attempt at a summary. Under the PR as currently written, we have
Under the split zarr.v2, zarr.v3 proposal
I'm still leaning slightly towards just using
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, but I have no strong opinion either :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's a convincing argument in favour of the approach here. Let's go with that |
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| # Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. | ||
| # See file LICENSE for terms. | ||
|
|
||
| import asyncio | ||
| import os | ||
| from pathlib import Path | ||
|
|
||
| import zarr.storage | ||
| from zarr.abc.store import ( | ||
| ByteRequest, | ||
| OffsetByteRequest, | ||
| RangeByteRequest, | ||
| SuffixByteRequest, | ||
| ) | ||
| from zarr.core.buffer import Buffer, BufferPrototype | ||
| from zarr.core.buffer.core import default_buffer_prototype | ||
|
|
||
| import kvikio | ||
|
|
||
| # The GDSStore implementation follows the `LocalStore` implementation | ||
| # at https://github.com/zarr-developers/zarr-python/blob/main/src/zarr/storage/_local.py | ||
| # with differences coming swapping in `cuFile` for the stdlib open file object. | ||
|
|
||
|
|
||
| def _get( | ||
| path: Path, prototype: BufferPrototype, byte_range: ByteRequest | None | ||
| ) -> Buffer: | ||
| file_size = os.path.getsize(path) | ||
| file_offset: int | ||
|
|
||
| match byte_range: | ||
| case None: | ||
| nbytes = file_size | ||
| file_offset = 0 | ||
| case OffsetByteRequest(): | ||
| nbytes = max(0, file_size - byte_range.offset) | ||
| file_offset = byte_range.offset | ||
| case RangeByteRequest(): | ||
| nbytes = byte_range.end - byte_range.start | ||
| file_offset = byte_range.start | ||
| case SuffixByteRequest(): | ||
| nbytes = byte_range.suffix | ||
| file_offset = max(0, file_size - byte_range.suffix) | ||
| case _: | ||
| # This isn't allowed by mypy, but the tests assert we raise | ||
| # something here. | ||
| raise TypeError(f"Unexpected byte_range, got {byte_range}") | ||
|
TomAugspurger marked this conversation as resolved.
|
||
|
|
||
| # kvikio doesn't support reading past the end of a file. Some zarr tests | ||
| # rely on this behavior: to "read" 3 bytes out of a 0 byte file, or to | ||
| # "seek" past the end of a file with file_offset. The semantics seem to | ||
| # be roughly the same as slicing an empty bytestring. | ||
|
|
||
| nbytes = min(nbytes, file_size) | ||
| file_offset = min(file_offset, file_size) | ||
|
|
||
| raw = prototype.nd_buffer.create(shape=(nbytes,), dtype="b").as_ndarray_like() | ||
| buf = prototype.buffer.from_array_like(raw) | ||
|
|
||
| with kvikio.CuFile(path) as f: | ||
| # Note: this currently creates an IOFuture and then blocks | ||
| # on reading it. The blocking read means this is in a regular | ||
| # sync function, and so this must be run in a threadpool. | ||
| future = f.pread(raw, size=nbytes, file_offset=file_offset) | ||
| future.get() # blocks | ||
|
Comment on lines
+60
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: Should we figure out a way to hook this up with the normal asyncio stuff?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe... When I went down a rabbit hole investigating zarr-developers/zarr-python#2863 (comment) I looked into this a bit and came across some discussions around supporting In theory we ought to be able to do stuff with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #79 (comment) has a bit of a writeup with my findings if you're curious, but the tl/dr is that this seems pretty challenging (at least given my understanding of the C++ / CUDA side of things).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks. It looks like if we would want to do that we need to return honest-to-goodness python futures from What I would be worried about getting right is that setting the python future status requires the gil so locking order is going to be tricky. |
||
|
|
||
| return buf | ||
|
|
||
|
|
||
| def _put( | ||
|
TomAugspurger marked this conversation as resolved.
|
||
| path: Path, | ||
| value: Buffer, | ||
| start: int | None = None, | ||
| exclusive: bool = False, | ||
| ) -> int | None: | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| if start is not None: | ||
| with kvikio.CuFile(path, "r+b") as f: | ||
| f.write(value.as_array_like(), file_offset=start) | ||
| return None | ||
| else: | ||
| buf = value.as_array_like() | ||
| if exclusive: | ||
| if path.exists(): | ||
| raise FileExistsError(f"File exists: {path}") | ||
| mode = "wb" | ||
| with kvikio.CuFile(path, flags=mode) as f: | ||
| return f.write(buf) | ||
|
|
||
|
|
||
| class GDSStore(zarr.storage.LocalStore): | ||
|
madsbk marked this conversation as resolved.
|
||
| def __repr__(self) -> str: | ||
| return f"{type(self).__name__}('{self}')" | ||
|
|
||
| async def get( | ||
| self, | ||
| key: str, | ||
| prototype: BufferPrototype | None = None, | ||
| byte_range: ByteRequest | None = None, | ||
| ) -> Buffer | None: | ||
|
TomAugspurger marked this conversation as resolved.
|
||
| if prototype is None: | ||
| prototype = default_buffer_prototype() | ||
| if not self._is_open: | ||
| await self._open() | ||
| assert isinstance(key, str) | ||
| path = self.root / key | ||
|
|
||
| try: | ||
| return await asyncio.to_thread(_get, path, prototype, byte_range) | ||
| except (FileNotFoundError, IsADirectoryError, NotADirectoryError): | ||
| return None | ||
|
|
||
| async def set(self, key: str, value: Buffer) -> None: | ||
| return await self._set(key, value) | ||
|
TomAugspurger marked this conversation as resolved.
|
||
|
|
||
| async def _set(self, key: str, value: Buffer, exclusive: bool = False) -> None: | ||
| if not self._is_open: | ||
| await self._open() | ||
| self._check_writable() | ||
| assert isinstance(key, str) | ||
| if not isinstance(value, Buffer): | ||
| raise TypeError( | ||
| f"LocalStore.set(): `value` must be a Buffer instance. Got an " | ||
| f"instance of {type(value)} instead." | ||
| ) | ||
| path = self.root / key | ||
|
|
||
| await asyncio.to_thread(_put, path, value, start=None, exclusive=exclusive) | ||
|
TomAugspurger marked this conversation as resolved.
|
||
|
|
||
|
|
||
| # Matching the check that zarr.__version__ > 2.15 that's | ||
| # part of the public API for our zarr 2.x support | ||
| # This module is behind a check that zarr.__version__ > 3 | ||
| # so we can just assume it's already checked and supported. | ||
| supported = True | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed to run the tests from zarr's
StoreTests.