Skip to content

feat(attributes): warn when writing attributes that are not valid JSON - #4400

Open
d-v-b wants to merge 3 commits into
zarr-developers:mainfrom
d-v-b:fix/attributes-json-deprecation
Open

d-v-b wants to merge 3 commits into
zarr-developers:mainfrom
d-v-b:fix/attributes-json-deprecation

Conversation

@d-v-b

@d-v-b d-v-b commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

This PR makes our attributes API stricter about accepting input that requires implicit casting (non-string keys) and values that are invalid JSON (NaN). The latter is load-bearing for xarray, so they will need to fix that behavior or use the configuration added in this PR to continue writing invalid JSON when we flip the switch on the default behavior

cc @keewis

🤖 AI text below 🤖

Refs #1125. This is the deprecation step toward the error that issue asks for.

Zarr stores attributes as JSON, but Python's json.dumps accepts two kinds of value that don't survive the trip to disk and back:

  • Non-string keys ({1: "a"}) are written as strings. The array or group you wrote through still reports {1: 'a'}, but reopening it gives {'1': 'a'}.
  • NaN and infinite floats are written as bare NaN / Infinity literals, which aren't valid JSON. Strict JSON parsers in other languages reject them.

The goal is to make the correct behavior the default, with a gradual deprecation and a way to keep the old behavior for anyone who needs it.

What this PR does

It adds two config options. Each takes "allow", "warn" or "raise":

Option Default Effect of the default
attributes.non_string_keys "warn" A ZarrFutureWarning names each offending key and says it will become an error. The value is still written as before.
attributes.non_finite_floats "allow" No change and no warning.

With the defaults, the only runtime change is the new warning. "raise" lets you opt in to the strict behavior now, and "allow" is the way back to the old behavior once the defaults change.

The check runs when metadata is serialized for writing, in to_buffer_dict on ArrayV2Metadata, ArrayV3Metadata and GroupMetadata. That covers every way of setting attributes: at creation, update_attributes, attrs.put, and attrs[k] = v. Reading is not checked, so opening existing data that has NaN in its attributes stays silent.

Only values that json.dumps currently writes without complaint are checked, at any depth inside the attributes. Keys of other types (tuples, objects) already make json.dumps raise TypeError, and that doesn't change. Warnings and errors list every offending location, e.g. attributes['a'][1].

Proposed deprecation schedule

  1. This PR: non-string keys warn; non-finite floats are allowed.
  2. Next: switch non_finite_floats to "warn" once xarray has a way off NaN literals. xarray writes _FillValue: NaN into attributes, and the last time zarr rejected those it was reverted within a day (convert inf, -inf, nan to JSON #3280). That's why this option starts at "allow" rather than "warn".
  3. Later: both default to "raise", with "allow" kept as the fallback.

Existing behavior left alone

These are unchanged here, since this PR changes nothing beyond the warning:

  • v3 groups (all four ways of setting attributes) and v2 group creation already raise TypeError for non-string top-level keys, from parse_attributes in group.py. That error still takes precedence over the policy. Nested non-string keys reach the new check on every node type.
  • Worth fixing before "raise" becomes the default: when a write is refused, the in-memory attributes have already been changed. Attributes.put clears the attributes dict in place, and AsyncGroup.update_attributes updates it in place, both before the metadata is saved. So after a refused write, the array or group object reports attributes that were never stored. v3 groups already behave this way with the existing TypeError.
  • For the synchronous API, metadata is serialized on the event-loop thread, so the warning's reported location isn't in the caller's code. The message names every offending attribute. This is the same limitation as the structured-dtype layout warning, which also uses skip_file_prefixes.

Tests

  • test_attributes_json_policies_write: valid JSON under the default and "raise" policies, non-string keys under "allow", and non-finite floats under the default. Each is written with no warning and read back as expected. It runs for v2 and v3 arrays and groups, both at creation and via update_attributes. The pytest config turns any unexpected warning into a failure.
  • One test for each outcome that warns or refuses: non-string keys warn by default; non-string keys under "raise" raise TypeError; non-finite floats under "warn" warn; non-finite floats under "raise" raise ValueError; an invalid policy value raises ValueError.

The full suite passes locally (10875 passed), and so do the docs example tests.

🤖 Generated with Claude Code

Attributes are stored as JSON, but `json.dumps` quietly accepts two kinds
of value that do not round-trip: non-string keys (written as strings) and
NaN / infinite floats (written as non-standard NaN / Infinity literals).

Add a policy for each, set with the config options
`attributes.non_string_keys` and `attributes.non_finite_floats`, taking
"allow", "warn" or "raise". Attributes are checked when metadata is
serialized for writing, so reading existing documents is unaffected.

The defaults keep today's behavior: non-string keys now emit a
ZarrFutureWarning saying they will become an error, and non-finite floats
are allowed without comment. "raise" opts in to the strict behavior now;
"allow" is the fallback once the defaults change.

Refs zarr-developers#1125

Assisted-by: ClaudeCode:claude-opus-5-5
Assisted-by: ClaudeCode:claude-opus-5-5
@github-actions github-actions Bot added needs release notes Automatically applied to PRs which haven't added release notes and removed needs release notes Automatically applied to PRs which haven't added release notes labels Sep 24, 2026
@codecov

codecov Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.38%. Comparing base (34b7c3d) to head (626fc33).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4400      +/-   ##
==========================================
+ Coverage   94.36%   94.38%   +0.02%     
==========================================
  Files          93       93              
  Lines       13142    13196      +54     
==========================================
+ Hits        12401    12455      +54     
  Misses        741      741              
Files with missing lines Coverage Δ
src/zarr/core/config.py 100.00% <ø> (ø)
src/zarr/core/group.py 95.23% <100.00%> (+0.02%) ⬆️
src/zarr/core/metadata/common.py 100.00% <100.00%> (ø)
src/zarr/core/metadata/v2.py 89.44% <100.00%> (+0.05%) ⬆️
src/zarr/core/metadata/v3.py 95.46% <100.00%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@d-v-b
d-v-b marked this pull request as ready for review September 24, 2026 11:12
@d-v-b

d-v-b commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

requesting review because this starts a deprecation path

@d-v-b d-v-b added this to the 3.5.0 milestone Sep 27, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant