personal/egparedes: connectivities as types - #32
Merged
Merged
Conversation
A neighbor connectivity is today spread over three user-authored objects that must agree by string equality (FieldOffset tag, local Dimension name, offset_provider key) plus the Python variable name the FieldOffset is bound to. Propose a connectivity class that contains its local dimension, is its own provider key, and whose identity is its qualified Python name (also the IR tag), building on shared/dimensions-as-types and gt4py#2844. Includes a research appendix with the full constraint catalogue and concept inventory derived from an audit of gt4py at b3c53fa7e (v1.2.2).
…ixes) No design changes. Fixes from an adversarial review of the proposal: - Keep the Cartesian FieldOffset form: as_offset() requires it (5 test modules, Ioff/Koff/EdgeOffset fixtures, ICON4Py diffusion/dycore). - Attribute the (tag, kind) fingerprint deconstructor to #2844/ADR 0028, not ADR 0023; ADR 0023 is a consequence, not a reversed decision. - Correct the __main__/spawn claim: file scripts resolve via __mp_main__; the limitation is interactive __main__ (REPL, notebooks, python -c). - "<locals>" check described as a heuristic; pickle's save_global is the real check. Note Staggered[D] tags need a grammar for resolve(). - "Cartesian already solved this" narrowed: no *separate* tag and no provider lookup; dimension names still cross gtfn/DaCe as strings. - Hedge the reduction claim in the main note to match what was run (embedded fails, roundtrip passes, gtfn by reading). - Add the roundtrip backend as a source-emitting consumer of dimension names; add the compat name-table caveat for string-keyed providers. - Present both sides of the conflict with shared/dimensions-as-types (46 "I" declarations, downstream == reliance, first-declaration-wins; and the typing subscription-cache aliasing that type identity removes). - Link mesh-and-first-class-halos and dimension-generic-fields; fix the dependent-local-dimensions citation (shared core is §6, U0/U1 are §9) and list the U0/U1 divergences. - Account for "seven of ten": A1-A5 dissolve, A6-A8 become one bind-time check. New open question on what an instance of V2E is. - Line-reference nits (common.py:62/1176/1177, fbuiltins.py:509, gtfn_module.py:118/132), _CONST_DIM has 12 use sites, concept-count row clarified, add type-checking tag (index keywords synced). Deferred to a second pass (design decisions): V2E.Local is not usable in annotations and Local[V2E] reconciliation is runtime-only (B1); the string-key shim needs a registry (B2); owner-less local dims such as ICON4Py's LsqUnkDim (B4); static-only max_neighbors vs generic meshes (S8).
Resolves the four items deferred from pass 1, with the typing probes that settle the first one added as typing_probe.py. - Local must be declared explicitly as a nested class. mypy --strict and pyright both accept `Field[V, V2E.Local]` for an explicit nested class and distinguish two connectivities' locals; both reject a ClassVar-typed generated Local as "not valid as a type". The generated form is dropped. - `Local[C]` and `C.Local` are distinct types for both checkers, so the __class_getitem__ reconciliation is removed; `C.Local` is the spelling and the cost to the chain proposals' static encodings is stated. - Local is not passed through the base subscription (pyright: "Class definition depends on itself"); the metaclass reads it after the body, or a PEP 696 default on 3.13 (new open question 2). - No deprecation window: string-keyed providers and FieldOffset removed outright with a scripted ICON4Py migration; no compat name table, so the "no registry" claim now holds without caveat. as_offset gets its dimension-based signature in the same release. - Owner-less local dimensions with a fixed `size` (ICON4Py's LsqUnkDim, RBFDimension); _CONST_DIM becomes the owner-less ConstList(size=1). - max_neighbors / min_neighbors optional on the class and completed at bind time (from the table, or from a NeighborConnectivityType in the AOT `connectivities=` path), because arity varies per mesh in fvm_nabla_setup.py and skip-value presence is configuration-dependent in ICON (icon.py:130, keep_skip_values). Declared counts remain a validated constraint.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical identity concerns and moderate design and validation issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a draft proposal for representing neighbor connectivities as nominal Python types, supported by research documentation and typing experiments.
Changes:
- Defines the connectivity-as-types model and migration plan.
- Documents current identity and naming constraints.
- Adds typing probes and an index entry.
File summaries
| File | Review summary |
|---|---|
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md |
Findings include one critical identity issue (2 votes), four moderate design/API issues (1 vote each), and two nits (1 vote each). |
content/personal/egparedes/connectivities-as-types/connectivities-as-types_research.md |
One nit regarding the scope of information carried by FieldOffset (1 vote). |
content/personal/egparedes/connectivities-as-types/typing_probe.py |
Two moderate validation issues and one naming inconsistency nit (1 vote each). |
content/index.md |
Index entry reviewed. |
Review details
Suppressed comments (10)
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:366
NeighborConnectivityis declared with only two type parameters above, but this paragraph cites a three-argument subscription and attributes pyright's diagnostic to it.typing_probe.pygets that diagnostic from the separateStaticMultiLevelMapping[V, "V_E2E.Local", E]stand-in, so it does not verify the proposedNeighborConnectivity[V, E]declaration; please either test the actual base or label this as a generic experiment.
- `Local` is **not** passed through the base subscription
(`NeighborConnectivity[V, "V2E.Local", E]`): mypy accepts that string
forward reference, pyright reports `Class definition for "V2E" depends on
itself`. The metaclass reads `cls.Local` after the class body has run; on
Python 3.13 a PEP 696 default type parameter is the typed alternative.
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:611
- Step 2 deletes
FieldOffsetoutright, but the dimension-based replacement foras_offsetis deferred to step 3 (and existing call sites are described as requiring the same release at lines 431–441). Landing these steps separately therefore breaks the Cartesian API and contradicts the claim that every step leaves the tree green; combine steps 2–3 or move the replacement before deletion.
2. **`NeighborConnectivity` + `LocalDimensionIndex`** (owned and owner-less)
in `common`; object-keyed provider; `FieldOffset` and string keys removed
outright; bind-time completion and validation of the counts.
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:202
- Using
LocalDimensionIndexhere widens the domain to any local axis, soV2EandE2Vhave the sameConnectivitydomain type at this boundary. That contradicts the proposal's claim that the concrete nestedV2E.Localis part of the static type; only manually spelling the nested class in a separate field annotation would distinguish them. The effective connectivity type needs to carry its concrete local type (or this protocol inheritance needs to be deferred until that can be expressed).
class NeighborConnectivity[Origin: DimensionIndex, Codomain: DimensionIndex](
Connectivity[MultiDimensionIndex[Origin, LocalDimensionIndex], Codomain],
metaclass=ConnectivityMeta,
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:597
- Q6 is a core API decision, not an implementation detail: the examples use the class object as a shift handle (
a(V2E)) and provider key, while this sketch also makesNeighborConnectivityaConnectivityimplementation. Until it is decided whether class objects or bound instances satisfy the runtime protocol, the proposed call syntax and provider typing cannot be implemented consistently, so stages 1–2 are not independently landable as claimed.
6. **What is an instance of `V2E`?** Today `Connectivity` is
`Field[DimsT, IntegralScalar]` parametrized by `Dims` plus a codomain
(`common.py:990`), and `CartesianConnectivity` is a dataclass with
instances. The sketch's `Connectivity[Src, Dst]` is a different
parametrization, and `V2E` is a data-less type constructor subclassing a
runtime protocol. Either instances of `V2E` are the bound tables (so
`{V2E: table}` becomes `V2E(table)`), or `NeighborConnectivity` is not a
`Connectivity` subclass at all and only *produces* one at bind time. The
sketch leaves this open and should not.
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:16
- The linked shared proposal does not choose qualified-name/type identity: it explicitly requires equality and hashing by
(value, kind)plus value-based serialization (content/shared/dimensions-as-types.md:99-112). Since this note later identifies that decision as a conflict, the TL;DR should describe qualified-name identity as this proposal's deliberate alternative rather than as behavior inherited from the shared note.
> own provider key, and whose identity — like every dimension after
> [[shared/dimensions-as-types|dimensions as types]] — is its qualified Python
> name, which is also its IR tag:
content/personal/egparedes/connectivities-as-types/connectivities-as-types.md:112
- This says
FieldOffsetcarries exactly the information inNeighborConnectivityType, but the latter also contains binding metadata such asdtype,skip_value, andmax_neighbors(listed in the appendix at lines 86–87). Please describe this as carrying only the domain/codomain dimensions, otherwise the duplication claim is factually too strong.
`FieldOffset(value, source=S, target=(T, L))` carries *exactly* the
information in `NeighborConnectivityType(domain=(T, L), codomain=S)` plus a
name — with inverted vocabulary (`FieldOffset.source` is the connectivity's
**codomain**; `target` is its **domain**) and no cross-check between the two.
content/personal/egparedes/connectivities-as-types/connectivities-as-types_research.md:242
- The appendix repeats the claim that
FieldOffsetcarries exactly theNeighborConnectivityTypeinformation, butNeighborConnectivityTypealso hasdtype,skip_value, andmax_neighbors. Narrow this description to the shared domain/codomain dimensions so the catalogue does not misstate the current type contents.
`FieldOffset` carries exactly the information in `NeighborConnectivityType` plus
a name, with **inverted vocabulary** and no cross-check. `FieldOffset.source` is
the connectivity's *codomain*; `FieldOffset.target` is its *domain*. The
inversion is because `source`/`target` describe the *field remap* (the field
lives on `source` and ends up on `target`), while `domain`/`codomain` describe
the *table*.
content/personal/egparedes/connectivities-as-types/typing_probe.py:56
- This explanatory example names
V_V2V.Local, but the probe declaresV_E2EandE_V2V; noV_V2Vclass exists in the file. That makes the forward-reference explanation inconsistent with the actual base subscriptions.
# cls isn't bound in its module yet, so forward refs like "V_V2V.Local"
content/personal/egparedes/connectivities-as-types/typing_probe.py:98
- This negative case changes both the primary dimension (
VtoE) and the local dimension, so a checker can reject it without proving that the two nestedLocaltypes are distinct. Use the other connectivity's local with the same primary dimension, and update the expected-line text in the module docstring accordingly, to isolate the claim this probe is meant to establish.
p1(Field[E, E_V2V.Local]()) # expected error: distinct locals
content/personal/egparedes/connectivities-as-types/typing_probe.py:6
- The checker behavior used to justify the
Localdecisions is only recorded as a manually run command, with negative cases described in comments rather than checked by a test harness. Similar typing prototypes in this repository run mypy from pytest and assert the exact expected error lines; without that here, checker upgrades or extra diagnostics can silently invalidate the proposal's conclusions.
Run with `mypy --strict --python-version 3.12` and `pyright --pythonversion 3.12`.
Expected: P1 accepts the explicit nested `Local` and rejects the cross-connectivity
mismatch on line `p1(Field[E, E_V2V.Local]())`; P2 rejects the generated `ClassVar` form
as "not valid as a type"; P3 rejects each spelling where the other is expected.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+243
to
+246
| A dimension's or connectivity's identity is the Python type; its tag is | ||
| `f"{cls.__module__}.{cls.__qualname__}"`. The static view (checkers see | ||
| nominal types) and the runtime view (equality is `is`) agree by | ||
| construction, and the tag is a valid, unique IR string. This is a deliberate |
This was referenced Sep 18, 2026
Closed
Draft
egparedes
added a commit
to GridTools/gt4py
that referenced
this pull request
Sep 23, 2026
Lowering emitted the **Python variable name** an offset was bound to as
the IR shift tag, because `ts.OffsetType` did not carry the tag.
Embedded execution keys on `FieldOffset.value`, so the same program
needed a *different* offset provider depending on how it was run —
confirmed by running it on v1.2.2:
```
MyOff = FieldOffset("TAGNAME", ...)
embedded: {"TAGNAME": conn} OK ; {"MyOff": conn} -> KeyError 'TAGNAME'
compiled: {"MyOff": conn} OK ; {"TAGNAME": conn} -> KeyError 'MyOff'
```
`ts.OffsetType` now carries `tag`, and lowering emits it.
Lowering also matches shift arguments on their **type** instead of their
node shape, so module-qualified offsets work. `a(mod.V2E)`,
`a(mod.E2V[0])` and `a(mod.Koff[1])` used to fail on every compiled
backend with `Unexpected shift arguments!` (a `foast.Attribute` is not a
`foast.Name`), while embedded ran them — the same embedded/compiled
divergence. Only tagged offsets are matched (`tag=str()`), and
`field(Off)` requires two targets, so an untagged `(Dim + 1)[0]` or a
bare Cartesian `a(Koff)` still gets the lowering error rather than an
assertion or a silently wrong `neighbors`.
This supersedes #2730 by @havogt, which carried the same fix under the
field name `name`; the type-driven matching and one of its tests are
taken from there (co-authored). `tag` is kept over `name` because it
matches `common.Tag` / `FieldOffset.value`, and `name` is easily
confused with the Python variable name — which is exactly what the bug
conflated.
### Why `tag` is `Optional`
A Cartesian shift written `Dim + offset` has no tag and needs none — it
lowers to an `itir.CartesianOffset` carrying both dimensions, with no
provider lookup. `type_deduction` builds an `OffsetType` for exactly
that case (`:711`), so a required field would break it. Subscripting
(`Off[1]`) drops the local dimension but *propagates* the tag, which is
the offset's identity.
### ⚠️ Behaviour change
For a declaration whose tag differs from the variable it is bound to,
compiled backends previously required `offset_provider={"MyOff": conn}`
and now require `{"TAGNAME": conn}`. That divergence from embedded
execution is the bug being fixed, but it is user-visible.
**ICON4Py is unaffected**: all 16 `FieldOffset` declarations have tag ==
variable name, there are no `Koff[...]` subscripts in model code, and
`as_offset` never consults the tag.
No `CHANGELOG.md` entry — that file is only ever touched by release PRs.
### Tests
The regression test grows from one cell (`a(Off[1])` on `GTFN_CPU`) to
`{shift, neighbor_sum} × {tag ≠ variable name, tag ≠ local dimension
name}` across the whole backend matrix, plus two lowering unit tests in
`test_foast_to_gtir.py` that assert the emitted `OffsetLiteral`
directly. Both unit tests were verified to **fail** with the fix
reverted.
For the type-driven matching: `test_import_from_mod.py` gains a
module-qualified `neighbor_sum(a(cases.V2E))` (from #2730) and
`a(cases.E2V[0])` across the backend matrix (20 compiled-backend
failures before the fix), and `test_foast_to_gtir.py` gains two
error-path tests (`(TDim + 1)[0]`, bare `inp(TOff)`), each verified to
fail with its guard removed.
Each `Case` holds exactly one connectivity on purpose: DaCe walks
*every* offset-provider entry while building the SDFG and looks a
connectivity up by its **local dimension's** name, so a second
non-conforming entry fails a program that never uses it.
The remaining failures are marked per backend, from measurement rather
than assumption:
| | roundtrip | roundtrip.gtir | gtfn | embedded | dace |
|---|---|---|---|---|---|
| shift, tag ≠ varname | ✅ | ✅ | ✅ | ✅ | ✅ |
| reduction, tag ≠ varname | ✅ | ✅ | ✅ | ✅ | ✅ |
| shift, tag ≠ local dim | ✅ | ✅ | ✅ | ✅ | xfail |
| reduction, tag ≠ local dim | ✅ | xfail | xfail | xfail | xfail |
Note `roundtrip` passes the reduction case while `roundtrip.gtir` does
not: `roundtrip.default` runs `apply_common_transforms`, so the
reduction unrolls keyed by the *offset tag*, whereas `roundtrip.gtir`
runs only the fieldview transforms and reaches `iterator/embedded.py`
keyed on the *local dimension*.
Both remaining constraints are one underlying issue: those paths resolve
a connectivity through the local dimension's name rather than the
offset's identity. Fixing it needs a back-pointer from the local
dimension to its connectivity, which is a later step in this stack.
GPU and JAX cells are marked by shared-code-path reasoning, not
measurement — they skip locally. `xfail_strict` is on, so if any is
wrong CI fails loudly rather than passing silently.
### Verification
At head (`d151516`): `pytest tests/next_tests -m "not uses_dace"` → 4495
passed / 0 failed; `pytest tests/next_tests/regression_tests -m
uses_dace` → 6 passed / 0 failed; `test_import_from_mod.py` on all CPU
backends incl. DaCe → 36 passed; `mypy src/`, `tach check`, `pre-commit
run` clean. The full `-m uses_dace` run (1477 passed / 0 failed) was
done on the first commit.
### Context
First PR of a stack implementing
[`egparedes/connectivities-as-types`](GridTools/gt4py_knowledge#32),
an alternative to #2844. **This PR stands alone** — it is a bugfix that
is correct regardless of whether the rest of the stack lands.
---------
Co-authored-by: Hannes Vogt <vogt@hey.com>
egparedes
added a commit
that referenced
this pull request
Sep 23, 2026
…ation (#33) Updates the *connectivities as types* note (#32) to the design as implemented in the GridTools/gt4py stack: GridTools/gt4py#2898 → #2899 → #2907 → #2908 → #2909 → #2910 → #2911 → #2912 (ADR 0028: dimensions as nominal types; ADR 0029: connectivities as types). ## What changed in the note - **TL;DR / status**: an implementation-status callout. - **Concepts and sketch** rewritten to the real code: - no `DimensionBaseIndex`, and `LocalDimensionIndex` subclasses `DimensionIndex` - `NeighborConnectivity` is not a `Connectivity` - `offset_tag` names a connectivity in the IR - a connectivity can adopt an existing local dimension, or share one (flattened sparse offsets like ICON4Py's `C2CE`) - `MultiDimensionIndex` is a tuple subclass - `as_offset(KDim, field)` - **Identity rules**: what `resolve` memoizes (so redefined declarations are found), the interactive-`__main__` fallback, the narrow `copyreg` hook for `Staggered[D]`, fingerprinting, the corrected injective mangling (the original scheme was not injective), and a new rule on `offset_tag`. - **Binding model**: providers are normalized to tags at the entry points, and tables are checked against declarations once per compiled variant, including the same-structure requirement for tables over one shared local dimension. - **Effect per layer / what it deletes**: updated. `ts.OffsetType`, `iterator.runtime.offset` and the string-keyed providers below the entry points are *kept*. - **Open questions**: each answered as implemented. Naming convergence with `dependent-local-dimensions` is still open. - **Staging** → **Implementation**: the PR table and a list of where the implementation departs from the original proposal. - The research appendix is marked as a historical record of the pre-implementation tree. `status` stays `draft`: this was written with AI assistance and needs a human review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New proposal under
personal/egparedes/, plus a research appendix and an index entry.What it proposes
A neighbor connectivity in
gt4py.nextis today spread over three user-authored objects that must agree by string equality — theFieldOffsettag, the localDimensionname and theoffset_providerkey — plus a hidden fourth: the Python variable theFieldOffsetis bound to. The proposal makes the connectivity a class that contains its local dimension, is its own provider key, and whose identity is its qualified Python name (which is also its IR tag):FieldOffset,ts.OffsetType, the string-keyedOffsetProviderand theV2EDim-next-to-V2Econvention go away. It is positioned as the single-hop shared core thatpersonal/havogt/dependent-local-dimensions§9 calls for, and takes no position on chains or reduction order.Evidence
Derived from a full audit of gt4py at
b3c53fa7e(v1.2.2). The appendix (connectivities-as-types_research.md) catalogues 10 cross-object identity constraints, 9 name-format constraints and 7 structural ones, each withfile:line, plus the complete concept inventory (~25 concepts across 10 layers) and diagrams. Two behaviours were confirmed by running them: embedded and compiled execution key the same program on different strings (FieldOffset.valuevs the Python variable name), and GridTools/gt4py#1789 lifted the tag-equals-local-dim requirement for the shift path only — reductions still fail with a mismatch.Conflicts, called out explicitly in the note
shared/dimensions-as-types.md/ feat[next]: make a concrete Dimension a class and its instances the indices gt4py#2844 on identity semantics: that proposal chose(name, kind)equality with an interning registry and value-based pickling; this one chooses type identity (qualified name), removing the registry andcopyreghook. If accepted, the shared note needs a revision._Staggeredprefix cannot survive type identity;Staggered[D]becomes required), ADR 0019 (theFieldOffset-as-identifier part is superseded).status: draft— AI-assisted, pending human review.