Skip to content
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

Extensible codecs for V3 #1588

Merged
merged 13 commits into from
Dec 6, 2023
Merged

Extensible codecs for V3 #1588

merged 13 commits into from
Dec 6, 2023

Conversation

normanrz
Copy link
Member

@normanrz normanrz commented Dec 5, 2023

Makes codecs extensible in the v3 branch by adding a codec registry.

@pep8speaks
Copy link

pep8speaks commented Dec 5, 2023

Hello @normanrz! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 76:20: E203 whitespace before ':'
Line 76:46: E203 whitespace before ':'
Line 76:72: E203 whitespace before ':'
Line 214:20: E203 whitespace before ':'
Line 214:46: E203 whitespace before ':'
Line 214:72: E203 whitespace before ':'

Line 301:44: E203 whitespace before ':'
Line 313:30: E203 whitespace before ':'

Comment last updated at 2023-12-06 10:51:06 UTC

Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

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

I mostly reviewed the entrypoints stuff since the rest seemed to be reorganization / porting from zarrita. Looks really nice!

zarr/tests/test_codecs_v3.py Outdated Show resolved Hide resolved
zarr/v3/codecs/registry.py Outdated Show resolved Hide resolved
zarr/v3/codecs/sharding.py Outdated Show resolved Hide resolved
@@ -59,24 +53,8 @@ def _structure_chunk_key_encoding_metadata(d: Dict[str, Any], _t) -> ChunkKeyEnc
)

def _structure_codec_metadata(d: Dict[str, Any], _t=None) -> CodecMetadata:
Copy link
Member

Choose a reason for hiding this comment

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

not for now but it would be nice if the codec metadata was a TypedDict so we could expect for keys like "name".

Copy link
Member Author

Choose a reason for hiding this comment

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

name is really the only shared attribute among all codecs. Even configuration is optional. I don't think there'd be much value in a TypedDict. This piece of code is in the middle of the deserialization; lots of untyped magic happening here anyways :-)

normanrz and others added 2 commits December 6, 2023 11:34
@jhamman jhamman merged commit 1a98886 into zarr-developers:v3 Dec 6, 2023
5 of 6 checks passed
Comment on lines +19 to +36
@frozen
class _AsyncArrayProxy:
array: AsyncArray

def __getitem__(self, selection: Selection) -> _AsyncArraySelectionProxy:
return _AsyncArraySelectionProxy(self.array, selection)


@frozen
class _AsyncArraySelectionProxy:
array: AsyncArray
selection: Selection

async def get(self) -> np.ndarray:
return await self.array.getitem(self.selection)

async def set(self, value: np.ndarray):
return await self.array.setitem(self.selection, value)
Copy link
Member Author

Choose a reason for hiding this comment

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

@jhamman @d-v-b What do you think about this making it into the AsyncArray? It allows for writing something like this

await _AsyncArrayProxy(array)[:10, 10:].get()

instead of

await array.getitem((slice(None, 10), slice(10, None)))

jhamman added a commit to jhamman/zarr-python that referenced this pull request Jan 24, 2024
* Pull Zarrita into Zarr-Python @ 78274781ad64aef95772eb4b083f7ea9b7d03d06

No code changes to Zarrita were made.

* apply zarr lint rules

* zarrita -> v3

* v3/abc [wip]

* use abcs plus implementation notes

* working on making codecs extensible

* adds index_location

* adds support for codec entry points

* adds tests from zarrita

* fixes types

* Apply suggestions from code review

Co-authored-by: Joe Hamman <jhamman1@gmail.com>

* remove test codec from pyproject.toml

---------

Co-authored-by: Joseph Hamman <joe@earthmover.io>
Co-authored-by: Joe Hamman <jhamman1@gmail.com>
@jhamman jhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants