Public API for buffer objects #2876

TomAugspurger · 2025-02-28T15:56:36Z

This moves the public imports from buffer things out of zarr.core.buffer.

Abstract stuff is availble under zarr.abc.buffer.
Concrete implementations are available under zarr.buffer.{cpu,gpu}.

I haven't added any new tests, but I updated the tests in tests/test_buffer.py to use the public API.

This moves the public imports from buffer things out of `zarr.core`. Abstract stuff is availble under `zarr.abc.buffer`. Concrete implementations are available under `zarr.buffer.{cpu,gpu}`.

dstansby · 2025-03-04T20:55:41Z

Do you think it's worth just moving the actual code to the new public location, so it doesn't live in zarr.core.buffer any more? That would make for a simpler code base and be my preference, but I might be missing some reason not to move the code.

TomAugspurger · 2025-03-05T02:46:57Z

Mmm, I think I'd still want the implementation to be in a private module so that implementation details like numpy imports don't leak into the public API's namespace. So it'd end up looking pretty similar. And if we're moving stuff out of zarr.core I'd want to do it properly, since I wouldn't be surprised if people were already using it. IMO not worth it right now.

TomAugspurger · 2025-03-05T17:29:36Z

I wouldn't be surprised if people were already using it.

Yep: https://github.com/earth-mover/icechunk/blob/584d4f2b0aad1160f8e4ba9a48ebbf770c058c00/icechunk-python/python/icechunk/store.py#L13

I think this PR takes care of those zarr.core.buffer imports. IMO, we should ensure that all the types in our public API (like BytesLike) are exported somewhere, but that should be done separately.

dstansby

I left some comments/suggestions inline.

I also think we should move the code to where we expect users to import it from. This is because:

having code in zarr.core.buffer, which we expcititly mark as private API, could lead developers to think they can make API changes without deprecations. But in reality it's public API.
It forces us to import code in our tests from where users are also importing it

I appreciate it's a bit more work to move the code around, but that's the cost of being a stable and widely used library 😄

And if we're moving stuff out of zarr.core I'd want to do it properly, since I wouldn't be surprised if people were already using it.

We decided that the API in zarr.core is private, so I think we should feel free to do what we want there - if downstream users are using it then I don't think that's our problem, since we don't document it's existence. On a practical level I can appreciate just moving the code might cause issues, in which case we could still have the code importable from zarr.core, but issue deprecation warnings before removing it completely.

src/zarr/buffer/__init__.py

dstansby · 2025-03-28T11:48:51Z

src/zarr/buffer/__init__.py

+"""
+
+from ..core.buffer import default_buffer_prototype
+from . import cpu, gpu


I don't think we import submodules like this anywhere else? If we don't, to be consistent I would remove these imports, and we can always discuss doing this throughout the package in an issue.

It doesn't look like cpu and gpu have been removed as of the latest commit? Because we don't do this kind of import elsewhere (e.g., see zarr/api/__init__.py, that doesn't import synchronous or asyncrhonous), I think we should be consistent and not import cpu or gpu here.

Sorry I misunderstood your earlier comment.

This is to ease migration for users: Previously import zarr would make, e.g. zarr.core.buffer.cpu.Buffer available, as a side-effect of us importing modules internally.

The closest analog here is zarr.storage, which imports all store subclasses, rather than making users use the full import path. We can't do that exact same thing here hough, since both .gpu and .cpu use the same names (Buffer, NDBuffer, etc), so the imports need to be namespaced.

We could just not import cpu and gpu and make the user do that import, but in this case I think consistency with zarr.store is most important.

src/zarr/buffer/__init__.py

TomAugspurger · 2025-03-28T14:30:14Z

I don't plan to move the implementation at this time. We already know that some libraries, like icechunk, are depending on it. I don't want to break those implementations because Zarr lacked a public API for this previously. We need to offer a public API first and then give them some time to migrate over.

I'll post a proposed plan over in #2621.

TomAugspurger · 2025-03-28T15:08:50Z

Should be good to go.

dstansby

As well as my inline comments/suggestions, I noticed that there are parts of the API that are typed with zarr.core.buffer... - it would be good to change those to use the new public interface. As an example, zarr.abc.codec.ArrayArrayCodec

dstansby · 2025-03-28T15:12:02Z

docs/user-guide/extending.rst

@@ -83,7 +83,10 @@ Coming soon.
 Custom array buffers
 --------------------

-Coming soon.
+zarr-python provides control where and how arrays stored in memory through


Suggested change

zarr-python provides control where and how arrays stored in memory through

Zarr-Python provides control for where and how arrays stored in memory through

We're inconsistent about zarr-python vs. Zarr-python in the docs.

dstansby · 2025-03-28T15:12:41Z

docs/user-guide/extending.rst

-Coming soon.
+zarr-python provides control where and how arrays stored in memory through
+:mod:`zarr.buffer`. Currently both CPU (the default) and GPU implementations are
+provided (see :ref:`user-guide-gpu` for more). You can implement your own buffer


Suggested change

provided (see :ref:`user-guide-gpu` for more). You can implement your own buffer

provided (see :ref:`user-guide-gpu` for more info). You can implement your own buffer

I think the current version sounds OK.

dstansby · 2025-03-28T15:15:31Z

src/zarr/buffer/__init__.py

+"""
+
+from ..core.buffer import default_buffer_prototype
+from . import cpu, gpu


It doesn't look like cpu and gpu have been removed as of the latest commit? Because we don't do this kind of import elsewhere (e.g., see zarr/api/__init__.py, that doesn't import synchronous or asyncrhonous), I think we should be consistent and not import cpu or gpu here.

dstansby · 2025-03-28T15:18:47Z

I don't plan to move the implementation at this time. We already know that some libraries, like icechunk, are depending on it. I don't want to break those implementations because Zarr lacked a public API for this previously. We need to offer a public API first and then give them some time to migrate over.

👍 - my suggestion would be to introduce those deprecations in this PR for the buffer stuff that's been made public, but happy to punt that to a later PR if that's easier.

TomAugspurger · 2025-03-28T19:13:42Z

The latest pair of commits updates our zarr.config, which references zarr.core.buffer. We needed to slightly update our registration code to let the class being registered have a different qualname / import path than where they're defined.

TomAugspurger · 2025-03-28T19:34:14Z

Thanks for the review.

I noticed that there are parts of the API that are typed

This is proving to be a bit more work than I can do right now. Feel free to push a commit if you're able to, or we can merge this as is since and handle that later.

TomAugspurger · 2025-03-28T19:35:05Z

👍 - my suggestion would be to introduce those deprecations in this PR for the buffer stuff that's been made public, but happy to punt that to a later PR if that's easier.

I'd recommend waiting. A release with this public API will give us a chance to migrate packages like icechunk without their users ever seeing a warning.

Public API for buffer objects

d4be973

This moves the public imports from buffer things out of `zarr.core`. Abstract stuff is availble under `zarr.abc.buffer`. Concrete implementations are available under `zarr.buffer.{cpu,gpu}`.

github-actions bot added the needs release notes Automatically applied to PRs which haven't added release notes label Feb 28, 2025

changelog

efa674d

github-actions bot removed the needs release notes Automatically applied to PRs which haven't added release notes label Mar 1, 2025

dstansby reviewed Mar 28, 2025

View reviewed changes

TomAugspurger added 3 commits March 28, 2025 09:17

Merge remote-tracking branch 'upstream/main' into tom/fix/public-buffers

ef2da4d

absolute imports

4ff4f7e

fixed warning in doc build

7946745

TomAugspurger mentioned this pull request Mar 28, 2025

Make zarr.core private #2621

Closed

dstansby requested changes Mar 28, 2025

View reviewed changes

TomAugspurger added 2 commits March 28, 2025 13:01

Updated config

5cf1bde

Updated config

50792a4

wording

674ca51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public API for buffer objects #2876

Public API for buffer objects #2876

TomAugspurger commented Feb 28, 2025 •

edited

Loading

dstansby commented Mar 4, 2025

TomAugspurger commented Mar 5, 2025

TomAugspurger commented Mar 5, 2025

dstansby left a comment

dstansby Mar 28, 2025

dstansby Mar 28, 2025

TomAugspurger Mar 28, 2025

TomAugspurger commented Mar 28, 2025 •

edited

Loading

TomAugspurger commented Mar 28, 2025

dstansby left a comment

dstansby Mar 28, 2025

TomAugspurger Mar 28, 2025

dstansby Mar 28, 2025

TomAugspurger Mar 28, 2025

dstansby Mar 28, 2025

dstansby commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

	zarr-python provides control where and how arrays stored in memory through
	Zarr-Python provides control for where and how arrays stored in memory through

	provided (see :ref:`user-guide-gpu` for more). You can implement your own buffer
	provided (see :ref:`user-guide-gpu` for more info). You can implement your own buffer

Public API for buffer objects #2876

Are you sure you want to change the base?

Public API for buffer objects #2876

Conversation

TomAugspurger commented Feb 28, 2025 • edited Loading

dstansby commented Mar 4, 2025

TomAugspurger commented Mar 5, 2025

TomAugspurger commented Mar 5, 2025

dstansby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Mar 28, 2025 • edited Loading

TomAugspurger commented Mar 28, 2025

dstansby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

TomAugspurger commented Mar 28, 2025

TomAugspurger commented Feb 28, 2025 •

edited

Loading

TomAugspurger commented Mar 28, 2025 •

edited

Loading