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

Different Objects Pointing at the Same store_path can have different metadata #2716

Open
ilan-gold opened this issue Jan 15, 2025 · 13 comments
Labels
bug Potential issues with the zarr-python library

Comments

@ilan-gold
Copy link
Contributor

ilan-gold commented Jan 15, 2025

Zarr version

v3.0.0

Numcodecs version

0.14.1

Python Version

3.12

Operating System

Mac

Installation

uv

Description

Something very subtle appears to be going on here when doing something like my_group["/"] - previously in v2, this operation was idempotent in that my_group == my_group["/"].

I am not sure about how this sort of thing is actually supposed to work, or if the old behavior itself was buggy.

Steps to reproduce

v2/v3 code:

import zarr
z = zarr.open("foo.zarr")
z["/"].attrs.setdefault("foo", "bar")
assert z["/"].attrs.asdict() == { "foo": "bar" }
assert z.attrs.asdict() == { "foo": "bar" } # fails in v3
assert z["/"] == z # fails in v3

Additional output

No response

@ilan-gold ilan-gold added the bug Potential issues with the zarr-python library label Jan 15, 2025
@d-v-b
Copy link
Contributor

d-v-b commented Jan 15, 2025

if the name of the root group is '', and <name>/ and <name> are treated equivalently, then '/' should resolve to the root of the hierarchy, i.e. the group itself. What's the error in v3?

@ilan-gold
Copy link
Contributor Author

@d-v-b I think that's the expected behavior, but that does not appear to be what is happening in v3. The final two asserts fails whereas I would expect them to succeed under your interpretation (and they do indeed pass under v2)

@ilan-gold
Copy link
Contributor Author

Ok the issue here is actually quite deep, and may have tentacles elsewhere unforeseen.

In short, because z and z["/"] are two different objects which have their metadata "cached" on the object, the original z never sees the updated metadata.

But this could happen with any two accesses:

import zarr
z = zarr.open("foo.zarr")
location_group = z.create_group("location")
same_location_group_but_different_object = z["location"]
location_group.attrs.setdefault("foo", "bar")
assert location_group.attrs.asdict() == { "foo": "bar" }
assert same_location_group_but_different_object.attrs.asdict() == { "foo": "bar" } # fails in v3
assert same_location_group_but_different_object == location_group # fails, probably for the best

The failing final check here and above make sense in that the attributes are different, but not in that it shouldn't happen.

@ilan-gold ilan-gold changed the title Forward-slash access leads to inconsistent behavior from v2 -> v3 Different Objects Pointing at the Same store_path can have different metadata Jan 20, 2025
@ilan-gold
Copy link
Contributor Author

I could see the way forward here as checking if the store is accepting alterations (write, append etc.) and then force re-reads of the metadata in this case. That would preserve the performance boost for read-only copies.

@ilan-gold
Copy link
Contributor Author

Another option would be ensuring metadata objects are always shared, but this could maybe get hairy with consistency...Although I suppose the other option could as well.

@d-v-b
Copy link
Contributor

d-v-b commented Jan 20, 2025

zarr-python 2.x had a cache_metadata parameter for arrays / groups which controlled whether the contents of .zattrs would be cached. Maybe we need something similar.

correction: cache_attrs controlled caching of .zattrs, cache_metadata controlled caching of the array metadata itself.

@ilan-gold
Copy link
Contributor Author

What was the default value for that though? I would think that we would want it to be False or no? Was this sort of thing possible in v2?

@d-v-b
Copy link
Contributor

d-v-b commented Jan 20, 2025

for arrays the default values for both was True:

zarr-python/zarr/core.py

Lines 125 to 126 in b1480d7

cache_metadata=True,
cache_attrs=True,
.

And for that reason this kind of thing was possible in v2 -- if cache_attrs was true, and you accessed the same array / group twice (resulting in two handles), you could mutate the attributes of one handle and thereby make the cache on the other go stale.

There are situations where you can be sure that the attributes won't change, and in those cases caching saves IO, but there are many other situations, and for them we should definitely expose a way to not cache the attributes.

@ilan-gold
Copy link
Contributor Author

Perhaps the difference is that this example is a group? I checked and this issue does not occur in v2. Maybe I am wrong though?

@ilan-gold
Copy link
Contributor Author

I've tried my hand at this for a few hours and it is quite difficult because of how the AsyncGroup class is structured. Declaring a group with its metadata bakes in a lot of assumptions...Fundamentally I'm not sure how to update one object from another without re-reading the data, but that is fraught with SyncError issues because so much of the architecture of AsyncGroup is built on metadata being available non-async and only in-memory.

@d-v-b
Copy link
Contributor

d-v-b commented Jan 22, 2025

thanks for working on this @ilan-gold, if I have some time soon I will try running your example and I will see if I can explain what's going on.

@ilan-gold
Copy link
Contributor Author

Ok @d-v-b I found out why it works previously in v2. The first access to attrs from the new object reads from disk. After that, it is cached. Everything is indeed cache-by-default as you posted in the param, groups included.

So here I can get it to fail:

import zarr
z = zarr.open("foo.zarr")
location_group = z.create_group("location")
same_location_group_but_different_object = z["location"]
same_location_group_but_different_object.attrs.asdict() # access first from the different location before the update
location_group.attrs.setdefault("foo", "bar")
assert location_group.attrs.asdict() == { "foo": "bar" }
assert same_location_group_but_different_object.attrs.asdict() == { "foo": "bar" } # fails in v3
assert same_location_group_but_different_object == location_group # fails, probably for the best

@ilan-gold
Copy link
Contributor Author

Given this fact, I am tempted to close the issue as no-fix, pending (hopefully) a note added to the zarr documentation about this fact (all metadata is declaration-time-only valid). I think in general, it could be good to add something about the consistency guarantees of a zarr object. I wonder if other folks in this project are aware of other thorny sides around read-write consistency perhaps (maybe the icechunk folks?).

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