add numcodec protocol

[d-v-b]

This PR adds a protocol to model numcodecs.abc.Codec. The motivation for this protocol is to ensure that we can process external codecs that adhere to the numcodecs API without needing numcodecs as a dependency.

TODO:

• Add unit tests and/or doctests in docstrings
• Add docstrings and API docs for any new/modified user-facing classes and functions
• New/modified features documented in docs/user-guide/*.rst
• Changes documented as a new file in changes/
• GitHub Actions have all passed
• Test coverage is 100% (Codecov passes)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.55%. Comparing base (18419f0) to head (194e70d).
⚠️ Report is 86 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3318      +/-   ##
==========================================
+ Coverage   94.51%   94.55%   +0.04%     
==========================================
  Files          78       79       +1     
  Lines        9423     9448      +25     
==========================================
+ Hits         8906     8934      +28     
+ Misses        517      514       -3     

Files with missing lines	Coverage Δ
src/zarr/abc/codec.py	95.06% <100.00%> (+0.77%)	⬆️
src/zarr/abc/numcodec.py	100.00% <100.00%> (ø)
src/zarr/api/asynchronous.py	87.62% <ø> (ø)
src/zarr/api/synchronous.py	92.95% <ø> (ø)
src/zarr/codecs/_v2.py	93.61% <100.00%> (-0.14%)	⬇️
src/zarr/core/_info.py	95.18% <100.00%> (ø)
src/zarr/core/array.py	97.10% <100.00%> (-0.01%)	⬇️
src/zarr/core/metadata/v2.py	91.27% <100.00%> (ø)
src/zarr/registry.py	88.81% <100.00%> (+0.23%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[d-v-b]

The way our code coverage works, class and function definitions are not counted as tested if they are imported by pytest prior to running actual tests. Pytest imports from a lot of our library due to fixtures defined in conftest.py, which is why our coverage reports in CI report function definitions as uncovered.

This means adding new functions always reduces coverage, because the function definition is uncovered. Until we fix how our coverage is measured, I excluded instances of NotImplementedError with #pragma: no cover to get coverage green for this PR.

[dstansby]

There's a lot of type: ignore comments currently dotted throughout - what's the reason for that? Is it perhaps because numcodecs.abc.Codec doesn't currently conform to the new protocol?

[d-v-b]

this particular example is a test, and if you look a few lines above you will see that compressors is either None or the string "auto", and mypy models this as str | None, leading to this error:

tests/test_api.py:1285: error: Argument "compressors" to "create_array" has incompatible type "str | None"; expected "CompressorsLike" [arg-type]

I suspect the other type: ignore statements were added for different reasons

[d-v-b]

Is it perhaps because numcodecs.abc.Codec doesn't currently conform to the new protocol?

Spinning this question out into the main thread: numcodecs.abc.Codec does not have type annotations for its methods, and even if it did, decode would need to be annotated as returning object thanks to Pickle.decode, which can return literally any python object.

without type annotations on numcodecs.abc.Codec, I don't think we can use a type checker to check if a numcodecs codec is assignable to Numcodec, but this PR does add runtime checks, which are tested.

[dstansby]

To make sure I understand, does passing an existing numcodecs codec to compressors or filters fail type checking in user code with this PR (because they don't conform to the new protocol)?

[martindurant]

even if it did, decode would need to be annotated as returning object thanks to Pickle.decode

Shouldn't it annotate our expectations, but allow for the (one?) exception to mark itself as non compliant? We probably don't want any other codecs making Object, and this is a way to say that. In fact, it raises the other question, for a different discussion: is having the pickle protocol reasonable?

[d-v-b]

Shouldn't it annotate our expectations, but allow for the (one?) exception to mark itself as non compliant? We probably don't want any other codecs making Object, and this is a way to say that.

Yes, which is why in this PR I am annotating the encode and decode methods as returning Buffer | NDBuffer.

In fact, it raises the other question, for a different discussion: is having the pickle protocol reasonable?

It might be reasonable for some applications (it was evidently reasonable enough to get implemented in the first place). It's also incredibly dangerous, because it performs arbitrary code execution.

[d-v-b]

To make sure I understand, does passing an existing numcodecs codec to compressors or filters fail type checking in user code with this PR (because they don't conform to the new protocol)?

this is a great question, and the answer depends on which type checker you ask.

if we take this script:

# /// script
# requires-python = ">=3.11"
# dependencies = [
#   "zarr"
# ]
# ///

import zarr

from numcodecs import GZip
import numcodecs
from zarr.abc.numcodec import Numcodec

a: Numcodec = GZip()
b: numcodecs.abc.Codec = GZip()

mypy reports no problems:

bennettd@dvb-desktop-0 ➜  zarr-python git:(feat/numcodecs-protocol) ✗ uvx mypy test_3.py
Success: no issues found in 1 source file

but the more thorough based-pyright type checker complains about both a and b, for different reasons:

Installed 2 packages in 126ms
/home/bennettd/dev/zarr-python/test_3.py
  /home/bennettd/dev/zarr-python/test_3.py:8:8 - warning: Import "zarr" is not accessed (reportUnusedImport)
  /home/bennettd/dev/zarr-python/test_3.py:10:6 - warning: Stub file not found for "numcodecs" (reportMissingTypeStubs)
  /home/bennettd/dev/zarr-python/test_3.py:11:8 - warning: Stub file not found for "numcodecs" (reportMissingTypeStubs)
  /home/bennettd/dev/zarr-python/test_3.py:14:15 - error: Type "GZip" is not assignable to declared type "Numcodec"
    "GZip" is incompatible with protocol "Numcodec"
      "encode" is an incompatible type
        Type "(buf: Unknown) -> bytes" is not assignable to type "(buf: Buffer | NDBuffer) -> (Buffer | NDBuffer)"
          Function return type "bytes" is incompatible with type "Buffer | NDBuffer"
            Type "bytes" is not assignable to type "Buffer | NDBuffer"
      "decode" is an incompatible type
        Type "(buf: Unknown, out: Unknown | None = None) -> (Unknown | bytes)" is not assignable to type "(buf: Buffer | NDBuffer, out: Buffer | NDBuffer | None = None) -> (Buffer | NDBuffer)"
          Function return type "Unknown | bytes" is incompatible with type "Buffer | NDBuffer"
    ... (reportAssignmentType)
  /home/bennettd/dev/zarr-python/test_3.py:15:4 - warning: Type of "abc" is unknown (reportUnknownMemberType)
  /home/bennettd/dev/zarr-python/test_3.py:15:4 - warning: Type of "Codec" is unknown (reportUnknownMemberType)
  /home/bennettd/dev/zarr-python/test_3.py:15:14 - error: "abc" is not a known attribute of module "numcodecs" (reportAttributeAccessIssue)
2 errors, 5 warnings, 0 notes

so, tldr is that the status quo is broken because numcodecs doesn't support type checking, AND the new protocol is using a too-narrow type. i'll look into fixing this, but it's important to keep in mind that the status quo is broken.

edit: changing the numcodecs annotation from numcodecs.abc.Codec to Codec, after importing Codec from numcodecs.abc does pass pyright. but we never use that import in our codebase.

[normanrz]

Could this work with https://docs.python.org/3/library/typing.html#typing.runtime_checkable?

[d-v-b]

I tried :( but I learned that @runtime_checkable doesn't work for protocols with non-methods (i.e., attributes), and the numcodecs.abc.Codec ABC uses codec_id as an attribute. I'm not 100% sure we need the codec_id here, but if we ever wanted to register these codecs, then it would be important.

[d-v-b]

important context that I forgot when I answered this question: given the structure of Numcodec, runtime_checkable would work for isinstance checks, but not for issubclass checks, and I think there are places where we need the latter. But we should revisit this later if it turns out that we don't need issubclass, or if we want to use isinstance and rely on a custom function for issubclass.