Skip to content

Prevent update_attributes from erasing all prior attributes #2870

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

Merged
merged 3 commits into from
Feb 27, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/2870.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Prevent update_attributes calls from deleting old attributes
2 changes: 0 additions & 2 deletions src/zarr/core/array.py
Original file line number Diff line number Diff line change
Expand Up @@ -1609,8 +1609,6 @@ async def update_attributes(self, new_attributes: dict[str, JSON]) -> Self:
- The updated attributes will be merged with existing attributes, and any conflicts will be
overwritten by the new values.
"""
# metadata.attributes is "frozen" so we simply clear and update the dict
self.metadata.attributes.clear()
self.metadata.attributes.update(new_attributes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in principle we do want the metadata documents to be immutable, but as long as we are using plain python dicts for the attributes, immutability is not realistic. so updating in-place is fine.


# Write new metadata
Expand Down
1 change: 1 addition & 0 deletions src/zarr/core/attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
>>> attrs
{'a': 3, 'c': 4}
"""
self._obj.metadata.attributes.clear()

Check warning on line 53 in src/zarr/core/attributes.py

View check run for this annotation

Codecov / codecov/patch

src/zarr/core/attributes.py#L53

Added line #L53 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a separate bug fix to the change in zarr/core/array.py? If so, it should get it's own changelog entry, and test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to keep put (which formerly was using the undesired behavior in update_attributes) working as expected rather than a new feature or bugfix

self._obj = self._obj.update_attributes(d)

def asdict(self) -> dict[str, JSON]:
Expand Down
4 changes: 2 additions & 2 deletions src/zarr/core/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import functools
import operator
import warnings
from collections.abc import Iterable, Mapping
from collections.abc import Iterable, Mapping, Sequence
from enum import Enum
from itertools import starmap
from typing import (
Expand Down Expand Up @@ -37,7 +37,7 @@
ChunkCoordsLike = Iterable[int]
ZarrFormat = Literal[2, 3]
NodeType = Literal["array", "group"]
JSON = str | int | float | Mapping[str, "JSON"] | tuple["JSON", ...] | None
JSON = str | int | float | Mapping[str, "JSON"] | Sequence["JSON"] | None
MemoryOrder = Literal["C", "F"]
AccessModeLiteral = Literal["r", "r+", "a", "w", "w-"]

Expand Down
2 changes: 0 additions & 2 deletions src/zarr/core/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -1254,8 +1254,6 @@ async def update_attributes(self, new_attributes: dict[str, Any]) -> AsyncGroup:
-------
self : AsyncGroup
"""
# metadata.attributes is "frozen" so we simply clear and update the dict
self.metadata.attributes.clear()
self.metadata.attributes.update(new_attributes)

# Write new metadata
Expand Down
39 changes: 39 additions & 0 deletions tests/test_attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,42 @@ def test_asdict() -> None:
)
result = attrs.asdict()
assert result == {"a": 1, "b": 2}


def test_update_attributes_preserves_existing() -> None:
"""
Test that `update_attributes` only updates the specified attributes
and preserves existing ones.
"""
store = zarr.storage.MemoryStore()
z = zarr.create(10, store=store, overwrite=True)
z.attrs["a"] = []
z.attrs["b"] = 3
assert dict(z.attrs) == {"a": [], "b": 3}

z.update_attributes({"a": [3, 4], "c": 4})
assert dict(z.attrs) == {"a": [3, 4], "b": 3, "c": 4}


def test_update_empty_attributes() -> None:
"""
Ensure updating when initial attributes are empty works.
"""
store = zarr.storage.MemoryStore()
z = zarr.create(10, store=store, overwrite=True)
assert dict(z.attrs) == {}
z.update_attributes({"a": [3, 4], "c": 4})
assert dict(z.attrs) == {"a": [3, 4], "c": 4}


def test_update_no_changes() -> None:
"""
Ensure updating when no new or modified attributes does not alter existing ones.
"""
store = zarr.storage.MemoryStore()
z = zarr.create(10, store=store, overwrite=True)
z.attrs["a"] = []
z.attrs["b"] = 3

z.update_attributes({})
assert dict(z.attrs) == {"a": [], "b": 3}
9 changes: 7 additions & 2 deletions tests/test_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,10 @@ def test_group_update_attributes(store: Store, zarr_format: ZarrFormat) -> None:
assert group.attrs == attrs
new_attrs = {"bar": 100}
new_group = group.update_attributes(new_attrs)
assert new_group.attrs == new_attrs

updated_attrs = attrs.copy()
updated_attrs.update(new_attrs)
assert new_group.attrs == updated_attrs


async def test_group_update_attributes_async(store: Store, zarr_format: ZarrFormat) -> None:
Expand Down Expand Up @@ -1008,7 +1011,9 @@ async def test_asyncgroup_update_attributes(store: Store, zarr_format: ZarrForma
)

agroup_new_attributes = await agroup.update_attributes(attributes_new)
assert agroup_new_attributes.attrs == attributes_new
attributes_updated = attributes_old.copy()
attributes_updated.update(attributes_new)
assert agroup_new_attributes.attrs == attributes_updated


@pytest.mark.parametrize("store", ["local"], indirect=["store"])
Expand Down