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

Make zarr.core private

Open jhamman opened this issue 1 year ago • 13 comments

We seem to be converging on using a leading underscore to indicate private modules. Should we do this for zarr.core or can we simply document the intent of the module?

jhamman avatar Jan 02 '25 15:01 jhamman

Is it intended to be private? If so, I'd definitely add a leading underscore to mark it as private.

dstansby avatar Jan 03 '25 22:01 dstansby

Yes, the intent is to make it private.

jhamman avatar Jan 03 '25 22:01 jhamman

I had a quick go, and there seem to be bits that need to be public (at least zarr.core.config, zarr.core.buffer.Buffer). Making the whole of core private is going to need someone who knows the codebase well enough (definitely not me 😄) to know what should be public going through carefully and decided what should be public and what should be private.

dstansby avatar Jan 03 '25 22:01 dstansby

I can take this on.

jhamman avatar Jan 03 '25 23:01 jhamman

As some small first steps I cleaned up zarr.core.buffer (https://github.com/zarr-developers/zarr-python/pull/2641) and zarr.core.metadata (https://github.com/zarr-developers/zarr-python/pull/2642)

dstansby avatar Jan 04 '25 10:01 dstansby

@dstansby - what makes you think zarr.core.config needs to be public? The intent, as I see it, is just that zarr.core.config.config is public which is why it is available as zarr.config.

@normanrz - do you think we could move zarr.core.buffer to zarr.buffer? The Buffer ABC seems like it actually belongs in the abc module.

jhamman avatar Jan 06 '25 05:01 jhamman

what makes you think zarr.core.config needs to be public?

There's user facing documentation in the docstring, but maybe that should now be deleted in favour of the user guide? It looks like BadConfigError isn't used so can be removed, but I think Config should be user facing as it's the type of zarr.config, so maybe that could move to zarr.Config?

dstansby avatar Jan 06 '25 10:01 dstansby

@normanrz - do you think we could move zarr.core.buffer to zarr.buffer? The Buffer ABC seems like it actually belongs in the abc module.

Yes, we could do that. And yes, the Buffer and NDBuffer make sense in the abc module.

normanrz avatar Jan 06 '25 16:01 normanrz

Part 1 done here -> https://github.com/zarr-developers/zarr-python/pull/2664

jhamman avatar Jan 07 '25 00:01 jhamman

update: a fairly major gotcha, see this comment comment...

https://github.com/zarr-developers/numcodecs/blob/6c0ea0f43788f22e4ad5d6655862c6c212a0dece/numcodecs/zarr3.py#L51-L54

jhamman avatar Jan 07 '25 00:01 jhamman

This came up again in https://github.com/zarr-developers/zarr-python/pull/2876#pullrequestreview-2725267688. I'm going to reopen this and look into how we could move everything to zarr._core and deprecate the module with a warning.

TomAugspurger avatar Mar 28 '25 15:03 TomAugspurger

Assuming we are all aligned on the value of making core explicitly private by renaming it to _core, do we think this change would fit in a 3.1 release?

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

xref https://github.com/zarr-developers/zarr-python/pull/2876.

Once that's released we can (conditionally) update the imports in numcodecs to use the public API (and IIRC Icechunk had some references to zarr.core.buffer too)

Then once numcodecs has a public release with that we can begin the deprecation.

TomAugspurger avatar Jun 07 '25 14:06 TomAugspurger