chore: switch disallow_forward_slash_in_h5ad default to True (#2436)

Author: flying-sheep

docs/release-notes/0.12.3.md

@@ -10,5 +10,7 @@
 - Deprecate `__version__` and use standard {func}`~importlib.metadata.version` API {user}`flying-sheep` ({pr}`1318`)
 - Allow writing of views of {class}`dask.array.Array` {user}`ilan-gold` ({pr}`2084`)
 - Enable writing of views of {class}`~anndata.AnnData` in backed mode {user}`ilan-gold` ({pr}`2092`)
-- Reallow writing of keys in `h5ad` files with forward slashes instead of erroring.  Now a warning will be raised that the behavior will be disallowed in the future.  To enable the new behavior, use {attr}`anndata.settings.disallow_forward_slash_in_h5ad`. {user}`ilan-gold` ({pr}`2097`)
+- Reallow writing of keys in `h5ad` files with forward slashes instead of erroring.
+  Now a warning will be raised that the behavior will be disallowed in the future.
+  To enable the new behavior, use {attr}`anndata.settings.disallow_forward_slash_in_h5ad`. {user}`ilan-gold` ({pr}`2097`)
 - Respect off-axis merge options in {func}`anndata.experimental.concat_on_disk` {user}`ilan-gold` ({pr}`2122`)

docs/release-notes/2436.chore.md

@@ -0,0 +1,2 @@
+Flip default for {attr}`anndata.settings.disallow_forward_slash_in_h5ad` to `True`.
+Document writing to `k="/"` in {func}`~anndata.io.write_elem` and check `k` more strictly {user}`flying-sheep`

src/anndata/_io/h5ad.py

@@ -28,6 +28,7 @@
 from .specs import read_elem, write_elem
 from .specs.registry import IOSpec, write_spec
 from .utils import (
+    _check_has_no_slash_key,
     _read_legacy_raw,
     idx_chunks_along_axis,
     no_write_dataset_2d,
@@ -86,6 +87,8 @@ def write_h5ad(
         f.attrs.setdefault("encoding-type", "anndata")
         f.attrs.setdefault("encoding-version", "0.1.0")
         for k, elem in iter_outer(adata):
+            _check_has_no_slash_key(k, elem)
+
             if k == "raw":
                 _write_raw(
                     f, adata.raw, as_dense=as_dense, dataset_kwargs=dataset_kwargs
@@ -139,9 +142,10 @@ def _write_raw(
     if "raw/X" in as_dense and isinstance(
         raw.X, CSMatrix | BaseCompressedSparseDataset
     ):
-        write_sparse_as_dense(f, "raw/X", raw.X, dataset_kwargs=dataset_kwargs)
-        write_elem(f, "raw/var", raw.var, dataset_kwargs=dataset_kwargs)
-        write_elem(f, "raw/varm", dict(raw.varm), dataset_kwargs=dataset_kwargs)
+        g = f.require_group("raw")
+        write_sparse_as_dense(g, "X", raw.X, dataset_kwargs=dataset_kwargs)
+        write_elem(g, "var", raw.var, dataset_kwargs=dataset_kwargs)
+        write_elem(g, "varm", dict(raw.varm), dataset_kwargs=dataset_kwargs)
     elif raw is not None:
         write_elem(f, "raw", raw, dataset_kwargs=dataset_kwargs)
 

src/anndata/_io/specs/methods.py

@@ -25,7 +25,11 @@
 from anndata._core.index import _normalize_indices
 from anndata._core.merge import intersect_keys
 from anndata._core.sparse_dataset import _CSCDataset, _CSRDataset, sparse_dataset
-from anndata._io.utils import check_key, zero_dim_array_as_scalar
+from anndata._io.utils import (
+    _check_has_no_slash_key,
+    check_key,
+    zero_dim_array_as_scalar,
+)
 from anndata._types import StorageType
 from anndata._warnings import OldFormatWarning
 from anndata.compat import (
@@ -340,19 +344,19 @@ def write_anndata(
 ):
     g = f.require_group(k)
     for sub_key, elem in iter_outer(adata):
-        if not (sub_key == "X" and elem is None):
-            if sub_key == "layers":
-                if None in elem:
-                    _writer.write_elem(
-                        g, "X", elem[None], dataset_kwargs=dataset_kwargs
-                    )
-                elem = {k: v for k, v in elem.items() if k is not None}
-            _writer.write_elem(
-                g,
-                sub_key,
-                dict(elem) if isinstance(elem, MutableMapping) else elem,
-                dataset_kwargs=dataset_kwargs,
-            )
+        if sub_key == "X" and elem is None:
+            continue
+        _check_has_no_slash_key(sub_key, elem)
+        if sub_key == "layers":
+            if None in elem:
+                _writer.write_elem(g, "X", elem[None], dataset_kwargs=dataset_kwargs)
+            elem = {k: v for k, v in elem.items() if k is not None}
+        _writer.write_elem(
+            g,
+            sub_key,
+            dict(elem) if isinstance(elem, MutableMapping) else elem,
+            dataset_kwargs=dataset_kwargs,
+        )
 
 
 @_REGISTRY.register_read(H5Group, IOSpec("anndata", "0.1.0"))

src/anndata/_io/specs/registry.py

@@ -364,42 +364,47 @@ def write_elem(
 
         from anndata._io.zarr import is_group_consolidated
 
-        # we allow stores to have a prefix like /uns which are then written to with keys like /uns/foo
-        is_zarr_group = isinstance(store, ZarrGroup)
-        if "/" in k.rsplit(store.name, maxsplit=1)[-1][1:]:
-            if is_zarr_group or settings.disallow_forward_slash_in_h5ad:
-                msg = f"Forward slashes are not allowed in keys in {type(store)}"
-                raise ValueError(msg)
-            else:
-                msg = "Forward slashes will be disallowed in h5 stores in the next minor release"
-                warn(msg, FutureWarning)
-
         if isinstance(store, h5py.File):
             store = store["/"]
-
-        dest_type = type(store)
-
-        # Normalize k to absolute path
-        if isinstance(store, h5py.Group) and not PurePosixPath(k).is_absolute():
-            k = str(PurePosixPath(store.name) / k)
-        is_consolidated = is_group_consolidated(store) if is_zarr_group else False
-        if is_consolidated:
+        elif is_group_consolidated(store, strict=False):
             msg = "Cannot overwrite/edit a store with consolidated metadata"
             raise ValueError(msg)
+
         if k == "/":
+            if store.name != "/":
+                msg = f"'/' is not in the subpath of {store.name!r}"
+                raise ValueError(msg)
+
             if isinstance(store, ZarrGroup):
                 from zarr.core.sync import sync
 
                 sync(store.store.clear())
             else:
                 store.clear()
-        elif k in store:
-            del store[k]
+        else:
+            # we allow stores to have a prefix like /uns which are then written to with keys like /uns/foo
+            if k.startswith("/"):
+                k = str(PurePosixPath(k).relative_to(store.name, walk_up=False))
+
+            # Apart from this code, we also ban keys containing slashes in `write_adata`/`write_h5ad`
+            # for AnnData elements other than `obs`, `var`, and `uns`.
+            if "/" in k:
+                if (
+                    isinstance(store, ZarrGroup)
+                    or settings.disallow_forward_slash_in_h5ad
+                ):
+                    msg = f"Forward slashes are not allowed in keys in {type(store)}"
+                    raise ValueError(msg)
+                msg = "Forward slashes will be written differently in a future anndata version"
+                warn(msg, FutureWarning)
+
+            if k in store:
+                del store[k]
 
         # Normalize array-API (e.g., JAX/CuPy) even if not AnnData
         elem = normalize_nested(elem)
 
-        write_func = self.find_write_func(dest_type, elem, modifiers)
+        write_func = self.find_write_func(type(store), elem, modifiers)
 
         if self.callback is None:
             return write_func(store, k, elem, dataset_kwargs=dataset_kwargs)
@@ -525,8 +530,9 @@ def write_elem(
     store
         The group to write to.
     k
-        The key to write to in the group. Note that absolute paths will be written
-        from the root.
+        The key to write into the group.
+        If the group is the root, set `k` to `"/"` to write directly into it.
+        Passing an absolute path referring to a direct child of the group is also allowed.
     elem
         The element to write. Typically an in-memory object, e.g. an AnnData, pandas
         dataframe, scipy sparse matrix, etc.

src/anndata/_io/utils.py

@@ -1,6 +1,6 @@
 from __future__ import annotations
 
-from collections.abc import Callable
+from collections.abc import Callable, Mapping
 from functools import WRAPPER_ASSIGNMENTS, cache, wraps
 from itertools import pairwise
 from typing import TYPE_CHECKING, Literal, cast
@@ -12,7 +12,7 @@
 from ..utils import warn
 
 if TYPE_CHECKING:
-    from collections.abc import Callable, Mapping
+    from collections.abc import Callable
     from typing import Any, Literal
 
     from pandas.core.dtypes.dtypes import BaseMaskedDtype
@@ -257,6 +257,7 @@ def report_write_key_on_error(func):
 
     @wraps(func)
     def func_wrapper(*args, **kwargs):
+        __tracebackhide__ = True
         from anndata._io.specs import Writer
 
         # Figure out signature (method vs function) by going through args
@@ -278,6 +279,16 @@ def func_wrapper(*args, **kwargs):
     return func_wrapper
 
 
+def _check_has_no_slash_key(attr: str, elem: object) -> None:
+    """Only attempt to write slash keys where people rely on it for backwards compatibility."""
+    if attr in {"obs", "var", "uns", "raw"}:
+        return  # separate check for `settings.disallow_forward_slash_in_h5ad` is done in `write_elem`
+    assert isinstance(elem, Mapping)
+    if any("/" in k for k in elem if k not in {"/", None}):
+        msg = f"Forward slashes are not allowed in keys in {attr}"
+        raise ValueError(msg)
+
+
 # -------------------------------------------------------------------------------
 # Common h5ad/zarr stuff
 # -------------------------------------------------------------------------------

src/anndata/_io/zarr.py

@@ -26,6 +26,8 @@
     from zarr.core.common import AccessModeLiteral
     from zarr.storage import StoreLike
 
+    from .._types import _GroupStorageType
+
 
 @contextmanager
 def zarrs_context():
@@ -175,8 +177,10 @@ def open_write_group(
     return zarr.open_group(store, mode=mode, **kwargs)
 
 
-def is_group_consolidated(group: zarr.Group) -> bool:
+def is_group_consolidated(group: _GroupStorageType, *, strict: bool = True) -> bool:
     if not isinstance(group, zarr.Group):
-        msg = f"Expected zarr.Group, got {type(group)}"
-        raise TypeError(msg)
+        if strict:
+            msg = f"Expected zarr.Group, got {type(group)}"
+            raise TypeError(msg)
+        return False
     return group.metadata.consolidated_metadata is not None

src/anndata/_settings.py

@@ -491,7 +491,7 @@ def validate_sparse_settings(val: Any, settings: SettingsManager) -> None:
 
 settings.register(
     "disallow_forward_slash_in_h5ad",
-    default_value=False,
+    default_value=True,
     description="Whether or not to disallow the `/` character in keys for h5ad files",
     validate=validate_bool,
     get_from_env=check_and_get_bool,

src/anndata/_settings.pyi

@@ -44,7 +44,7 @@ class _AnnDataSettingsManager(SettingsManager):
     zarr_write_format: Literal[2, 3] = 3
     use_sparse_array_on_read: bool = False
     min_rows_for_chunked_h5_copy: int = 1000
-    disallow_forward_slash_in_h5ad: bool = False
+    disallow_forward_slash_in_h5ad: bool = True
     write_csr_csc_indices_with_min_possible_dtype: bool = False
     auto_shard_zarr_v3: bool = True
 

src/anndata/_types.py

@@ -156,10 +156,10 @@ def __call__(
 class ReadCallback[S: StorageType, RWAble: typing.RWAble](Protocol):
     def __call__(
         self,
-        /,
         read_func: Read[S, RWAble],
         elem_name: str,
         elem: StorageType,
+        /,
         *,
         iospec: IOSpec,
     ) -> RWAble:
@@ -188,11 +188,11 @@ def __call__(
 class WriteCallback[RWAble: typing.RWAble](Protocol):
     def __call__(
         self,
-        /,
         write_func: Write[RWAble],
         store: StorageType,
         elem_name: str,
         elem: RWAble,
+        /,
         *,
         iospec: IOSpec,
         dataset_kwargs: Mapping[str, Any],