-
-
Notifications
You must be signed in to change notification settings - Fork 290
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
Listable V3 Stores #1634
Listable V3 Stores #1634
Conversation
Hello @jhamman! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-02-07 04:46:23 UTC |
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.
A few comments on the state of things here.
zarr/v3/abc/store.py
Outdated
async def get(self, key: str) -> bytes: | ||
async def get( | ||
self, key: str, byte_range: Optional[Tuple[int, Optional[int]]] = None | ||
) -> Optional[bytes]: |
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.
Question for discussion. Do we want to have the byte_range
parameter in the single key get method or just in get_partial_values
? I lean toward this API but it is somewhat duplicative and goes against the suggestion in the spec:
get - Retrieve the value associated with a given key.
Parameters: key
Output: value
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.
My instinct would be to ignore the abstract store interface and just do what makes sense locally.
I don't understand why the spec describes both get(key) -> value
and get_partial_values(key_range) -> List[Optional[value]]
, as opposed to get(key, range) -> Optional[value]
and letting clients handle iteration themselves.
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.
Thinking about this more, we are definitely going to want to be able to pass the store a batch of values keys/ranges to the store. This will let us push optimizations down to lower level code or let us do optimizations like coalescing requests.
I still feel like its worth supporting single partial requests though so I'm going to include that in the abc.
zarr/v3/store/local.py
Outdated
except (FileNotFoundError, IsADirectoryError, NotADirectoryError): | ||
return None |
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.
I found this behavior in Zarrita to be surprising. @normanrz - can you comment on the motivation here? In V2 we would have just raised a KeyError.
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.
I opted for using None
as non-existent. Raising and catching a KeyError would also work.
zarr/v3/store/remote.py
Outdated
@@ -0,0 +1,95 @@ | |||
from __future__ import annotations |
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.
I have not really worked on this store yet.
zarr/tests/test_storage.py
Outdated
@@ -1,87 +1,96 @@ | |||
import array |
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.
Big diff in this file as I decided to comment everything out and bring it back piece by piece. Here's the current test run:
=================================================================================== 13 passed in 0.08s ===================================================================================
❯ pytest zarr/tests/test_storage.py -vvv
================================================================================== test session starts ===================================================================================
platform darwin -- Python 3.11.6, pytest-7.4.3, pluggy-1.3.0 -- /Users/jhamman/miniforge3/envs/zarr-dev/bin/python3.11
cachedir: .pytest_cache
rootdir: /Users/jhamman/Library/CloudStorage/Dropbox/src/zarr-python
configfile: pyproject.toml
plugins: anyio-4.0.0, asyncio-0.23.3
asyncio: mode=Mode.STRICT
collected 13 items
zarr/tests/test_storage.py::test_kvstore_repr PASSED [ 7%]
zarr/tests/test_storage.py::test_ensure_store PASSED [ 15%]
zarr/tests/test_storage.py::test_capabilities PASSED [ 23%]
zarr/tests/test_storage.py::TestMappingStore::test_get_set_del_contains PASSED [ 30%]
zarr/tests/test_storage.py::TestMappingStore::test_set_invalid_content PASSED [ 38%]
zarr/tests/test_storage.py::TestMappingStore::test_writeable_values PASSED [ 46%]
zarr/tests/test_storage.py::TestMappingStore::test_pickle PASSED [ 53%]
zarr/tests/test_storage.py::TestMappingStore::test_hierarchy PASSED [ 61%]
zarr/tests/test_storage.py::TestDirectoryStore::test_get_set_del_contains PASSED [ 69%]
zarr/tests/test_storage.py::TestDirectoryStore::test_set_invalid_content PASSED [ 76%]
zarr/tests/test_storage.py::TestDirectoryStore::test_writeable_values PASSED [ 84%]
zarr/tests/test_storage.py::TestDirectoryStore::test_pickle PASSED [ 92%]
zarr/tests/test_storage.py::TestDirectoryStore::test_hierarchy PASSED
ec2d43d
to
e00ce0b
Compare
- removed abcs for groups/arrays - improved return types in group.py - warn (temporarily) when an implicit group is found - add attributes.py with Attributes class add test file wip
fixes after rebas e make all tests pass
e00ce0b
to
661c0d6
Compare
I'm going to merge this into the V3 branch to make way for tomorrow's sprint. I expect at least one more major rev on the store interface in the coming weeks. |
This goes on top of #1590
The core of this PR is to add
list_*
methods to the v3 stores. The Store interface is very much still in flux but this should be enough to pick up #1590 again.TODO: