refactor[next]: merge OTF definitions into stages, extract artifacts, enforce the layering - #2737
refactor[next]: merge OTF definitions into stages, extract artifacts, enforce the layering#2737egparedes wants to merge 1 commit into
Conversation
4f33611 to
fa8a324
Compare
fa8a324 to
6c96ef4
Compare
6c96ef4 to
4215849
Compare
4215849 to
5fd4ac0
Compare
There was a problem hiding this comment.
Pull request overview
This PR continues the gt4py.next.otf refactor by reorganizing module responsibilities: DSL-aware stage/type vocabulary is consolidated into otf.stages, DSL-agnostic artifact models and code-specs are extracted into a new otf.artifacts module, and the ConcreteArtifact carrier type is moved into the DSL-neutral otf.workflow. It updates runners, compilation/build-system code, tests, and docs to use the new import locations, and removes the old modules without compatibility re-exports.
Changes:
- Merged
otf.definitionscontracts/type aliases intootf.stagesand deletedotf/definitions.py. - Introduced
otf.artifacts(migratingcode_specs+ artifact data models +format_source) and deletedotf/code_specs.py. - Moved
ConcreteArtifacttootf.workflowand updated toolchain/runners/tests/docs accordingly.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/test_dace_translation.py | Update ConcreteArtifact import to otf.workflow. |
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/test_dace_compilation.py | Switch OTF source models/code specs to otf.artifacts. |
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/test_dace_cache_consistency.py | Switch OTF source models/code specs to otf.artifacts. |
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/test_dace_bindings.py | Switch OTF source model generics/code specs to otf.artifacts. |
| tests/next_tests/unit_tests/program_processor_tests/runners_tests/dace_tests/test_dace_backend.py | Move CompilableProgramDef reference from definitions to stages. |
| tests/next_tests/unit_tests/program_processor_tests/codegens_tests/gtfn_tests/test_gtfn_module.py | Update CompilableProgramDef/code spec imports to stages+artifacts. |
| tests/next_tests/unit_tests/otf_tests/test_languages.py | Update code spec + ProgramSource imports to otf.artifacts. |
| tests/next_tests/unit_tests/otf_tests/test_compiled_program.py | Update ConcreteArtifact import to otf.workflow. |
| tests/next_tests/unit_tests/otf_tests/compilation_tests/build_systems_tests/test_cache_consistency.py | Update build-cache test types to otf.artifacts. |
| tests/next_tests/unit_tests/otf_tests/compilation_tests/build_systems_tests/conftest.py | Update test fixtures to construct otf.artifacts sources/specs. |
| src/gt4py/next/program_processors/runners/roundtrip.py | Update workflow typing to use stages.CompilableProgramDef and artifact types. |
| src/gt4py/next/program_processors/runners/gtfn.py | Switch executable/source types to otf.artifacts; inline strict fingerprinter. |
| src/gt4py/next/program_processors/runners/dace/workflow/translation.py | Update translation step typing + ProgramSource/code spec usage to artifacts. |
| src/gt4py/next/program_processors/runners/dace/workflow/factory.py | Inline strict fingerprinter and remove dependency on stages fingerprinter alias. |
| src/gt4py/next/program_processors/runners/dace/workflow/compilation.py | Switch extension source/compiled program types to otf.artifacts; drop nominal protocol bases. |
| src/gt4py/next/program_processors/runners/dace/workflow/bindings.py | Switch binding/extension source models to otf.artifacts. |
| src/gt4py/next/program_processors/runners/dace/workflow/backend.py | Switch CompilationArtifact/ExecutableProgram types to otf.artifacts. |
| src/gt4py/next/program_processors/runners/dace/program.py | Update ConcreteArtifact usage to otf.workflow. |
| src/gt4py/next/program_processors/codegens/gtfn/gtfn_module.py | Switch code specs + ProgramSource + format_source to otf.artifacts; use stages.TranslationStep. |
| src/gt4py/next/otf/workflow.py | Add ConcreteArtifact carrier type here (DSL-neutral location). |
| src/gt4py/next/otf/toolchain.py | Remove local ConcreteArtifact; update adapters to reference workflow.ConcreteArtifact. |
| src/gt4py/next/otf/stages.py | Redefine as the DSL-aware vocabulary + step contracts; remove artifact model classes. |
| src/gt4py/next/otf/runners.py | Switch runner contracts to otf.artifacts.CompilationArtifact. |
| src/gt4py/next/otf/recipes.py | Update recipe workflow generics to stages + artifacts. |
| src/gt4py/next/otf/definitions.py | Deleted (merged into otf.stages). |
| src/gt4py/next/otf/compiled_program.py | Switch compiled-program pools/futures to otf.artifacts types. |
| src/gt4py/next/otf/compilation/compiler.py | Switch build pipeline types/specs to otf.artifacts; drop nominal protocol base. |
| src/gt4py/next/otf/compilation/cache.py | Change cache folder API to accept artifacts.ExtensionSource. |
| src/gt4py/next/otf/compilation/build_systems/compiledb.py | Switch code spec + source/container types to otf.artifacts. |
| src/gt4py/next/otf/compilation/build_systems/cmake.py | Switch code spec + source/container types to otf.artifacts. |
| src/gt4py/next/otf/compilation_tasks.py | Update type aliases to stages.*ProgramDef and executable program type to artifacts. |
| src/gt4py/next/otf/code_specs.py | Deleted (moved into otf.artifacts). |
| src/gt4py/next/otf/binding/nanobind.py | Switch code specs + binding/extension source types; use artifacts.format_source. |
| src/gt4py/next/otf/binding/interface.py | Remove format_source helper (moved to otf.artifacts). |
| src/gt4py/next/otf/artifacts.py | New module containing code specs, source containers, artifact protocols, and format_source. |
| src/gt4py/next/ffront/stages.py | Update ConcreteArtifact type aliases to point to otf.workflow. |
| src/gt4py/next/ffront/past_to_itir.py | Move CompilableProgramDef reference to otf.stages; update examples to workflow.ConcreteArtifact. |
| src/gt4py/next/ffront/past_process_args.py | Update ConcreteArtifact construction to otf.workflow. |
| src/gt4py/next/ffront/foast_to_past.py | Update ConcreteArtifact construction to otf.workflow. |
| src/gt4py/next/ffront/decorator.py | Update ConcreteArtifact construction to otf.workflow. |
| src/gt4py/next/backend.py | Switch backend contracts to stages (DSL-aware) + artifacts (DSL-agnostic). |
| docs/user/next/advanced/HackTheToolchain.md | Update tutorial snippets to new module layout (workflow/stages/artifacts). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The move itself checks out. I normalised the diff through the rename map Three comments, none of them blocking. They're all about the invariant the PR I measured them by building the module-level import graph from the AST at 1. The layering claim holds for
|
| module | before | after |
|---|---|---|
otf.compilation.compiler |
IR ✓ frontend ✓ | neither |
otf.compilation.build_systems.cmake |
IR ✓ frontend ✓ | neither |
otf.compilation.build_systems.compiledb |
IR ✓ frontend ✓ | neither |
otf.runners |
already neither | neither |
otf.binding.nanobind |
IR ✓ | IR ✓ |
otf.binding.cpp_interface |
IR ✓ | IR ✓ |
otf.compilation is genuinely severed from the IR and the frontend, and that's the real
result here — worth stating precisely, because it's the part that's true. otf.runners
was already clean before this PR. otf.binding still reaches the IR, unchanged:
otf/binding/nanobind.py:21 from gt4py.next.otf.binding import cpp_interface, interface
otf/binding/cpp_interface.py:13 from gt4py.next.type_system import type_info as ti, ...
type_system/type_info.py:19 from gt4py.next.iterator.type_system import ...
That last edge is the one you already have a TODO on — type_info.py:429, "This function
has specializations on Iterator types, which are not part of the general / shared type
system." So otf.binding can't be clean until is_compatible_type is split, which is
clearly not this PR's job.
Suggest narrowing the sentence to otf.compilation and noting otf.binding as blocked on
that TODO — it makes the claim checkable, and it puts a second caller behind fixing it.
2. artifacts.py states an invariant it doesn't hold
src/gt4py/next/otf/artifacts.py:17:
Nothing in here knows about GT4Py IRs or the type system.
The IR half is true — artifacts has no path to the IR at all. The type-system half isn't,
structurally rather than incidentally:
artifacts.py:29 from gt4py.next.otf.binding import interface
artifacts.ProgramSource.entry_point : interface.Function
binding/interface.py:19 Parameter.type_ : ts.TypeSpec
so import gt4py.next.otf.artifacts loads gt4py.next.type_system.type_specifications,
and every ProgramSource carries TypeSpecs in its entry point.
This is the module that defines where the DSL-agnostic line falls, so the docstring is
load-bearing: someone reading it would reasonably conclude artifacts can sink below the
type system, and it can't without moving binding.interface with it. The commit message's
narrower wording — no IR, no frontend — is the invariant that's actually true.
3. Nothing enforces the boundary, and it doesn't exist at runtime
At import time the separation isn't observable, because every one of these modules does
from gt4py.next import config, which executes the package __init__:
import gt4py.next.otf.compilation.compiler
# before: iterator=True ffront=True type_system=True (249 gt4py modules)
# after: iterator=True ffront=True type_system=True (248 gt4py modules)Which is fine — a cleaner source-level dependency graph is a good goal on its own. But it
does mean the invariant has no self-defence: it isn't visible in a profile, it doesn't
break a test when violated, and tach.toml treats gt4py.next as one opaque node, so
tach check says nothing about it either. No PR in the stack touches tach.toml.
That leaves 42 files of boundary that the next from gt4py.next.iterator import ... in
otf/compilation/ silently undoes. Worth landing the enforcement in the same PR that
creates the invariant — either tach.toml entries for the gt4py.next.otf.* submodules,
or a small test asserting the reachability set. Otherwise it's documentation, and this
stack is largely about not trusting documentation over checks.
| This module hosts the DSL-aware half of the on-the-fly compilation vocabulary | ||
| (ex `otf.definitions`, merged here): which forms a program definition can | ||
| take on its way from DSL source to a compilable IR program, and the typed | ||
| contracts of the steps a compile pipeline is composed of. The DSL-agnostic | ||
| artifact models these contracts produce live in `gt4py.next.otf.artifacts`. |
There was a problem hiding this comment.
| This module hosts the DSL-aware half of the on-the-fly compilation vocabulary | |
| (ex `otf.definitions`, merged here): which forms a program definition can | |
| take on its way from DSL source to a compilable IR program, and the typed | |
| contracts of the steps a compile pipeline is composed of. The DSL-agnostic | |
| artifact models these contracts produce live in `gt4py.next.otf.artifacts`. | |
| This module hosts the DSL-aware half of the on-the-fly compilation vocabulary: | |
| which forms a program definition can | |
| take on its way from DSL source to a compilable IR program, and the typed | |
| contracts of the steps a compile pipeline is composed of. The DSL-agnostic | |
| artifact models these contracts produce live in `gt4py.next.otf.artifacts`. |
There was a problem hiding this comment.
Please double-check if we are documenting history in comments/docstrings somewhere else.
There was a problem hiding this comment.
Applied, with the line-wrapping normalized. The history note is gone.
There was a problem hiding this comment.
Swept the 42 files this commit touches for ex <module> / moved from / merged here / formerly / previously / extracted from phrasing. Two real hits, both introduced by this commit, both removed:
otf/stages.py:13— "(exotf.definitions, merged here)" (your suggestion above).otf/artifacts.py:13— "(SourceCodeSpecand friends, excode_specsmodule)".
The remaining grep hits are not history notes and are left alone: otf/compilation/cache.py:86 ("incompatibility with previously cached builds" — describes runtime behaviour), artifacts.py:187 ("source code extracted from an ExtensionSource" — describes dataflow), and two pre-existing lines in the dace runner that this commit only re-indented.
| the toolchain. It deliberately lives in this DSL-neutral bottom module: | ||
| `ffront.stages` parameterizes it at module-import time, while the | ||
| DSL-aware stage definitions in `otf.stages` import `ffront.stages`. |
There was a problem hiding this comment.
Not 100% sure about this: should we keep these references to other modules? There is a potential for drift.
There was a problem hiding this comment.
Agreed on the drift risk, but the rationale is load-bearing, so I kept it and removed the drift-prone part.
The constraint is real and I verified it: ffront/stages.py:82,96,108,121 build workflow.ConcreteArtifact[...] aliases at module scope, and otf/stages.py:23 imports ffront.stages. So putting ConcreteArtifact in otf.stages closes an ffront.stages -> otf.stages -> ffront.stages cycle. Deleting the paragraph would leave the next reader free to "tidy" it into otf.stages and rediscover that the hard way.
What I dropped is the pair of module names, which is exactly the part that rots. The docstring now states the shape of the constraint ("those parameterize it while they are being imported, so hosting it any higher would close an import cycle") and points at the mechanical check instead of at siblings:
This is the envelope threaded through the definition-transforming half of
the toolchain, so it is generic over DSL-frontend types. It nevertheless
lives in this DSL-neutral bottom module rather than beside the stage
definitions that use it: those parameterize it while they are being
imported, so hosting it any higher would close an import cycle. The
invariant that keeps this working -- this module reaching no IR or
frontend module at import time -- is checked by the OTF import-boundary
test in `tests/next_tests/unit_tests/otf_tests/`.
otf.workflow is in that test's IR_FREE_MODULES set, and I confirmed it currently reaches zero ffront/iterator modules at import time. So if the prose does drift, the test fails rather than the docstring quietly lying.
5fd4ac0 to
f4abbb9
Compare
|
All three are valid. Addressed in the amended commit (force-pushed, I rebuilt the graph independently before changing anything, with the same construction you describe: module-level statements only, 1. Layering claim. Reproduced your numbers exactly, comparing
The three that changed all went through 2. 3. Enforcement. Added the test, not So:
I checked it actually bites: adding Verification: One thing I could not do: |
f4abbb9 to
ec3ea91
Compare
ec3ea91 to
5c6854a
Compare
|
Two follow-ups to my previous reply, both after re-running the new check against the whole stack. The base is now retargeted. #2736 was squash-merged, so this PR was still The import-boundary test was wrong, and running it across the stack is what That is not a real violation. It now walks only what runs at module-load time: class bodies and module-level I re-verified it still bites rather than passing vacuously: injecting a |
5c6854a to
2155d4d
Compare
457d4a1 to
b415489
Compare
… enforce the layering Restructure the `gt4py.next.otf` modules so that the DSL-aware and DSL-agnostic halves of the toolchain vocabulary live in separate modules, without renaming any class: - Merge `otf.definitions` into `otf.stages`: definition-stage TypeVars and aliases (`IRDefinitionT`, `ArgsDefinitionT`, `ConcreteProgramDef`, `CompilableProgramDef`) and the step contracts (`TranslationStep`, `CompilationStep`) now live in `stages`; `otf.definitions` is deleted. - Extract `otf.artifacts`: the DSL-agnostic artifact models (`ProgramSource`, `BindingSource`, `ExtensionSource`, `BuildSystemProject`, `ExecutableProgram`, `CompilationArtifact`) plus all code-spec classes (`otf.code_specs` is deleted) and `format_source` (moved from `binding.interface`, breaking the interface->code_specs import edge). - Move the `ConcreteArtifact` pair type from `otf.toolchain` to the DSL-neutral bottom module `otf.workflow`. - Drop the step-protocol base classes from `CPPCompiler`, `DaCeTranslator` and `DaCeCompiler`; the protocols are structural, so nothing changes for isinstance/fingerprint purposes (fingerprints use qualified names only). - Delete `otf.stages.compilable_program_fingerprinter`, a one-line alias for `fingerprinting.strict_fingerprinter` with two users, and name the fingerprinter directly at the gtfn and dace call sites instead. Note that ADR 0023 still refers to the alias by name, and describes that input fingerprinter as lenient where the code has always used the strict one; that pre-existing drift is left for a follow-up, since ADRs are append-only. This severs `otf.compilation` from the IRs, and only that. Measured on the module-level import graph (AST, counting only what a real interpreter runs when the module is loaded -- the `if TYPE_CHECKING:` branch and function-body imports are not edges, though a `TYPE_CHECKING` `else:` is -- and resolving `from gt4py.next import config` to `gt4py.next.config` rather than to the package): before, `compilation.compiler`, `compilation.build_systems.cmake` and `compilation.build_systems.compiledb` each reached 10 `ffront`/`iterator` modules through `otf.definitions -> ffront.stages`; after, they reach none. `otf.runners` and `otf.compilation.cache` were already IR-free before this change, so no credit is due there. `otf.binding` is *not* severed: it still reaches `iterator.type_system.type_specifications` through `binding.cpp_interface -> type_system.type_info`, unchanged by this commit. Fixing that means splitting `is_compatible_type`, whose iterator specializations already carry a TODO at `src/gt4py/next/type_system/type_info.py:429`; that is out of scope here. `otf.artifacts` is IR- and frontend-free but not type-system-free: a `ProgramSource` entry point is an `otf.binding.interface.Function` whose parameters carry `TypeSpec`s, so importing `otf.artifacts` loads `type_system.type_specifications` by construction. `tests/next_tests/unit_tests/otf_tests/test_import_boundaries.py` turns the above into a check instead of a claim. It reconstructs that same graph from the AST and asserts the DSL-agnostic OTF modules (all of `otf.compilation.*`, plus `otf.workflow`, `otf.artifacts`, `otf.binding.interface` and `otf.runners`) reach no IR module, while pinning `otf.binding`'s remaining IR edge exactly, so the debt can neither grow nor silently outlive its fix. Deferring an import into a function body is deliberately not an edge: it is the sanctioned way to reach a higher layer without an import-time dependency, and `instrumentation.stage_dump` relies on it to pretty-print FOAST stages. It cannot be done by importing the modules: each of them does `from gt4py.next import config`, which executes the package `__init__` and pulls in the whole DSL, making every module reachable from every other at runtime. `tach` is also the wrong tool here -- it models direct edges between coarse declared modules, not transitive reachability, and with `exact = true` a single `gt4py.next.otf.compilation` entry forces the reverse edges (`runners`, `gtfn`, the dace workflow) to be declared too, cascading into a repo-wide restructure of `tach.toml`. Moving `ConcreteArtifact` rotates the two persistent translation-cache keys (gtfn and dace) once. The strict input fingerprinter tags a dataclass with its fully qualified name, so `otf.toolchain.ConcreteArtifact` and `otf.workflow.ConcreteArtifact` hash differently even though the fields are unchanged. The effect is a one-time cold rebuild, never a stale hit, and `BUILD_CACHE_VERSION_ID` already rotates these keys at every release. The later `ConcreteArtifact` -> `ProgramWithArgs` rename rotates them a second time, so the two collapse into a single cold rebuild only if both land in the same release. Breaking: `gt4py.next.otf.definitions` and `gt4py.next.otf.code_specs` are gone, and the names they held must be imported from `gt4py.next.otf.stages` and `gt4py.next.otf.artifacts` respectively; `otf.toolchain.ConcreteArtifact` moves to `otf.workflow.ConcreteArtifact`. No compatibility re-exports are left behind.
b415489 to
0655582
Compare
Restructure the
gt4py.next.otfmodules so that the DSL-aware andDSL-agnostic halves of the toolchain vocabulary live in separate modules,
without renaming any class:
otf.definitionsintootf.stages: definition-stage TypeVars andaliases (
IRDefinitionT,ArgsDefinitionT,ConcreteProgramDef,CompilableProgramDef) and the step contracts (TranslationStep,CompilationStep) now live instages;otf.definitionsis deleted.otf.artifacts: the DSL-agnostic artifact models (ProgramSource,BindingSource,ExtensionSource,BuildSystemProject,ExecutableProgram,CompilationArtifact) plus all code-spec classes(
otf.code_specsis deleted) andformat_source(moved frombinding.interface, breaking the interface->code_specs import edge).ConcreteArtifactpair type fromotf.toolchainto theDSL-neutral bottom module
otf.workflow.CPPCompiler,DaCeTranslatorand
DaCeCompiler; the protocols are structural, so nothing changes forisinstance/fingerprint purposes (fingerprints use qualified names only).
otf.stages.compilable_program_fingerprinter, a one-line alias forfingerprinting.strict_fingerprinterwith two users, and name thefingerprinter directly at the gtfn and dace call sites instead. Note that
ADR 0023 still refers to the alias by name, and describes that input
fingerprinter as lenient where the code has always used the strict one;
that pre-existing drift is left for a follow-up, since ADRs are
append-only.
This severs
otf.compilationfrom the IRs, and only that. Measured on themodule-level import graph (AST, counting only what a real interpreter runs
when the module is loaded -- the
if TYPE_CHECKING:branch and function-bodyimports are not edges, though a
TYPE_CHECKINGelse:is -- and resolvingfrom gt4py.next import configtogt4py.next.configrather than to thepackage): before,
compilation.compiler,compilation.build_systems.cmakeandcompilation.build_systems.compiledbeach reached 10ffront/iteratormodules through
otf.definitions -> ffront.stages; after, they reach none.otf.runnersandotf.compilation.cachewere already IR-free before thischange, so no credit is due there.
otf.bindingis not severed: it stillreaches
iterator.type_system.type_specificationsthroughbinding.cpp_interface -> type_system.type_info, unchanged by this commit.Fixing that means splitting
is_compatible_type, whose iteratorspecializations already carry a TODO at
src/gt4py/next/type_system/type_info.py:429; that is out of scope here.otf.artifactsis IR- and frontend-free but not type-system-free: aProgramSourceentry point is anotf.binding.interface.Functionwhoseparameters carry
TypeSpecs, so importingotf.artifactsloadstype_system.type_specificationsby construction.tests/next_tests/unit_tests/otf_tests/test_import_boundaries.pyturns theabove into a check instead of a claim. It reconstructs that same graph from
the AST and asserts the DSL-agnostic OTF modules (all of
otf.compilation.*, plusotf.workflow,otf.artifacts,otf.binding.interfaceandotf.runners) reach no IR module, while pinningotf.binding's remaining IR edge exactly, so the debt can neither grow norsilently outlive its fix. Deferring an import into a function body is
deliberately not an edge: it is the sanctioned way to reach a higher layer
without an import-time dependency, and
instrumentation.stage_dumprelieson it to pretty-print FOAST stages. It cannot be done by importing the
modules: each
of them does
from gt4py.next import config, which executes the package__init__and pulls in the whole DSL, making every module reachable fromevery other at runtime.
tachis also the wrong tool here -- it modelsdirect edges between coarse declared modules, not transitive reachability,
and with
exact = truea singlegt4py.next.otf.compilationentry forcesthe reverse edges (
runners,gtfn, the dace workflow) to be declared too,cascading into a repo-wide restructure of
tach.toml.Moving
ConcreteArtifactrotates the two persistent translation-cache keys(gtfn and dace) once. The strict input fingerprinter tags a dataclass with
its fully qualified name, so
otf.toolchain.ConcreteArtifactandotf.workflow.ConcreteArtifacthash differently even though the fields areunchanged. The effect is a one-time cold rebuild, never a stale hit, and
BUILD_CACHE_VERSION_IDalready rotates these keys at every release. Thelater
ConcreteArtifact->ProgramWithArgsrename rotates them a secondtime, so the two collapse into a single cold rebuild only if both land in
the same release.
Breaking:
gt4py.next.otf.definitionsandgt4py.next.otf.code_specsaregone, and the names they held must be imported from
gt4py.next.otf.stagesand
gt4py.next.otf.artifactsrespectively;otf.toolchain.ConcreteArtifactmoves to
otf.workflow.ConcreteArtifact. No compatibility re-exports areleft behind.