zarr-python icon indicating copy to clipboard operation
zarr-python copied to clipboard

create a module for group metadata

Open d-v-b opened this issue 8 months ago • 6 comments

puts the group metadata classes in their own module in metadata.

closes #3018

d-v-b avatar Apr 24 '25 19:04 d-v-b

this is failing CI because of cursed hypothesis test. The failure is not due to any changes in this PR.

d-v-b avatar Apr 24 '25 22:04 d-v-b

I'm confused about what our approach is to API in zarr.core. Here you're proposing to just move it without any deprecation period, but in https://github.com/zarr-developers/zarr-python/pull/2876 (specifically, see https://github.com/zarr-developers/zarr-python/pull/2876#issuecomment-2699625064) we seem to have decided that folks might be using stuff in zarr.core and therefore it should not be moved or deleted without deprecating.

I'm +1 to moving stuff around in core as we see fit (as long as it's clearly documented in release notes) without deprecation, since we don't document the zarr.core API, but either way we should have a consistent policy on whether API in zarr.core is considered public and needs deprecating, or if it's considered private and doesn't, and we should stick to this policy consistently.

dstansby avatar Apr 29 '25 09:04 dstansby

@dstansby since this PR is not creating any new public API, I think the issues in #2876 don't apply here. This PR does not change the symbols exportable from core/group.py, it just changes where those symbols are defined.

d-v-b avatar Apr 30 '25 06:04 d-v-b

The explicit decision in https://github.com/zarr-developers/zarr-python/pull/2876 was not to remove any existing API in zarr.core without at least one release with an alternative available. In other words, to move API in zarr.core there should be

  • one release where API is importable from the old and new places
  • then one release where the old place is deprecated
  • then one release that removes the old API

This PR skips the first step, because before it's possible to do from zarr.core.group import ConsolidatedMetadata, but this breaks after this PR.

dstansby avatar Apr 30 '25 09:04 dstansby

The explicit decision in #2876 was not to remove any existing API in zarr.core without at least one release with an alternative available. In other words, to move API in zarr.core there should be

* one release where API is importable from the old and new places

* then one release where the old place is deprecated

* then one release that removes the old API

This PR skips the first step, because before it's possible to do from zarr.core.group import ConsolidatedMetadata, but this breaks after this PR.

Aha, I see what you mean. Basically to comply with our operational policy, I need to import ConsolidatedMetadata in core/group.py. That's doable.

d-v-b avatar Apr 30 '25 10:04 d-v-b

Aha, I see what you mean. Basically to comply with our operational policy, I need to import ConsolidatedMetadata in core/group.py. That's doable.

I decided against doing this. Instead we should target this for a 3.1 release.

d-v-b avatar May 14 '25 08:05 d-v-b