test[next]: adopt class-style dimension declarations and add pyright coverage - #2845
test[next]: adopt class-style dimension declarations and add pyright coverage#2845egparedes wants to merge 1 commit into
Conversation
|
Review notes (kept out of the description so they don't land in the squashed commit) Part of a three-PR stack: #2843 → #2844 → #2845. Base is CI does not run on this PR. Every workflow in Verified locally with the sessions CI runs, on the current rebased tip: |
d3af347 to
33fcf97
Compare
33fcf97 to
b6a2192
Compare
b6a2192 to
5b6224e
Compare
5b6224e to
5ef083b
Compare
5ef083b to
8a728a8
Compare
8a728a8 to
d85aa9b
Compare
d85aa9b to
f603ffb
Compare
f603ffb to
a232cec
Compare
a232cec to
2901a53
Compare
…ndices A concrete dimension is now declared as a class, and an index along it is an instance of that class -- the shape `enum.Enum` uses, where the class is the collection and the instances are its members: ```python class IDim(gtx.DimensionIndex): ... class KDim(gtx.DimensionIndex, kind=gtx.DimensionKind.VERTICAL): ... IDim # the dimension -- annotated `gtx.Dimension` IDim(0) # an index into it -- annotated `IDim` ``` so `gtx.Field[gtx.Dims[IDim], gtx.float64]` is a valid annotation for any PEP 484 type checker, with no gt4py mypy plugin. `DimensionMeta` carries the API that belongs to the dimension itself (`I + 1`, `I > 5`, `I == 5`, `repr`, equality, hashing); binary operators on a class object dispatch through the metaclass, so that is the only place they can live. This drops the dimension half of `mypy_plugin.py`, which substituted at most four distinct placeholders per run (`_DimA`..`_DimD`, then `_AnyDim` for everything after), made `TypeVar`s over dimensions impossible, and served only mypy. Because `IDim(0)` is now ordinary instantiation, the whole apparatus that a foreign return type required is gone: no `__new__` returning a non-instance, no duplicated overloads on both `DimensionMeta.__call__` and `__new__`, and no `# type: ignore[misc]` anywhere on the instantiation path. mypy and pyright agree natively -- pinned by the typing tests in #2845. Indices also carry their dimension in the type, so mixing them is a static error; `common.NamedIndex` is deleted. Naming: the dimension's name is `.tag` (typed `common.Tag`, which already existed), and `.value` keeps its meaning as the index position. The reverse split does not type-check at all -- an instance attribute cannot shadow a `ClassVar` -- and this direction leaves every index expression, downstream included, untouched. `.dim` survives as a property returning `type(self)`. `common.Dimension` is a plain `TypeAlias` for `type[DimensionIndex]`, re-exported as `gtx.Dimension`, and `common.dimension(tag, kind)` is the programmatic constructor for the IR boundaries that rebuild a dimension from its tag. A `TYPE_CHECKING`-split callable shim was tried first and rejected: `eve.datamodels` resolves annotations at run time, so a `Dimension` field would see the shim rather than a type. A PEP 695 alias was also rejected: `get_origin()` of one is `None` rather than `type`, which silently misroutes the `get_origin(t) is type` dispatch in `ffront.fbuiltins` (see #2841). Reading `.value` on a dimension *class* would otherwise return the `__slots__` member descriptor rather than raising, and the nonsense value only surfaces much later as a missing offset-provider key or an `AxisLiteral` validation failure. A metaclass property makes it a loud `AttributeError` pointing at `.tag`; instance access is unaffected, since a metaclass attribute is not on an instance's lookup path. Also: pickling is registered through `copyreg` because `pickle.Pickler.save` routes anything whose type subclasses `type` to `save_global` before consulting `__reduce_ex__`, and `fingerprinting.py` gets a `DimensionMeta` deconstructor keyed on `(tag, kind)` so a dimension is not fingerprinted by qualified name. Behaviour change: `repr()` of a dimension is now `I[horizontal]`; `str()` is unchanged, so error messages are byte-identical. Design record: ADR 0028, added here. Implements the `shared/dimensions-as-types` proposal (gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503. Deliberately not here: a `DimensionBase` root above the user-declarable class, deferred until the requirements of non-user-declarable dimensions such as `Staggered[D]` are known; ICON4Py migration, which needs a note for `.tag` and for the removal of `NamedIndex`.
2901a53 to
0bfc8d5
Compare
…ndices A concrete dimension is now declared as a class, and an index along it is an instance of that class -- the shape `enum.Enum` uses, where the class is the collection and the instances are its members: ```python class IDim(gtx.DimensionIndex): ... class KDim(gtx.DimensionIndex, kind=gtx.DimensionKind.VERTICAL): ... IDim # the dimension -- annotated `gtx.Dimension` IDim(0) # an index into it -- annotated `IDim` ``` so `gtx.Field[gtx.Dims[IDim], gtx.float64]` is a valid annotation for any PEP 484 type checker, with no gt4py mypy plugin. `DimensionMeta` carries the API that belongs to the dimension itself (`I + 1`, `I > 5`, `I == 5`, `repr`, equality, hashing); binary operators on a class object dispatch through the metaclass, so that is the only place they can live. This drops the dimension half of `mypy_plugin.py`, which substituted at most four distinct placeholders per run (`_DimA`..`_DimD`, then `_AnyDim` for everything after), made `TypeVar`s over dimensions impossible, and served only mypy. Because `IDim(0)` is now ordinary instantiation, the whole apparatus that a foreign return type required is gone: no `__new__` returning a non-instance, no duplicated overloads on both `DimensionMeta.__call__` and `__new__`, and no `# type: ignore[misc]` anywhere on the instantiation path. mypy and pyright agree natively -- pinned by the typing tests in #2845. Indices also carry their dimension in the type, so mixing them is a static error; `common.NamedIndex` is deleted. Naming: the dimension's name is `.tag` (typed `common.Tag`, which already existed), and `.value` keeps its meaning as the index position. The reverse split does not type-check at all -- an instance attribute cannot shadow a `ClassVar` -- and this direction leaves every index expression, downstream included, untouched. `.dim` survives as a property returning `type(self)`. `common.Dimension` is a PEP 695 alias for `type[DimensionIndex]`, re-exported as `gtx.Dimension`, and `common.dimension(tag, kind)` is the programmatic constructor for the IR boundaries that rebuild a dimension from its tag. The PEP 695 spelling is what makes the deprecated `gtx.Dimension("I")` raise rather than silently misbehave. A plain `TypeAlias` for `type[X]` is a `types.GenericAlias`, and calling one forwards to its `__origin__` while discarding the arguments -- so `Dimension("I")` would evaluate to `type("I")`, i.e. `str`, with no error. Its cost is that `get_origin()` of such an alias is `None`, so a site dispatching on an annotation's shape must resolve it first; exactly one in-tree site needed that, `ffront.fbuiltins._type_conversion_helper`, via the `xtyping.resolve_annotation` helper added in #2841. A `TYPE_CHECKING`-split callable shim was also tried and rejected: `eve.datamodels` resolves annotations at run time, so a `Dimension` field would see the shim rather than a type. Reading `.value` on a dimension *class* would otherwise return the `__slots__` member descriptor rather than raising, and the nonsense value only surfaces much later as a missing offset-provider key or an `AxisLiteral` validation failure. A metaclass property makes it a loud `AttributeError` pointing at `.tag`; instance access is unaffected, since a metaclass attribute is not on an instance's lookup path. Also: pickling is registered through `copyreg` because `pickle.Pickler.save` routes anything whose type subclasses `type` to `save_global` before consulting `__reduce_ex__`, and `fingerprinting.py` gets a `DimensionMeta` deconstructor keyed on `(tag, kind)` so a dimension is not fingerprinted by qualified name. Behaviour change: `repr()` of a dimension is now `I[horizontal]`; `str()` is unchanged, so error messages are byte-identical. Design record: ADR 0028, added here. Implements the `shared/dimensions-as-types` proposal (gt4py_knowledge#27, @havogt) and closes the static-typing gap reported in #2503. Deliberately not here: a `DimensionBase` root above the user-declarable class, deferred until the requirements of non-user-declarable dimensions such as `Staggered[D]` are known; ICON4Py migration, which needs a note for `.tag` and for the removal of `NamedIndex`.
01b3dc6 to
c6ada98
Compare
c6ada98 to
617d152
Compare
Migrates every remaining factory-style declaration to a class -- 116 under `tests/`, plus the QuickstartGuide, the workshop notebooks, `helpers.py` and the cartesian-vs-next example -- and adds type-checker coverage for the new spelling. Where the Python name differs from the dimension's tag, the class keeps the name and pins `tag` explicitly, so the backend tag and every downstream reference are unchanged. Adds pyright to the `typing_exports` group and a `test_pyright.py` runner over `pyright_cases.py` asserting diagnostics line for line. pyright has no plugin mechanism, so it is the honest check that dimension annotations work unaided; nothing exercised pyright before. The new checker cases pin what this design buys: `IDim(0)` binds to `IDim` (not a shared index type) under both checkers with no suppressions, `IDim(0).value` is `int` while `IDim.tag` is the name, passing a `JDim` index where an `IDim` is required is an `arg-type` error, and `gtx.Dimension` annotates the class so an index is not assignable to it. Notebook code cells were migrated without touching stored outputs: those hold recorded tracebacks whose text must keep naming the symbols that produced them.
617d152 to
9682fbf
Compare
Follow-up to #2844, which introduced the class spelling but rewrote existing declarations only to
the minimal form that keeps them working (
I = gtx.dimension("I")). This adopts the class spellingacross the test suite -- 74 declarations, 22 of which pin
tagexplicitly -- and adds type-checkercoverage for it.
Where the Python name differs from the dimension's tag, the class keeps the name and pins
tag, sothe backend tag and every downstream reference are unchanged.
Adds pyright to the
typing_exportsgroup and atest_pyright.pyrunner overpyright_cases.pyasserting diagnostics line for line. pyright has no plugin mechanism, so it is the honest check that
dimension annotations work unaided; nothing exercised pyright before. The new checker cases pin what
this design buys:
IDim(0)binds toIDim-- not to a shared index type -- under both checkerswith no suppressions,
IDim(0).valueisintwhileIDim.tagis the name, passing aJDimindexwhere an
IDimis required is anarg-typeerror, andgtx.Dimensionannotates the class, so anindex is not assignable to it.
No
src/changes: a spelling migration plus new checker coverage.