Adding copy_store convenience method for Group

[melonora]

[Description of PR]
This PR adds a convenience method for copying the store of a Group to a destination store.

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/user-guide/*.md
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

@d-v-b

[codecov]

Codecov Report

❌ Patch coverage is 94.73684% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.52%. Comparing base (be62f5a) to head (41e3e64).

Files with missing lines	Patch %	Lines
src/zarr/core/group.py	94.73%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3612   +/-   ##
=======================================
  Coverage   93.52%   93.52%           
=======================================
  Files          90       90           
  Lines       11926    11962   +36     
=======================================
+ Hits        11154    11188   +34     
- Misses        772      774    +2     

Files with missing lines	Coverage Δ
src/zarr/testing/strategies.py	94.73% <ø> (ø)
src/zarr/core/group.py	95.17% <94.73%> (-0.03%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[melonora]

@d-v-b I know right now this won't work when zarr v2 is used. I can add that functionality as well or we say the convenience function is not supported for zarr v2.

[melonora]

added zarr v2 support. If in general the code is approved, I will add documentation.

[d-v-b]

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.

[melonora]

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 if member. metadata.consolidated_metadata when looping through members. WDYT should I add this?

[d-v-b]

I would copy the original group and its children exactly, including their consolidated metadata.

[melonora]

What would be the purpose of copying over stale consolidated metadata?

[d-v-b]

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.

[melonora]

regarding the from_store method, I would open a separate PR for this.

[joshmoore]

Consolidated metadata is already in the group metadata document for zarr v3 if "kind" is set to "inline".

👍 but doing something other (more or less) than just copying would require interpreting that which is too far IMO.

[d-v-b]

FYI @joshmoore the method under discussion here is a method on the Group class, not on stores. maybe this is a point of confusion.

[d-v-b]

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 members.

[melonora]

I added both, only thing remaining is probably adding consolidated_metadata in the async group call.

[melonora]

@d-v-b @joshmoore I think I have now addressed all comments and I also added documentation, so I think this would be ready for a final round of review. @d-v-b thanks for your patience.

[melonora]

@d-v-b @joshmoore , just bumping in case you have some time to see whether you approve the current state:)

[melonora]

@d-v-b When the last test passes and if you are happy with the code changes I think this is good to go. Thanks for reviewing!

[d-v-b]

@melonora I just realized that this implementation is decoding and re-encoding array chunks. Was there a particular reason for that choice? Because it would be far faster to copy the bytes directly

[melonora]

Hmm, mostly a lack of familiarity with getting the raw bytes, but looking at the documentation, I should be able to do something like store.get to get them, correct?
I will change it

[d-v-b]

you don't have to! I already did in a local branch. If it's OK with you, I would push those changes here.

[melonora]

Ah ok, thanks! Yes I am ok with it.

[melonora]

@d-v-b is this still of interest? I could resolve the conflicts or otherwise close it.

[d-v-b]

it is, sorry for the delay in dealing with things here. lmk if you run into any trouble with the merge conflicts

[melonora]

will do, thanks!

[melonora]

@d-v-b if you are ok with the current state, it can be merged

[d-v-b]

@melonora I'm going to give this a careful look today. I might identify some things in this PR that would be better off implemented in separate parts of the zarr codebase, do you mind if I push to your branch? some changes would come in that way, others might be spun off into separate PRs