Skip to content

create a module for group metadata #3019

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Apr 24, 2025

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

closes #3018

@github-actions github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Apr 24, 2025
@github-actions github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Apr 24, 2025
@d-v-b d-v-b requested a review from jhamman April 24, 2025 19:22
@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 24, 2025

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

@d-v-b d-v-b requested a review from TomAugspurger April 24, 2025 22:50
@dstansby
Copy link
Contributor

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 #2876 (specifically, see #2876 (comment)) 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.

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 30, 2025

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

@dstansby
Copy link
Contributor

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.

@d-v-b
Copy link
Contributor Author

d-v-b commented Apr 30, 2025

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 d-v-b added this to the 3.1.0 milestone May 14, 2025
@d-v-b
Copy link
Contributor Author

d-v-b commented May 14, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

a group metadata module is missing
2 participants