[flying-sheep]

This and the two PRs below it are I need in 0.14 (for scanpy 2.0 preview), except for .uns accessors.

See also zarr-developers/zarr-specs#128

• Closes Allow any unicode incl. / and \ in obsm keys, obs columns, uns keys, … #2450
• Tests added

• Release note not necessary because:

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.05%. Comparing base (2e0a84c) to head (cd2b842).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@               Coverage Diff                @@
##           feat/json-io    #2662      +/-   ##
================================================
+ Coverage         85.94%   86.05%   +0.11%     
================================================
  Files                50       50              
  Lines              8329     8400      +71     
================================================
+ Hits               7158     7229      +71     
  Misses             1171     1171              

Files with missing lines	Coverage Δ
src/anndata/_io/h5ad.py	92.88% <100.00%> (ø)
src/anndata/_io/specs/methods.py	92.34% <100.00%> (+0.10%)	⬆️
src/anndata/_io/specs/registry.py	95.15% <100.00%> (+0.13%)	⬆️
src/anndata/_io/utils.py	82.53% <100.00%> (+3.69%)	⬆️
src/anndata/_io/zarr.py	87.21% <100.00%> (ø)
src/anndata/_write_compat.py	96.66% <100.00%> (+0.11%)	⬆️
src/anndata/experimental/backed/_io.py	93.44% <100.00%> (+0.10%)	⬆️
src/anndata/experimental/merge.py	88.80% <100.00%> (+0.41%)	⬆️

[ilan-gold]

Discussion is a bit mangled now, but scverse/zarr-convention-dataframe#1 is quite a strong suggestion that seems simpler to implement. Maybe let's discuss there and then I will implement there (and you here) whatever mechanism we settle on (your implementation here of other data frame things does not have to match mine otherwise)

[read-the-docs-business]

Documentation build overview

📚 anndata | 🛠️ Build #4656666 | 📁 Comparing cd2b842 against latest (c4f5138)

  🔍 Preview build  

156 files changed · + 8 added · ± 147 modified · - 1 deleted

+ Added

• write-compat.html
• generated/anndata.AnnData.__contains__.html
• generated/anndata.AnnData.__getitem__.html
• generated/anndata.AnnData.__sizeof__.html
• generated/anndata.WriteCompat.html
• generated/anndata.WriteConfig.html
• generated/anndata.experimental.AnnCollection.__getitem__.html
• generated/anndata.types.WriteCompatStr.html

± Modified

• _key_contributors.html
• accessors.html
• api.html
• contributing.html
• fileformat-prose.html
• genindex.html
• index.html
• interoperability.html
• py-modindex.html
• references.html
• and 137 more...

- Deleted

• _static/sbt-webpack-macros.html

[selmanozleyen]

compat needs to be passed through write_use.

Otherwise these fail:

@pytest.mark.parametrize(
    "existing", ["x", "x/y"], ids=["unescaped-group", "escaped-group"]
)
@pytest.mark.parametrize("key", ["a/b", "100%"])
@pytest.mark.parametrize(
    "use", [lambda k: A.obsm[k], lambda _: A.obsm], ids=["single-key", "mapping"]
)
def test_use_escapes_keys(
    diskfmt_store: Path | MemoryStore,
    diskfmt: Literal["h5ad", "zarr"],
    existing: str,
    key: str,
    use: Callable[[str], object],
) -> None:
    """`use=` escapes keys like a full write with the same `compat` does.

    `100%` into an already escaped group is the silent case:
    written literally, it would be read back as an escape sequence.
    """
    adata = ad.AnnData(np.zeros((2, 3)), obsm={existing: np.zeros((2, 2))})
    getattr(adata, f"write_{diskfmt}")(diskfmt_store, compat="0.14")

    adata.obsm[key] = np.ones((2, 2))
    getattr(adata, f"write_{diskfmt}")(diskfmt_store, use=use(key), compat="0.14")
    back = getattr(ad, f"read_{diskfmt}")(diskfmt_store)

    assert set(back.obsm) == {existing, key}
    np.testing.assert_array_equal(back.obsm[key], np.ones((2, 2)))

[ilan-gold]

anndata/src/anndata/_settings.py

Line 25 in c8bbe5a

disallow_forward_slash_in_h5ad: bool = True

should be removed wholesale (and maybe a deprecation cycle kicked off)

Unrelated, going to suggest taking a page out of Davis' Bennet's book, and having Claude do an adversarial test of this. Can Claude come up with use-cases where this breaks?

[ilan-gold]

Why only mapping? #2039 seems to be for data frames

[flying-sheep]

I removed the data frame part because you asked for alignment with the data frame convention, which no longer uses escaping (while I think that potentially nested mappings might benefit from escaping)

I originally had the data frame part in here but removed it in order to not impose two new specs on other implementers in possibly quick succession. I thought that’s what you wanted.

Please tell me if you want working slashes in data frame keys now (at the cost of two spec changes for data frames) or hamstrung data frames until the convention is done.

[flying-sheep]

OK, did the adversarial review, it found the following issues with this PR:

• read_lazy returns the escaped keys
• concat_on_disk merges escaped and unescaped keys because it operates at the raw storage level
• unescape_key doesn’t error on all malformed keys
• passing the wrong type as key (e.g. int) now has a worse message

It also found these, which I’d say we ignore:

• zarr v2 reserved names aren’t covered

It also flagged these issues that aren’t ours:

• Zarr can’t deal with quirks like windows path character limit, case-insensitive filenames, and the old MacOS HFS+ file system’s quirks – I say we gotta draw the line about what we want to mitigate

  I gotta say though that this is the first big HDF5 win compared to zarr – not using file names just avoids a bait shop’s whole inventory of cans of worms

  That does make me think though – maybe we should go back on the escaping idea and instead use fixed key and attribute names (like in the data frame convention) as it avoids all these issues.

• fsspec has a bad bug that affects the escaping scheme: Inconsistencies regarding quoting / escaping in URLs / paths fsspec/filesystem_spec#1713

  We should address that ASAP before merging. I don’t want to use a different escape scheme just because of that as it’s a horrible bug for zarr-over HTTP that can’t remain unfixed