-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Refactor: store mode and initialization #2442
Changes from 17 commits
64ad525
8b706f2
59d7f79
075b929
b072545
8726734
858d4fb
c367483
a94e53c
9c173c1
939fb53
d7a6982
c38c2f7
fce2600
cafb46a
a1e71f1
ee47265
b5e5216
89e8539
cb470d4
10f0d97
542da8e
ed8a8ec
9ace4fa
62939b6
2296c3f
9804446
f5f71fa
a1c2818
af30833
a0c86c5
ebe8b67
f9dbd5b
44f20ff
ab8e823
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 |
---|---|---|
|
@@ -3,72 +3,34 @@ | |
from abc import ABC, abstractmethod | ||
from asyncio import gather | ||
from itertools import starmap | ||
from typing import TYPE_CHECKING, NamedTuple, Protocol, runtime_checkable | ||
from typing import TYPE_CHECKING, Literal, Protocol, runtime_checkable | ||
|
||
if TYPE_CHECKING: | ||
from collections.abc import AsyncGenerator, Iterable | ||
from types import TracebackType | ||
from typing import Any, Self, TypeAlias | ||
|
||
from zarr.core.buffer import Buffer, BufferPrototype | ||
from zarr.core.common import AccessModeLiteral, BytesLike | ||
from zarr.core.common import BytesLike | ||
|
||
__all__ = ["AccessMode", "ByteGetter", "ByteSetter", "Store", "set_or_delete"] | ||
__all__ = ["StoreAccessMode", "ByteGetter", "ByteSetter", "Store", "set_or_delete"] | ||
|
||
ByteRangeRequest: TypeAlias = tuple[int | None, int | None] | ||
|
||
|
||
class AccessMode(NamedTuple): | ||
"""Access mode flags.""" | ||
|
||
str: AccessModeLiteral | ||
readonly: bool | ||
overwrite: bool | ||
create: bool | ||
update: bool | ||
|
||
@classmethod | ||
def from_literal(cls, mode: AccessModeLiteral) -> Self: | ||
""" | ||
Create an AccessMode instance from a literal. | ||
|
||
Parameters | ||
---------- | ||
mode : AccessModeLiteral | ||
One of 'r', 'r+', 'w', 'w-', 'a'. | ||
|
||
Returns | ||
------- | ||
AccessMode | ||
The created instance. | ||
|
||
Raises | ||
------ | ||
ValueError | ||
If mode is not one of 'r', 'r+', 'w', 'w-', 'a'. | ||
""" | ||
if mode in ("r", "r+", "a", "w", "w-"): | ||
return cls( | ||
str=mode, | ||
readonly=mode == "r", | ||
overwrite=mode == "w", | ||
create=mode in ("a", "w", "w-"), | ||
update=mode in ("r+", "a"), | ||
) | ||
raise ValueError("mode must be one of 'r', 'r+', 'w', 'w-', 'a'") | ||
StoreAccessMode = Literal["r", "w"] | ||
|
||
|
||
class Store(ABC): | ||
""" | ||
Abstract base class for Zarr stores. | ||
""" | ||
|
||
_mode: AccessMode | ||
_mode: StoreAccessMode | ||
_is_open: bool | ||
|
||
def __init__(self, *args: Any, mode: AccessModeLiteral = "r", **kwargs: Any) -> None: | ||
def __init__(self, *args: Any, mode: StoreAccessMode = "r", **kwargs: Any) -> None: | ||
self._is_open = False | ||
self._mode = AccessMode.from_literal(mode) | ||
assert mode in ("r", "w") | ||
jhamman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self._mode = mode | ||
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 may consider a different setting here (e.g. 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. I generally prefer the attribute matching the parameter name. We can have a property
Edit: which I see you have down below. |
||
|
||
@classmethod | ||
async def open(cls, *args: Any, **kwargs: Any) -> Self: | ||
|
@@ -112,49 +74,48 @@ async def _open(self) -> None: | |
------ | ||
ValueError | ||
If the store is already open. | ||
FileExistsError | ||
If ``mode='w-'`` and the store already exists. | ||
|
||
Notes | ||
----- | ||
* When ``mode='w'`` and the store already exists, it will be cleared. | ||
""" | ||
if self._is_open: | ||
raise ValueError("store is already open") | ||
if self.mode.str == "w": | ||
await self.clear() | ||
elif self.mode.str == "w-" and not await self.empty(): | ||
raise FileExistsError("Store already exists") | ||
self._is_open = True | ||
|
||
async def _ensure_open(self) -> None: | ||
"""Open the store if it is not already open.""" | ||
if not self._is_open: | ||
await self._open() | ||
|
||
@abstractmethod | ||
async def empty(self) -> bool: | ||
async def empty(self, prefix: str = "") -> bool: | ||
""" | ||
Check if the store is empty. | ||
Check if the directory is empty. | ||
jhamman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Parameters | ||
---------- | ||
prefix : str | ||
Prefix of keys to check. | ||
|
||
Returns | ||
------- | ||
bool | ||
True if the store is empty, False otherwise. | ||
""" | ||
... | ||
|
||
@abstractmethod | ||
if not prefix.endswith("/"): | ||
prefix += "/" | ||
async for _ in self.list_prefix(prefix): | ||
return False | ||
return True | ||
|
||
async def clear(self) -> None: | ||
""" | ||
Clear the store. | ||
|
||
Remove all keys and values from the store. | ||
""" | ||
... | ||
async for key in self.list(): | ||
await self.delete(key) | ||
|
||
@abstractmethod | ||
def with_mode(self, mode: AccessModeLiteral) -> Self: | ||
def with_mode(self, mode: StoreAccessMode) -> Self: | ||
jhamman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
Return a new store of the same type pointing to the same location with a new mode. | ||
|
||
|
@@ -163,7 +124,7 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: | |
|
||
Parameters | ||
---------- | ||
mode : AccessModeLiteral | ||
mode : StoreAccessMode | ||
The new mode to use. | ||
|
||
Returns | ||
|
@@ -179,14 +140,19 @@ def with_mode(self, mode: AccessModeLiteral) -> Self: | |
... | ||
|
||
@property | ||
def mode(self) -> AccessMode: | ||
def mode(self) -> StoreAccessMode: | ||
"""Access mode of the store.""" | ||
return self._mode | ||
|
||
@property | ||
def readonly(self) -> bool: | ||
"""Is the store read-only?""" | ||
return self.mode == "r" | ||
|
||
def _check_writable(self) -> None: | ||
"""Raise an exception if the store is not writable.""" | ||
if self.mode.readonly: | ||
raise ValueError("store mode does not support writing") | ||
if self.mode != "w": | ||
raise ValueError(f"store mode ({self.mode}) does not support writing") | ||
|
||
@abstractmethod | ||
def __eq__(self, value: object) -> bool: | ||
|
@@ -342,8 +308,8 @@ def list(self) -> AsyncGenerator[str, None]: | |
@abstractmethod | ||
def list_prefix(self, prefix: str) -> AsyncGenerator[str, None]: | ||
""" | ||
Retrieve all keys in the store that begin with a given prefix. Keys are returned with the | ||
common leading prefix removed. | ||
Retrieve all keys in the store that begin with a given prefix. Keys are returned as | ||
absolute paths (i.e. including the prefix). | ||
|
||
Parameters | ||
---------- | ||
|
@@ -371,6 +337,20 @@ def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: | |
""" | ||
... | ||
|
||
async def delete_dir(self, prefix: str) -> None: | ||
""" | ||
Remove all keys and prefixes in the store that begin with a given prefix. | ||
""" | ||
if not self.supports_deletes: | ||
raise NotImplementedError | ||
if not self.supports_listing: | ||
raise NotImplementedError | ||
self._check_writable() | ||
if not prefix.endswith("/"): | ||
prefix += "/" | ||
async for key in self.list_prefix(prefix): | ||
await self.delete(key) | ||
|
||
def close(self) -> None: | ||
"""Close the store.""" | ||
self._is_open = False | ||
|
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.
Feel free to ignore, but as long as we're making changes here...
I don't like ABCs enforcing a specific signature for
__init__
. Subclasses just always seem to have their own specific parameters, and how exactly that interacts with the base signature is a mess.And I think that generally interfaces don't care about the init signature. Like, we aren't creating instances of
Store
subclasses out of thin air, the user is.I think all the interface cares about is that
.mode
(or.readonly
or whatever) returns the proper thing.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.
Store.__init__
is currently called by all subclasses (e.g.super().__init__(read_only=read_only)
). I did take away the*args
and**kwargs
though. Subclasses should feel free to use their own__init__
and can optionally call the base class one.