Migrate to the class-based Dimension API (GridTools/gt4py#2844) - #1455
Migrate to the class-based Dimension API (GridTools/gt4py#2844)#1455havogt wants to merge 10 commits into
Conversation
Temporary source override so icon4py can be built and checked against the unmerged PR. Pinned to the frozen rev 194608fc rather than `branch = "dimensions-as-types-2-core"`: a branch source re-resolves on every lock, so the tree can go red with no diff of ours changing when the PR head moves. A frozen rev cannot. The ten `gt4py==1.2.1` manifest pins are deliberately untouched: a [tool.uv.sources] override replaces the source and uv does not enforce version specifiers against it. Resolved here as 1.2.1.post27+194608fc with no manifest edits. uv.lock is reformatted wholesale by the local uv; the only resolution change is gt4py 1.2.1 -> 1.2.1.post27+194608fc (282 packages before and after). Revert this commit once #2844 has merged and a release carries it.
Under gt4py#2844 a concrete dimension is a subclass of `gtx.Dimension`, not an instance of it. Every Python name is kept and `value` pinned explicitly, so the backend tag and all ~190 downstream references are unchanged: `CellDim.value` is still "Cell". Verified against a capture taken from the factory form before the rewrite: all 20 dimensions (value, kind) and all 16 FieldOffsets (value, source, target) are identical. The three remaining `gtx.Dimension(...)` calls build a dimension from a runtime string or a parametrized kind and cannot be class statements; they keep working through the compatibility factory, which interns to the same class.
A dimension is now a subclass of `gtx.Dimension`, so `isinstance(d, gtx.Dimension)` is False for every dimension and these five predicates silently selected nothing. Measured on the unmigrated tree against gt4py#2844: `horizontal_dims()`, `vertical_dims()` and `local_dims()` all returned empty iterators. That is a wrong answer, not an error - `mpi_decomposition` builds its GHEX domain descriptors and exchange patterns by iterating `horizontal_dims()`, and roughly 30 test parametrizations would have collected zero cases and reported green. `DimensionMeta` is not re-exported from `gt4py.next`, so it is reached through `gt4py.next.common`, which icon4py already imports as `gtx_common` in 15 modules. After the fix the three helpers yield 3 horizontal, 2 vertical and 15 local dimensions, matching the counts derived from the pre-migration capture.
A dimension is now a class, so a parameter that receives one is annotated `type[gtx.Dimension]`, not `gtx.Dimension`. 196 occurrences across 40 files; the 21 class-statement base classes and the 3 remaining compatibility-factory calls are deliberately untouched. Four dict literals additionally needed an explicit annotation. `dict` is invariant, and a dimension-keyed literal no longer infers a usable key type: mypy gives `dict[type[CellDim], ...]` for a single key, and joins keys of different dimensions to their shared metaclass, `dict[DimensionMeta, ...]`. Neither is compatible with the declared `dict[type[gtx.Dimension], ...]`. This drops mypy from 2142 errors to 97, all of which are a single gt4py-side defect (see the next commit's evidence file), none of them icon4py's.
Two changes that together make the tree type-check cleanly against gt4py#2844; mypy now reports "Success: no issues found in 444 source files". mypy 2.1.0 does not type `IDim(0)` as `NamedIndex`: it rewrites the overloaded `DimensionBase.__new__` return type to the class, so a sparse access such as `ikoffset[dims.E2CDim(0)]` is rejected, 95 times across 13 modules, and a dimension built through the compatibility factory is typed as an instance rather than `type[Dimension]`. 2.2.0 is the first release that gets both right (bisected 2.1.0 -> 2.2.0 on a reduced case); the full suite is clean on 2.2.0 and on 2.3.1. The previous floor of 1.13.0 admitted versions that cannot check this code base, so it is raised rather than relying on the lock alone. The remaining 20 errors were ours. An unannotated dimension-keyed dict literal no longer infers a usable key type: mypy joins keys of different dimensions to the overloaded constructor type, which it renders as "type[Dimension]" - so the diagnostic reads "Invalid index type type[Dimension] ...; expected type type[Dimension]", incompatible with an identically printed type. Indexing such a dict with a `type[gtx.Dimension]` then fails. The six `_ICON_*` tables in grid/horizontal.py follow `_ICON_LATERAL_BOUNDARY`, which already carried the annotation and was the one table in that file that did not error. Only dicts that are stored and later indexed are affected; the many stencil `domain` literals are passed straight to a call and are left alone.
Was 194608fc8, which #2844 has since rebased away twice; that commit is no longer on the PR's history (`compare 194608fc8...fefb8e213` reports diverged). GitHub still serves it by SHA, so the old pin resolved - it just held a line of development the PR no longer contained. fefb8e213 is a redesign rather than a newer revision of the same one, so the commits that follow rewrite the migration rather than adjust it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt2tWSiNGcdxC9QTeXqT8b
The design changed under the previous migration: `gtx.Dimension` is no longer a class but a `TypeAlias` for `type[DimensionIndex]`, `common.NamedIndex` is gone, and a dimension's name moved from `.value` to `.tag` - `.value` now means an index position along the dimension, so `IDim(0)` is ordinary instantiation returning an instance of `IDim`. Mechanically: 21 class X(gtx.Dimension, ...) -> class X(gtx.DimensionIndex, ...) 20 value = "Cell" -> tag = "Cell" 217 type[gtx.Dimension] -> gtx.Dimension (the alias is already type[...]) 3 gtx.Dimension(<str>) -> gtx.dimension(<str>) The `isinstance(..., gtx_common.DimensionMeta)` predicates are unchanged and still correct; `DimensionMeta` survives the redesign and is still not exported from `gt4py.next`. The 16 dict annotations and the `mypy>=2.2.0` floor from the previous migration are both left in place but are no longer load-bearing: the overloaded `__new__` that caused the version split and the self-contradictory dict-join diagnostic is gone, so mypy reports no issues across 444 files after the mechanical pass alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt2tWSiNGcdxC9QTeXqT8b
#2844 splits what was one attribute: a dimension's *name* is `.tag`, while `.value` now means an index position along it, so `.value` on a dimension class raises. Twelve reads across six modules, six of them in the MPI paths of `mpi_decomposition`. mypy does not catch any of this. `DimensionIndex` declares `value: int` as an instance attribute, so reading it on the class object type-checks; only the metaclass property catches it, at run time, once per executed path. The full gate was green with all twelve wrong, and the test suite found them. `decomposition/halo.py` keeps `.value`: its `relevant_dimension` list holds `FieldOffset`s, not dimensions, and `FieldOffset` still uses `.value`. The two now diverge, so code holding either has to know which it has - a distinction the old API did not force and that this list's name gets wrong. The `mypy>=2.2.0` floor added earlier is reverted: it existed only to escape a 2.1.0 bug with the previous design's overloaded `__new__`, which is gone. Measured on this tree - mypy 2.1.0 and 2.3.1 both report no issues across 444 files. The lock keeps 2.3.1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt2tWSiNGcdxC9QTeXqT8b
`ab5e8d5a0` was rebased away (diverged, behind_by 1). The API this branch depends on is unchanged at the new head - `DimensionIndex` is still the declarable base, the `.tag` name / `.value` index split is intact, `dimension(tag, kind)` is still the programmatic factory, and `DimensionMeta` is still absent from `gt4py.next`'s exports. So the migration itself needs no edit: mypy reports no issues across 444 source files and the suite is 602 passed / 129 skipped / 3 xfailed, with zero source changes on top of the previous head. The pin is not the only thing that moves, though. gt4py's own manifest went from `dace>=2.0.0a6` to `dace>=2.0.0a7,<2.0.0a8`, so this drags dace 2.0.0a6 -> 2.0.0a7 and pulls in `distro` and `scikit-build` as new transitive dependencies (282 -> 284 packages). That is forced by gt4py, not chosen here, but it means this branch now requires the same dace revision that #1429 is separately validating. gt4py's base version also moved to 1.2.2, resolved as 1.2.2.post1+26016ac9. The ten `gt4py==1.2.1` manifest pins are still untouched and still overridden. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xt2tWSiNGcdxC9QTeXqT8b
main took four merges since this branch was cut, two of which collide with it.
Conflicts, and how they were resolved:
model/common/src/icon4py/model/common/dimension.py
#1429 replaced the standalone `KHalfDim` declaration with
`KHalfDim = gtx.flip_staggered(KDim)`. Taken as-is: it is a merged
decision and this branch has no business reverting it. Note it changes
the dimension's tag from "KHalf" to "_StaggeredK" - main's call, not
ours. The remaining 19 declarations keep this branch's class form; the
factory list main still carries is what #2844 removes.
#1429 also dropped `KHalfOff`, which merged cleanly.
uv.lock
Regenerated from main's rather than hand-merged: `git checkout --theirs`
then `uv lock`, so the resolution is uv's own and not a hand-stitched
file.
`pyproject.toml` merged cleanly and keeps the `[tool.uv.sources]` git override
for #2844. main's manifest pins moved to `gt4py==1.2.2` with #1451 and are, as
before, ignored by the override.
The earlier concern that this branch drags dace 2.0.0a6 -> a7 is void: #1451
already put dace 2.0.0a7 on main, so the branch aligns with main rather than
moving ahead of it.
The merge reintroduced no old-API usage: zero `class X(gtx.Dimension, ...)`,
zero `type[gtx.Dimension]`, zero `gtx.Dimension(<str>)` factory calls, zero
`isinstance(..., gtx.Dimension)`.
Gates after the merge: mypy reports no issues across 435 source files - down
from 444 because #1429 deleted ten stencil modules that staggered dimensions
made redundant, not because anything stopped being checked - and the suite is
602 passed / 129 skipped / 3 xfailed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xt2tWSiNGcdxC9QTeXqT8b
|
When developing, you can test your changes on CSCS CI before merge with the You can pass options to override pipeline variables, for example:
Available options are:
For each option, See The Merging Once your PR is approved and ready for merging, add it to the merge queue. The Optional Tests To run benchmarks you can use:
For more detailed information please look at CI in the EXCLAIM universe. |
Not for merge. This branch pins an unmerged, still-rebasing gt4py PR. It exists to answer
"does GridTools/gt4py#2844 require changes in icon4py, and is the new design good?" and to feed
that answer back to #2844's author.
What #2844 does
A concrete dimension becomes a class and an index along it an instance of that class — the shape
enum.Enumuses.gtx.Dimensionis no longer a class but an alias fortype[DimensionIndex],common.NamedIndexis deleted, and a dimension's name moves from.valueto.tag(.valuenow means an index position).
Does icon4py need changes? Yes
Without them the tree does not import:
class KDim(gtx.Dimension, kind=...)fails, becausegtx.Dimensionis an alias.class X(gtx.Dimension, …)→gtx.DimensionIndexvalue = "Cell"→tag = "Cell"type[gtx.Dimension]→gtx.Dimension(the alias is alreadytype[…])gtx.Dimension(<str>)→gtx.dimension(<str>)dim.value→dim.tagisinstance(…, DimensionMeta)predicates are unchanged and still correct.Verification
mypy— no issues across 444 source files, on both 2.1.0 and 2.3.1.pytest -n0 --datatest-skipovermodel/common/tests/common/{grid,states,decomposition,interpolation},model/driver/tests/driver/unit_tests,bindings/tests,tools/tests, and two dycore stenciltests covering sparse
E2Caccess and ascan_operator— 602 passed, 129 skipped, 3 xfailed.Not run: datatests, MPI tests, GPU/dace/gtfn backends, the full stencil suite.
Feedback for #2844
The redesign is a clear improvement. It dissolves rather than patches the two worst problems
of the previous revision: an overloaded
__new__returning a foreign type, which made thecodebase check differently on mypy 2.1.0 vs 2.2.0 and produced a diagnostic naming a type as
incompatible with itself. Both are gone because
IDim(0)is ordinary instantiation. Themigration is mechanical and the checker confirms it in one pass.
Four things worth the author's attention, in order:
.value→.tagis invisible to the type checker.DimensionIndexdeclaresvalue: intas an instance attribute, so reading it on the class type-checks fine; onlythe metaclass property catches it, at run time, once per executed path. The full mypy gate
here was green with all twelve reads still wrong — the test suite found them, and six were in
MPI paths that a non-MPI run would never have reached. The
AttributeErroritself isexcellent (it names
.tagand the class/instance distinction); the problem is that nothingfinds the call sites for you.
FieldOffset.valueandDimension.tagnow diverge, where both used to be.value. Codeholding either must now know which it has. In this repo a list literally named
relevant_dimensionholdsFieldOffsets — the split turned a latent naming bug into acrash.
ab5e8d5a0.class KDim(gtx.Dimension, kind=…)— the single most likely downstream edit — was
TypeError: KDim.__init_subclass__() takes no keyword argumentsand is nowTypeError: typealias() takes at most 3 arguments (4 given),which names a callable that appears nowhere in user code and no longer names the class.
Neither points at
DimensionIndex.DimensionMetais still not exported fromgt4py.next.isinstance(x, gtx.Dimension)raises, so
DimensionMetais the only way to ask "is this a dimension", and it is reachableonly through
gt4py.next.common. One__all__entry.Already fixed upstream after earlier rounds of this feedback:
Dimension("X")used to return<class 'str'>silently (a plain alias forwards a call to__origin__); the PEP 695 form atab5e8d5a0makes it aTypeError.Before this can merge
[tool.uv.sources]gt4py pin (first commit) once #2844 lands in a release.fefb8e213; #2844 has rebased threetimes in three days and is currently
DIRTY, so the head has moved on.🤖 Generated with Claude Code