basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

basic support for v2 and v3 groups - #1590

Closed
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support
Closed

basic support for v2 and v3 groups#1590
jhamman wants to merge 3 commits into
zarr-developers:v3from
jhamman:basic-v2-and-v3-group-support

Conversation

@jhamman

Copy link
Copy Markdown
Member

This begins the work of harmonizing the V2 and V3 group APIs from Zarrita with the Zarr-Python Group API.

I'll leave some comments inline.

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)

@pep8speaks

pep8speaks commented Dec 5, 2023

Copy link
Copy Markdown

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

Line 30:1: W293 blank line contains whitespace

Comment last updated at 2023-12-06 15:13:06 UTC

@jhammanjhamman left a comment

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

A few thoughts below 👇

Comment threadzarr/v3/group.py
attributes: Dict[str, Any] = field(factory=dict)
zarr_format: Literal[3] = 3
node_type: Literal["group"] = "group"
zarr_format: Literal[2, 3] = 3 # field(default=3, validator=validators.in_([2, 3]))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@normanrz - I tried this and failed! Happy to return to it later.

Comment threadzarr/v3/abc/group.py Outdated

@abstractmethod
def __setitem__(self, key: str, value: Union[SyncArray, "SyncGroup"]) -> None:
"""get child"""

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
"""get child"""

Proposing that we do away with Group.__setitem__ all together. It really doesn't make sense to include in the api.

Comment threadzarr/v3/abc/group.py Outdated
@abstractmethod
def group_keys(self) -> AsyncIterator[str]:
"""iterate over child group keys"""
...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

all of these iterators need listable stores before they can be built.

Comment threadzarr/v3/config.py
@frozen
class SyncConfiguration:
concurrency: Optional[int] = None
asyncio_loop: Optional[AbstractEventLoop] = None

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

this was a half hearted attempt at rethinking the config topic. I expect this to go away soon.

Comment threadzarr/v3/group.py
Comment on lines +317 to +334
def __setitem__(self, key, value):
raise NotImplementedError

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change
def__setitem__(self, key, value):
raiseNotImplementedError

Comment threadzarr/v3/group.py Outdated
return self._sync(self._async_group.nchildren)

@property
def children(self) -> List[Array, "Group"]:

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some exploratory work is needed to figure out how to make this (and similar methods) a proper iterator/generator instead of a list. The tricky bit is that the async method is returning an AsyncIterator and we need to synchronize that into a regular Iterator.

Comment threadzarr/v3/abc/group.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that there is only 1 Group and AsyncGroup. Do we actually want to keep the Group ABC?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@d-v-b and I were just talking about that last night. I think the answer is NO. For ABCs, I we probably only need the Store and Codec base classes.

Comment threadzarr/v3/group.py
# TODO: consider trying to autodiscover the zarr-format here
if zarr_format == 3:
# V3 groups are comprised of a zarr.json object
# (it is optional in the case of implicit groups)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are implicit groups really a thing? I don't think they show up in the v3 spec.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

They do show up in the spec (in quite a few places actually)! https://zarr-specs.readthedocs.io/en/latest/v3/core/v3.0.html#explicit-vs-implicit-groups

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I didn't know that!

Comment threadzarr/v3/group.py
if self.zarr_format == 3:
zarr_json_bytes = await (store_path / ZARR_JSON).get_async()
if zarr_json_bytes is None:
# implicit group?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a bit dangerous because we have no good way of telling whether this path even exists.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I agree and we should return to this. For now I'm going to put a log statement there and we can address this later.

Comment threadzarr/v3/group.py
store_path, zarr_json, runtime_configuration=self.runtime_configuration
)
elif self.zarr_format == 2:
# Q: how do we like optimistically fetching .zgroup, .zarray, and .zattrs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the overfetching is fine because it is happening in parallel.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was thinking about this issue the other day. One possible behavior when you "open" anything in Zarr would be to call the store.tree(depth=1) method, fetching all existing metadata docs at the root level and one level below.

Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
Comment threadzarr/v3/group.py Outdated
- 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
@jhamman
jhammanforce-pushed the basic-v2-and-v3-group-support branch from 888551a to 84b497fCompareDecember 6, 2023 15:24
@jhamman
jhamman marked this pull request as ready for review December 6, 2023 16:52
Comment threadzarr/v3/group.py
elif self.zarr_format == 2:
return {
ZGROUP_JSON: self.zarr_format,
ZATTRS_JSON: json.dumps(self.attributes).encode(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how would custom JSON serialization of user attributes plug in here? e.g., i don't think numpy dtype objects serialize to JSON easily. Maybe to_bytes can take a attrs_serializer kwarg or something.

Comment threadzarr/v3/group.py
def create_array(self, name: str, **kwargs) -> Array:
return Array(self._sync(self._async_group.create_array(name, **kwargs)))

def empty(self, **kwargs) -> Array:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thoughts on ultimately putting these routines in a dedicated namespace?

Comment threadzarr/v3/store.py
try:
if self.store == other.store and self.path == other.path:
return True
except Exception:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can probably be a bit more specific about the exception type here

@jhammanjhamman closed this Feb 7, 2024
@jhammanjhamman mentioned this pull request Mar 30, 2024
6 tasks
@jhammanjhamman added this to the 3.0.0.alpha milestone Apr 22, 2024
Sign up for freeto 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.

5 participants

@jhamman@pep8speaks@normanrz@rabernat@d-v-b