StoreTests don't test Store.set_partial_values #2859

Activity

1. TomAugspurger

   added

   bugPotential issues with the zarr-python libraryPotential issues with the zarr-python library

   on Feb 24, 2025
2. d-v-b commented on Feb 24, 2025

   

   d-v-b

   on Feb 24, 2025

   Contributor

   More actions

   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.
3. TomAugspurger commented on Feb 24, 2025

   

   TomAugspurger

   on Feb 24, 2025

   ContributorAuthor

   More actions

   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.
4. d-v-b commented on Feb 24, 2025

   

   d-v-b

   on Feb 24, 2025

   Contributor

   More actions

   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)
5. shoyer commented on Aug 27, 2025

   

   shoyer

   on Aug 27, 2025

   Contributor

   More actions

   I'm not sure Store.set_partial_values() (or any form of partial writes) is currently being used at all. When I search for set_partial_values in the Zarr-Python codebase, I can only find implementations in Store class and their test.

   This suggests to me that maybe we don't need set_partial_values after all?

   👍React with 👍1Reacted by Davis Bennett
6. d-v-b

   closed this as completedin #3413on Aug 29, 2025