this is pretty substantial so I would appreciate a lot of eyes @zarr-developers/python-core-devs

if anyone has concerns about whether we should do any runtime type checking at all, maybe send those thoughts to theissue this PR closes

I'm going to keep working on tests for the type checker, but so far it's working great.

This PR does violating liskov for a few subclasses of ourMetadata ABC, because that class requires thatto_dict andfrom_dict usedict[st, JSON], which is not very accurate. After this PR we need to make changes to class so that it supports type safety, then we won't be violating liskov any more.

Similarly, there are lots of# type: ignores added in various places. As you might guess, those were added because without them, mypy flagging type errors. Many of these checks will go away when we fix the lax typing of theMetadata ABC, and other classes.

@TomAugspurger I think you in particular will appreciate some of the effects of this PR. Since we can annotate methods likeArrayV3Metadata.from_dict() as takingArrayMetadataJSON_V3, we don't need to do any runtime validation insidefrom_dict. The assumption is that the caller has already checked the input. This is not possible without the types defined in this PR, and it's notpractical without the type checker. Once we extrapolate the type safe style, we can push almost all our type checking to the IO boundary.

That being said, I think the ArrayMetadata class will still need to do some internal consistency checks, like ensuring that the number of dimension names matches the length ofshape. I don't think we want our type checker to be smart enough to catch that kind of thing.

Codecov Report

❌ Patch coverage is98.01980% with8 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.90%. Comparing base (7d43f32) to head (c7096b1).

@@            Coverage Diff             @@##             main    #3400      +/-   ##==========================================- Coverage   94.92%   94.90%   -0.03%==========================================  Files          79       80       +1       Lines        9503     9752     +249     ==========================================+ Hits         9021     9255     +234- Misses        482      497      +15

In the dev meeting today@jhamman rightly challenged me to justify writing our own in-house runtime type checker instead of bringing in pydantic. I had a few answers:

• The type checker here is small (~500 LoC, with docstrings) and it has a very simple structure -- a bunch of functions. Pydantic is huge and complex. It relies on dynamic class creation, and bugs in in pydantic can be hard to debug.
• pydantic is class-centric. I think afunctional API for type checking is a better fit for our codebase. Replacing all our classes withBaseModel is relatively disruptive comparing to bringing in a few functions to help with a type guard.
• Pydantic contains a lot of stuff we don't need -- For zarr, we only need to check the types that could fit inzarr.json, but pydantic covers alot more. The scale of pydantic is also an upside -- it means that new features will get added, and bugs will be squashed, by a large number of developers. But as long as we test our in-house type checker extensively against the small set of types that matter to us, I don't think we would gain much from adding pydantic as a dependency.
• Pydantic makes breaking changes. If we depend on pydantic, we then become responsible for managing how those breaking changes impact our users. This is a cost that I think we can avoid.
• The changes here are very non-invasive (because it's just functions). We can always switch to pydantic later, if this type checker sucks. We could also revert and replace all instances of this type checker with a hand-written type checking function.

My calculation was, 500 LoC for an API that does exactly what we need, (and nothing else), was a better deal than bringing in a big, celebrity dependency that would require pretty substantial changes to basic Zarr Python classes. Other folks might see this calculation differently.

as illustration, consider this diff enabled by this PR:

@@ -286,14 +283,7 @@ class NullTerminatedBytes(ZDType[np.dtypes.BytesDType[int], np.bytes_], HasLengt             True if the input is a valid representation of this class in Zarr V3, False             otherwise.         """-        return (-            isinstance(data, dict)-            and set(data.keys()) == {"name", "configuration"}-            and data["name"] == cls._zarr_v3_name-            and isinstance(data["configuration"], dict)-            and "length_bytes" in data["configuration"]-            and isinstance(data["configuration"]["length_bytes"], int)-        )+        return check_type(data, NullTerminatedBytesJSON_V3).success

I worry that if type safety requires writing tedious, error-prone functions like what we have in main, we might not do it, and then we lose type safety. Boiling it down to a much simpler function makes type safety look a lot easier.