You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
factory-boy is a test-data library, but gt4py.next used it in production
to compose the GTFN and DaCe backends and their compile workflows. Every
object it built is already a frozen dataclass, so the Trait/SubFactory/SelfAttribute/LazyAttribute machinery added a second
construction language that no type checker can see, and its __-path
overrides failed silently (run_gtfn_imperative was the declarative backend
under another name for its whole life).
Replace the eight factory classes with plain builder functions and move factory-boy to the test dependency group (the cartesian and eve IR
test-data factories keep using it for its intended purpose).
The builders follow three rules (ADR 0028):
Shared settings live in one config.GTFNConfig and DaCeConfig
(both extending backend.ToolchainConfig) hold what the steps must agree
on: device, build type, cache lifetime, data layout, translation caching,
and for DaCe auto-optimize and the external workspace. Defaults are read
from gt4py.next.config when the config is created.
Every step is created by a step builder that receives the config and
takes only step-local settings as keyword arguments, so a shared setting
cannot be set for one step alone. A step is customized by passing a
different builder: a functools.partial of the default one, or any
callable taking the config.
The toolchain builder owns the composition. It wraps the translation
step in the cache after its builder ran, so a customization always lands on
the bare step. A fully custom step builder is responsible for configuring
its step consistently with the config it receives; its output is not
validated.
mypy rejects misspelled step-local settings, wrong value types, and attempts
to set a shared setting through a step builder; the same mistakes raise TypeError when the toolchain is built.
The step builders take step-local settings under the step's own field names,
e.g. make_dace_translator(cfg, auto_optimize_args=..., disable_itir_transforms=..., disable_field_origin_on_program_arguments=...) and make_dace_compiler(cfg, add_gpu_trace_markers=...). The step-builder types
(GTFNTranslationBuilder, GTFNBindingsBuilder, GTFNCompilationBuilder and
their DaCe* counterparts) are public in runners.gtfn and runners.dace.
DaCeTranslator derives unit_strides_kind itself (next to gpu and constant_symbols) and rejects these three keys in auto_optimize_args on
construction, so the check covers every way a translator is built, including
direct construction and dataclasses.replace.
The translation-cache wrapping is shared in otf.compilation.cache.persistent_translation_cache.
make_dace_backend keeps its flat keyword signature as a front end over make_dace_toolchain, but is deprecated (DeprecationWarning). It
builds field-identical toolchains for the same arguments, so external callers
(icon4py) keep working until they migrate.
A compile workflow built on its own now caches its translation step by
default, like the toolchains; GTFNConfig(cached_translation=False) / DaCeConfig(cached_translation=False) opt out.
GTFNTranslationStep.device_type has no default any more: a builder that
forgets to pass it fails instead of silently targeting the CPU.
Rebase adaptation (2026-09): while this PR was pending, the gtfn imperative
backend was removed from the codebase in #2877, resolving #2810. run_gtfn_imperative is therefore dropped instead of fixed, together with the
two #2810 xfails; the incident itself is documented in the Context of
ADR 0028.
Latent bug: run_gtfn_no_transforms.name was run_gtfn_cpu, colliding with run_gtfn. It is now run_gtfn_cpu_no_transforms. This rotates no cache --
the build cache keys on the entry-point name plus a fingerprint of the ExtensionSource, and the translation-cache directory is keyed on the
literal backend family (gtfn / dace). Backend.name is only used in the
metrics metadata, in the per-name bookkeeping of compiled-program pools, and
in the DaCe __sdfg__ check for a dace backend (which neither name passes),
so the collision's real cost was two distinct backends sharing one metrics
identity and one pool name.
All other pre-built backends are unchanged, verified field-by-field against
the previous construction, except that the DaCe translators no longer store unit_strides_kind in auto_optimize_args (the translator now derives it,
with the same value). This rotates the DaCe translation-cache keys once.
Removing the factories also removed the 8 # type: ignore[assignment] # factory-boy typing not precise enough suppressions in src/, which had
been masking real typing problems. One remains as a scoped, documented type: ignore in make_gtfn_bindings: OTFCompileWorkflow is not
parameterized over the code spec, while ExtensionGenerator accepts only
C++-like specs. The pipeline rework (#2743) drops the ignore, but only because
its workflow.Step fields accept ProgramSource[Any]; parameterizing the
pipeline over the code spec is left to a follow-up (see ADR 0029).
egparedes
changed the title
refactor[next]: replace factory-boy factories with plain builders
refactor[next]: replace factory-boy factories with plain builders, fixing run_gtfn_imperative
Aug 28, 2026
The new DaCeConfig.cached_translation branch is not covered by the added runner tests: they exercise cached defaults, while only the analogous GTFN path tests cached_translation=False. Please add a DaCe regression test that builds with this flag and asserts the workflow contains the bare translator, otherwise this advertised opt-out can regress unnoticed.
Replying to the Copilot review summaries, whose suggestions have no inline threads:
DaCe cached_translation=False test: added in 81df14d (test_make_toolchain_uncached_translation).
Device check on the bindings step: not done. No bindings step records a device (ExtensionGenerator, the bind_sdfg partial), so check_device_agreement would always pass. The check runs on the steps that do record one: translation and compilation.
Expose code_spec in make_gtfn_translation: not done, on purpose. GTFNTranslationStep derives it from device_type. Making it a step-local setting would let it disagree with the device, which the step builders are meant to prevent. A custom translation builder can still set it.
Reject derived optimization keys when auto-optimize is off: not done. make_dace_backend never checked them in that case; it only warns that the arguments are unused. This PR keeps that behavior, and changing it is out of scope.
Expose add_gpu_trace_markers on make_dace_compiler: not done. It already defaults from config.ADD_GPU_TRACE_MARKERS, and a custom compilation builder can set it. The step builders only list settings that callers of the old factories actually used.
`factory-boy` is a test-data library, but `gt4py.next` used it in production
to compose the GTFN and DaCe backends and their compile workflows. Every
object it built is already a frozen dataclass, so the
`Trait`/`SubFactory`/`SelfAttribute`/`LazyAttribute` machinery added a second
construction language that no type checker can see, and its `__`-path
overrides failed silently (`run_gtfn_imperative` was the declarative backend
under another name for its whole life).
Replace the eight factory classes with plain builder functions and move
`factory-boy` to the `test` dependency group (the cartesian and eve IR
test-data factories keep using it for its intended purpose).
The builders follow three rules (ADR 0028):
1. **Shared settings live in one config.** `GTFNConfig` and `DaCeConfig`
(both extending `backend.ToolchainConfig`) hold what the steps must agree
on: device, build type, cache lifetime, data layout, translation caching,
and for DaCe auto-optimize and the external workspace. Defaults are read
from `gt4py.next.config` when the config is created.
2. **Every step is created by a step builder that receives the config** and
takes only step-local settings as keyword arguments, so a shared setting
cannot be set for one step alone. A step is customized by passing a
different builder: a `functools.partial` of the default one, or any
callable taking the config.
3. **The toolchain builder owns the composition.** It wraps the translation
step in the cache after its builder ran, so a customization always lands on
the bare step, and checks the device of whatever a custom builder returns
(`workflow.check_device_agreement`: check, never mutate).
```python
gtfn.make_gtfn_toolchain(
gtfn.GTFNConfig(gpu=True),
name_postfix="_no_transforms",
translation=functools.partial(gtfn.make_gtfn_translation, enable_itir_transforms=False),
)
```
mypy rejects misspelled step-local settings, wrong value types, and attempts
to set a shared setting through a step builder; the same mistakes raise
`TypeError` when the toolchain is built.
API:
- `make_gtfn_toolchain`, `make_gtfn_compile_workflow`, `make_gtfn_translation`,
`make_gtfn_bindings`, `make_gtfn_compiler`, `make_gtfn_build_system`.
- `make_dace_toolchain`, `make_dace_compile_workflow`, `make_dace_translator`,
`make_dace_bindings`, `make_dace_compiler`.
- `make_dace_backend` keeps its flat keyword signature as a front end over
`make_dace_toolchain`, so external callers (icon4py) are unaffected; it
builds field-identical toolchains for the same arguments.
- `GTFNTranslationStep.device_type` has no default any more: a builder that
forgets to pass it fails instead of silently targeting the CPU.
Rebase adaptation (2026-09): while this PR was pending, the gtfn imperative
backend was removed from the codebase in #2877, resolving #2810.
`run_gtfn_imperative` is therefore dropped instead of fixed, together with the
two `#2810` xfails; the incident itself is documented in the Context of
ADR 0028.
Latent bug: `run_gtfn_no_transforms.name` was `run_gtfn_cpu`, colliding with
`run_gtfn`. It is now `run_gtfn_cpu_no_transforms`. This rotates no cache --
the build cache keys on the entry-point name plus a fingerprint of the
`ExtensionSource`, and the translation-cache directory is keyed on the
literal backend family (`gtfn` / `dace`). `Backend.name` reaches only the
metrics source key and one error message, so the collision's real cost was
two distinct backends sharing one metrics identity.
All other pre-built backends are unchanged, verified field-by-field against
the previous construction.
Removing the factories also removed the 8 `# type: ignore[assignment] #
factory-boy typing not precise enough` suppressions in `src/`, which had
been masking real typing problems. One remains as a scoped, documented
`type: ignore` in `make_gtfn_bindings`: `OTFCompileWorkflow` is not
parameterized over the code spec, while `ExtensionGenerator` accepts only
C++-like specs. Parameterizing the pipeline belongs with the pipeline rework.
Migration for downstream code:
- `GTFNBackendFactory(gpu=on_gpu)` -> `make_gtfn_toolchain(GTFNConfig(gpu=on_gpu))`
- `DaCeBackendFactory(..., otf_workflow__bare_translation__async_sdfg_call=False)`
-> `make_dace_backend(..., async_sdfg_call=False)`
See ADR 0028.
…e device check
Address review: the dace package facade exposed every new builder except
make_dace_compile_workflow, and only the GTFN side of the device-agreement
check of custom step builders was tested. Also pin the corrected
run_gtfn_no_transforms name.
The reason will be displayed to describe this comment to others. Learn more.
I still believe passing a dict is the better approach, with a niceer interface, good maintainability and extensibility in the future. Nonetheless this is a nice improvement to the current state.
Notes on this approach:
not composable: only one global config that needs to store everything. Right now we don't need more, but the approach inherently fixes this.
hard to understand how information from the config flows into the various steps as forwarding is burred into verbose factories, e.g. make_dace_bindings is an example for this. Inversion from factory boy: instead of allowing configuration flow from the parent, the information is consumed in the client. From discussion in person, @egparedes likes the other direction more: Every component gathers the information it needs to construct itself. Additionally new features added to a component might silently not be setup correctly in an upper step.
The inversion also makes consistency checks of the config harder to verify and requires more thorough unit tests.An example for this is make_dace_backend. Similary the logic for constructing steps is placed in many small factories now.
The reason will be displayed to describe this comment to others. Learn more.
I think some of the information here should not be in the ADR, but part of the PR description. Everything that is a statement about the code in other places rods very quick and will confuse agents as they take this for granted.
The reason will be displayed to describe this comment to others. Learn more.
Agreed, done in 0e472ed. The ADR now only has the context, the decision, the consequences in abstract terms and the alternatives. The statements about specific code (suppression counts, the run_gtfn_no_transforms rename and its cache/metrics analysis, the verification of the pre-built toolchains, the migration list) now live in the PR description.
The reason will be displayed to describe this comment to others. Learn more.
I would remove this, it adds little value and it is unclear where consistency checks end. Checking everything is certainly impractical and adds a lot of code for a narrow use case. I would argue it's the responsibility of the user to make sure a workflow is configured consistently with the rest.
The reason will be displayed to describe this comment to others. Learn more.
Agreed, removed in 0e472ed together with the DeviceConfigurable protocol. With the config-based builders, the default and functools.partial-customized steps are in sync by construction; the only remaining case is a fully custom step builder, and the builders' docstrings, the ADR and HackTheToolchain now state that configuring it consistently is the caller's responsibility. The construction-time checks that #2743 adds on CompilePipeline/Toolchain reuse the same protocol, so the same question applies there; let's settle it once in that PR.
The reason will be displayed to describe this comment to others. Learn more.
the `TypedDict`s mirror the step fields and drift from them, they cannot
This is a little misleading as drift can only occur in one direction: The step could have more settings than the typed dict, but not the other way around. So correctness is not an issue. Additionally the approach taken in this PR has the very same issue, if you don't put the argument of a Step into its factory it is invisible to the user inspecting the factory only.
The reason will be displayed to describe this comment to others. Learn more.
You're right, and I checked it: mypy checks a **typed_dict unpacked into the step constructor against its parameters (a stale key gives Extra argument ... from **args), so the only possible drift is a step setting missing from the dict, which the step builders here share. Rewritten in 0e472ed: the paragraph now says so, and the remaining reasons for not choosing it are that replacing a whole step needs a second mechanism, and that shared settings have to be forwarded by hand through each builder layer. Your structural points (one flat, non-composable config; configuration pulled by each step builder instead of pushed by the parent, so a builder's signature doesn't show what it reads; construction logic spread over many small builders) are now recorded as consequences.
The reason will be displayed to describe this comment to others. Learn more.
Done in 0e472ed: make_dace_backend now emits a DeprecationWarning pointing to make_dace_toolchain + functools.partial(make_dace_translator, ...), and all internal callers (the DaCe backend and bindings tests) use the new builder. A test pins the warning and that the deprecated path still builds the same toolchain. The migration is in the PR description.
- Remove the builder-level device check (`check_device_agreement` and the
`DeviceConfigurable` protocol): default and partially customized steps are
in sync by construction, and configuring a fully custom step builder
consistently is the caller's responsibility.
- Deprecate `make_dace_backend` in favor of `make_dace_toolchain`, and move
the internal callers to the new builder.
- Trim ADR 0028 to the decision: statements about specific code move to the
PR description, the `TypedDict` alternative is described accurately, and
the structural costs raised in review are recorded as consequences.
- `DaCeTranslator` derives `unit_strides_kind` itself, next to `gpu` and
`constant_symbols`, and rejects these three keys in `auto_optimize_args`
on construction, so the check covers every way a translator is built.
The warning about unused optimization arguments stays in the step builder,
where it is attributed to the caller.
- `make_dace_translator` takes the step-local fields of `DaCeTranslator` under
their own names (`auto_optimize_args`,
`disable_field_origin_on_program_arguments`) and now also exposes
`disable_itir_transforms`; `make_dace_compiler` exposes
`add_gpu_trace_markers`. The deprecated `make_dace_backend` maps its legacy
names onto them.
- Share the translation-cache wrapping in
`cache.persistent_translation_cache`, and name the step-builder types
(`GTFN*Builder`, `DaCe*Builder`).
- The deprecation test checks that `make_dace_backend` builds the same
toolchain as the equivalent `make_dace_toolchain` call.
- ADR 0028 states the no-default rule accurately; fix a stale comment in
`otf/compilation/cache.py`; note that a `DaCeConfig` holding a workspace is
not hashable.
The DaCe translators no longer store `unit_strides_kind` in
`auto_optimize_args`, so DaCe translation-cache keys rotate once; the value
passed to `gt_auto_optimize` is unchanged.
The reason will be displayed to describe this comment to others. Learn more.
Valid, fixed in 65e9f39: DaCeTranslationBuilder, DaCeBindingsBuilder and DaCeCompilationBuilder are now imported in runners.dace and listed in its __all__.
The reason will be displayed to describe this comment to others. Learn more.
Valid, added in 65e9f39: test_compile_workflow_without_config_caches_translation (DaCe) calls make_dace_compile_workflow() with no arguments and asserts the translator is wrapped in a CachedStep.
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 65e9f39, in the error message, the make_dace_backend docstring and the test assertions. (The typo predates this PR, it was in make_dace_backend on main.)
The reason will be displayed to describe this comment to others. Learn more.
Valid, added in 65e9f39: test_compile_workflow_without_config_caches_translation (GTFN) calls make_gtfn_compile_workflow() with no arguments and asserts a cached GTFNTranslationStep.
…defaults
Address review:
- Export `DaCeTranslationBuilder`, `DaCeBindingsBuilder` and
`DaCeCompilationBuilder` from `runners.dace`, next to the other builders.
- Fix "overriden" in the error for derived optimization arguments.
- Test that `GTFNTranslationStep` requires `device_type`, and that
`make_gtfn_compile_workflow()` and `make_dace_compile_workflow()` without a
config cache their translation step.
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
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.
factory-boyis a test-data library, butgt4py.nextused it in productionto compose the GTFN and DaCe backends and their compile workflows. Every
object it built is already a frozen dataclass, so the
Trait/SubFactory/SelfAttribute/LazyAttributemachinery added a secondconstruction language that no type checker can see, and its
__-pathoverrides failed silently (
run_gtfn_imperativewas the declarative backendunder another name for its whole life).
Replace the eight factory classes with plain builder functions and move
factory-boyto thetestdependency group (the cartesian and eve IRtest-data factories keep using it for its intended purpose).
The builders follow three rules (ADR 0028):
GTFNConfigandDaCeConfig(both extending
backend.ToolchainConfig) hold what the steps must agreeon: device, build type, cache lifetime, data layout, translation caching,
and for DaCe auto-optimize and the external workspace. Defaults are read
from
gt4py.next.configwhen the config is created.takes only step-local settings as keyword arguments, so a shared setting
cannot be set for one step alone. A step is customized by passing a
different builder: a
functools.partialof the default one, or anycallable taking the config.
step in the cache after its builder ran, so a customization always lands on
the bare step. A fully custom step builder is responsible for configuring
its step consistently with the config it receives; its output is not
validated.
mypy rejects misspelled step-local settings, wrong value types, and attempts
to set a shared setting through a step builder; the same mistakes raise
TypeErrorwhen the toolchain is built.API:
make_gtfn_toolchain,make_gtfn_compile_workflow,make_gtfn_translation,make_gtfn_bindings,make_gtfn_compiler,make_gtfn_build_system.make_dace_toolchain,make_dace_compile_workflow,make_dace_translator,make_dace_bindings,make_dace_compiler.e.g.
make_dace_translator(cfg, auto_optimize_args=..., disable_itir_transforms=..., disable_field_origin_on_program_arguments=...)andmake_dace_compiler(cfg, add_gpu_trace_markers=...). The step-builder types(
GTFNTranslationBuilder,GTFNBindingsBuilder,GTFNCompilationBuilderandtheir
DaCe*counterparts) are public inrunners.gtfnandrunners.dace.DaCeTranslatorderivesunit_strides_kinditself (next togpuandconstant_symbols) and rejects these three keys inauto_optimize_argsonconstruction, so the check covers every way a translator is built, including
direct construction and
dataclasses.replace.otf.compilation.cache.persistent_translation_cache.make_dace_backendkeeps its flat keyword signature as a front end overmake_dace_toolchain, but is deprecated (DeprecationWarning). Itbuilds field-identical toolchains for the same arguments, so external callers
(icon4py) keep working until they migrate.
default, like the toolchains;
GTFNConfig(cached_translation=False)/DaCeConfig(cached_translation=False)opt out.GTFNTranslationStep.device_typehas no default any more: a builder thatforgets to pass it fails instead of silently targeting the CPU.
Rebase adaptation (2026-09): while this PR was pending, the gtfn imperative
backend was removed from the codebase in #2877, resolving #2810.
run_gtfn_imperativeis therefore dropped instead of fixed, together with thetwo
#2810xfails; the incident itself is documented in the Context ofADR 0028.
Latent bug:
run_gtfn_no_transforms.namewasrun_gtfn_cpu, colliding withrun_gtfn. It is nowrun_gtfn_cpu_no_transforms. This rotates no cache --the build cache keys on the entry-point name plus a fingerprint of the
ExtensionSource, and the translation-cache directory is keyed on theliteral backend family (
gtfn/dace).Backend.nameis only used in themetrics metadata, in the per-name bookkeeping of compiled-program pools, and
in the DaCe
__sdfg__check for adacebackend (which neither name passes),so the collision's real cost was two distinct backends sharing one metrics
identity and one pool name.
All other pre-built backends are unchanged, verified field-by-field against
the previous construction, except that the DaCe translators no longer store
unit_strides_kindinauto_optimize_args(the translator now derives it,with the same value). This rotates the DaCe translation-cache keys once.
Removing the factories also removed the 8
# type: ignore[assignment] # factory-boy typing not precise enoughsuppressions insrc/, which hadbeen masking real typing problems. One remains as a scoped, documented
type: ignoreinmake_gtfn_bindings:OTFCompileWorkflowis notparameterized over the code spec, while
ExtensionGeneratoraccepts onlyC++-like specs. The pipeline rework (#2743) drops the ignore, but only because
its
workflow.Stepfields acceptProgramSource[Any]; parameterizing thepipeline over the code spec is left to a follow-up (see ADR 0029).
Migration for downstream code:
GTFNBackendFactory(gpu=on_gpu)->make_gtfn_toolchain(GTFNConfig(gpu=on_gpu))DaCeBackendFactory(..., otf_workflow__bare_translation__async_sdfg_call=False)->
make_dace_toolchain(DaCeConfig(...), translation=functools.partial(make_dace_translator, async_sdfg_call=False))make_dace_backend(gpu=..., use_metrics=..., optimization_args=..., use_zero_origin=..., ...)->
make_dace_toolchain(DaCeConfig(gpu=...), translation=functools.partial(make_dace_translator, use_metrics=..., auto_optimize_args=..., disable_field_origin_on_program_arguments=..., ...))See ADR 0028.