Skip to content
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

StoreTests don't test Store.set_partial_values #2859

Open
TomAugspurger opened this issue Feb 24, 2025 · 3 comments
Open

StoreTests don't test Store.set_partial_values #2859

TomAugspurger opened this issue Feb 24, 2025 · 3 comments
Labels
bug Potential issues with the zarr-python library

Comments

@TomAugspurger
Copy link
Contributor

Zarr version

v3

Numcodecs version

na

Python Version

na

Operating System

Linux

Installation

source

Description

zarr.testing.store.StoreTests provides some unit tests that any class implementing the Store interface should pass. Currently, there aren't any tests that exercise the abstract Store.set_partial_values API.

Steps to reproduce

Spotted this while working on zarr-v3 support in kvikio: https://github.com/TomAugspurger/kvikio/tree/tom/zarr-v3

Additional output

No response

@TomAugspurger TomAugspurger added the bug Potential issues with the zarr-python library label Feb 24, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Feb 24, 2025

I suspect we have set_partial_values only because it's defined in the spec alongside set. In zarr-python we defined set to take a byte_range keyword argument, rendering set_partial_values is unnecessary. We should probably remove that method entirely. And maybe the spec should not have tried to define a store API.

@TomAugspurger
Copy link
Contributor Author

Gotcha, thanks. Although it looks like only some store classes support it, and it's not part of the Store abc yet. Happy to repurpose this issue to discuss that.

Would it be OK to add that to the zarr.abc.Store.set method, even with a default of None? Just unconditionally passing through a byte_range=... argument is liable to break stores that haven't been updated to accept that argument (even if they ignore it). I guess we have a couple options:

  1. Maybe that's the purpose of the supports_partial_writes abstract method? Should we look at that, and pass it through when that's True?
  2. Maybe use inspect.signature to see set accepts support_partial_writes keyword. This is a bit fragile, but might work.

And maybe the spec should not have tried to define a store API.

Yeah, IMO that seems a bit beyond the scope of the spec. I can see something like that being useful in the spec for examples, but not necessarily something that implementations should follow.

@d-v-b
Copy link
Contributor

d-v-b commented Feb 24, 2025

actually I was wrong -- StorePath.set takes a byte_range kwarg, but errors if it's not None. But StorePath.set has a different signature from Store.set -- the latter doesn't take a byte_range kwarg. Not sure why we have two store-like classes with subtly different set methods 🙃

to me it seems like the following could work:

  • Store.set takes a byte_range keyword argument, which defaults to None
  • Stores that don't support partial writes raise if invoked with a byte_range that's not None.
  • The caller is then responsible for handling this exception (e.g., by re-writing the entire object, or giving up)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Potential issues with the zarr-python library
Projects
None yet
Development

No branches or pull requests

2 participants