(feat): typesize declared with constructor for Blosc

[ilan-gold]

See zarr-developers/zarr-python#2766 and zarr-developers/zarr-python#2171 - I checked locally the reproducer in the later for the compression ratio, and we now are getting the old performance with 1383 bytes

I think a good place to start is to declare the itemsize information, which is stored anyway on the BloscCodec

This PR does no validation of typesize, although it could in theory if the incoming buffer is a numpy array. I'm not sure that's desireable though as this is purely a performance thing (i.e., a "wrong" typesize doesn't actually break compression/decompression). If you're so low-level that you're changing this while using arrays, and you get it wrong, you'd probably also notice that your files have bad compression ratios

TODO:

• Unit tests and/or doctests in docstrings
• Tests pass locally
• Docstrings and API docs for any new/modified user-facing classes and functions
• Changes documented in docs/release.rst
• Docs build locally
• GitHub Actions CI passes
• Test coverage to 100% (Codecov passes)

[d-v-b]

this a good idea, but we should be sure that we don't cause backwards compatibility problems with this, since it adds a key to zarr metadata. How do various zarr clients like zarr-python or tensorstore handle extra keys in the blosc JSON definition?

[ilan-gold]

this a good idea, but we should be sure that we don't cause backwards compatibility problems with this, since it adds a key to zarr metadata.

Why does it add a key? One of the reason I feel so secure doing this is precisely that I thought he were adding the key anyway: https://github.com/zarr-developers/zarr-python/blob/680142f6f370d862f305a9b37f3c0ce2ce802e76/src/zarr/codecs/blosc.py#L122-L136 so I felt extremely confident making this change. Maybe I'm misunderstanding

[ilan-gold]

This feels similar to the Zstd levels business - it's encode only, and can be safely (i.e., nothing breaks) ignored for reading back in. I understand why we serialize it, but it seems people sometimes ignore it anyway (thinking of the NVComp Zstd) with no effect (the utility/quality of such a decision can be debated probably)

[d-v-b]

this a good idea, but we should be sure that we don't cause backwards compatibility problems with this, since it adds a key to zarr metadata.

Why does it add a key? One of the reason I feel so secure doing this is precisely that I thought he were adding the key anyway: https://github.com/zarr-developers/zarr-python/blob/680142f6f370d862f305a9b37f3c0ce2ce802e76/src/zarr/codecs/blosc.py#L122-L136 so I felt extremely confident making this change. Maybe I'm misunderstanding

aha, I didn't see that we were serializing this already. So there's no backwards compatibility issues to worry about. which is great

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (8168e15) to head (80ff632).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #713   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files          63       63           
  Lines        2754     2771   +17     
=======================================
+ Hits         2753     2770   +17     
  Misses          1        1           

Files with missing lines	Coverage Δ
numcodecs/tests/test_blosc.py	100.00% <100.00%> (ø)

[LDeakin]

this a good idea, but we should be sure that we don't cause backwards compatibility problems with this, since it adds a key to zarr metadata. How do various zarr clients like zarr-python or tensorstore handle extra keys in the blosc JSON definition?

Indeed! This was a breaking change after all for implementations that reject unknown keys, as it seemingly was not serialised in the past. See this diff in https://github.com/LDeakin/zarrs/pull/174/files.

[ilan-gold]

@LDeakin looking back, I misunderstood @d-v-b 's point - I thought he was referring to https://github.com/zarr-developers/zarr-python/blame/018f61d93207112f68eba06ea8c2560a489767f6/src/zarr/codecs/blosc.py#L127-L136 (which has no change) and not the fact that this codec is used directly by zarr v2 file-format

[d-v-b]

ok so this was actually a breaking change, that's my bad, I definitely should have reviewed this more carefully. But we also really need to be catching this with tests.

@jbms @bogovicj sorry for the inconvenience but you might have to change how your implementations handle the blosc codec in zarr v2 data.

[dstansby]

Sorry, I don't have time to follow in detail, but this isn't released yet (right?), so we could in theory revert it if it's too much of an issue?

[d-v-b]

it was released in 0.16

[dstansby]

Sorry, for some reason I thought this was zarr-python and not numcodecs 🤦

[jbms]

Please revert/fix this to minimize breakage, e.g. avoid serializing the typesize parameter when it is None.

Zarr-python/numcodecs also fails on unknown keys so this also breaks compatibility with previous versions of numcodecs/zarr-python.

In general backwards compatibility is tricky so it would be great if you can take care when making any changes to the metadata.