-
-
Notifications
You must be signed in to change notification settings - Fork 427
Adding copy_store convenience method for Group
#3612
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
e3ee33b
83550a3
d473d6e
bb1405e
c62543a
cdbc2f7
e6e10df
9c42567
63c652e
d4924f5
e83dda5
59b18ea
b65d257
1056b9e
eadb647
8c3471c
2cbb9b9
128b924
fa95e9c
848811f
3398325
ae205b5
06ad2d7
6b81a07
3e92ee9
0a4b5b8
418c670
f3e4f87
c37e69d
1abaab6
8a1e9e9
29c9dbb
db7c120
dee93de
eff4c05
41e3e64
71e4bef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -697,6 +697,73 @@ def from_dict( | |
| store_path=store_path, | ||
| ) | ||
|
|
||
| async def copy_store( | ||
| self, | ||
| store: StoreLike, | ||
| *, | ||
| overwrite: bool = False, | ||
| consolidate_metadata: bool | None = None, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this here? IMO it's simpler if copying is just copying. If people want to add consolidated metadata, they should do that after copying.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adjusted it. It now consolidates if the source store was consolidated, but this does not require an argument. One more thing, I only take into account when consolidation happened at the root level. It could be (although the user should not do this) that consolidation happened at the subgroup level. This could be accounted for by checking
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would copy the original group and its children exactly, including their consolidated metadata.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What would be the purpose of copying over stale consolidated metadata?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it keeps the scope of this function simple -- it just copies the original group exactly. If this tool produces a different group, then it's not really copying.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. regarding the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 but doing something other (more or less) than just copying would require interpreting that which is too far IMO.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI @joshmoore the method under discussion here is a method on the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO the only consolidated metadata-related parameter we need is whether iterating over the child groups uses their consolidated metadata (if present). See the relevant kwarg in the signature of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added both, only thing remaining is probably adding |
||
| ) -> AsyncGroup: | ||
| target_zarr_format = self.metadata.zarr_format | ||
|
|
||
| new_group = await AsyncGroup.from_store( | ||
| store, | ||
| overwrite=overwrite, | ||
| attributes=self.metadata.attributes, | ||
| zarr_format=target_zarr_format, | ||
| ) | ||
|
|
||
| async for _, member in self.members(max_depth=None): | ||
| child_path = member.store_path.path | ||
| target_path = StorePath(store=new_group.store, path=child_path) | ||
| if isinstance(member, AsyncGroup): | ||
| await async_api.group( | ||
| store=target_path, | ||
| overwrite=overwrite, | ||
| attributes=member.metadata.attributes, | ||
| zarr_format=target_zarr_format, | ||
| ) | ||
| else: | ||
| kwargs = {} | ||
| if target_zarr_format == 3: | ||
| kwargs["chunk_key_encoding"] = member.metadata.chunk_key_encoding | ||
| kwargs["dimension_names"] = member.metadata.dimension_names | ||
| else: | ||
| kwargs["chunk_key_encoding"] = { | ||
| "name": "v2", | ||
| "separator": member.metadata.dimension_separator, | ||
| } | ||
| # Serializer done this way in case of having zarr_format 2, otherwise mypy complains. | ||
| new_array = await new_group.create_array( | ||
| name=child_path, | ||
| shape=member.shape, | ||
| dtype=member.dtype, | ||
| chunks=member.chunks, | ||
| shards=member.shards, | ||
| filters=member.filters, | ||
| compressors=member.compressors, | ||
| serializer=member.serializer if member.serializer is not None else "auto", | ||
| fill_value=member.metadata.fill_value, | ||
| attributes=member.attrs, | ||
| overwrite=overwrite, | ||
| config={"order": member.order}, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| if target_zarr_format == 3: | ||
| for region in member._iter_shard_regions(): | ||
| data = await member.getitem(selection=region) | ||
| await new_array.setitem(selection=region, value=data) | ||
| else: | ||
| for region in member._iter_chunk_regions(): | ||
| data = await member.getitem(selection=region) | ||
| await new_array.setitem(selection=region, value=data) | ||
|
melonora marked this conversation as resolved.
Outdated
|
||
|
|
||
| if consolidate_metadata: | ||
| await async_api.consolidate_metadata(new_group.store) | ||
|
melonora marked this conversation as resolved.
Outdated
|
||
|
|
||
| return new_group | ||
|
|
||
| async def setitem(self, key: str, value: Any) -> None: | ||
| """ | ||
| Fastpath for creating a new array | ||
|
|
@@ -1874,6 +1941,21 @@ def open( | |
| obj = sync(AsyncGroup.open(store, zarr_format=zarr_format)) | ||
| return cls(obj) | ||
|
|
||
| def copy_store( | ||
| self, | ||
| store: StoreLike, | ||
| *, | ||
| overwrite: bool = False, | ||
| consolidate_metadata: bool | None = None, | ||
| ) -> Group: | ||
| return Group( | ||
| sync( | ||
| self._async_group.copy_store( | ||
| store=store, overwrite=overwrite, consolidate_metadata=consolidate_metadata | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| def __getitem__(self, path: str) -> AnyArray | Group: | ||
| """Obtain a group member. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.