refactor[next]: replace factory-boy factories with plain builders, fixing run_gtfn_imperative - #2808
Open
egparedes wants to merge 1 commit into
Open
Conversation
egparedes
force-pushed
the
otf-split-1b-plain-builders
branch
from
August 20, 2026 16:51
65c6143 to
a0e589a
Compare
egparedes
force-pushed
the
otf-split-1b-plain-builders
branch
2 times, most recently
from
August 26, 2026 11:05
e059173 to
967b309
Compare
egparedes
force-pushed
the
otf-split-1b-plain-builders
branch
from
August 28, 2026 13:06
967b309 to
5ec40db
Compare
egparedes
force-pushed
the
otf-split-1b-plain-builders
branch
from
August 28, 2026 13:09
5ec40db to
0cc6d16
Compare
…xing run_gtfn_imperative `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. Replace the seven 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). Builders follow two rules: a builder takes cross-cutting configuration only (device, caching, build type, auto-optimize) and configures the steps it creates from it; an injected sub-component is used verbatim. The second rule needs a guard, so `workflow.check_device_agreement` raises when an injected step's device disagrees with the pipeline being built. It inspects only steps that structurally declare a device (`workflow.DeviceConfigurable`) and is used to check, never to mutate. Behavior change: `run_gtfn_imperative` becomes imperative for the first time. It was declared with `otf_workflow__translation__use_imperative_backend=True`, but `translation` is a `LazyAttribute` rather than a `SubFactory`, so factory-boy dropped the deep path silently. (The sibling override on `run_gtfn_no_transforms` did take effect precisely because `bare_translation` *is* a `SubFactory` — that asymmetry is the whole bug.) The backend was a duplicate of `run_gtfn` under another name, so the `GTFN_CPU_IMPERATIVE` test-matrix entry had never exercised imperative code generation. Switching it on exposes a pre-existing IR defect: the imperative code path leaves CSE temporaries undeclared, so symbol validation rejects the IR. That is tracked as issue #2810 and is out of scope for a construction refactor, so the two call sites that hit it -- `test_hdiff` and `test_concat_where::test_lap_like[static_domains]` -- are xfailed against it. `test_lap_like[dynamic_domains]` does not fold enough to trigger the defect and is deliberately left running. Second 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. Three are fixed here (device-type narrowing in both backend builders, and `CachedStep.persistent`'s unsolved `HashT` at two call sites). One remains as a scoped, documented `type: ignore`: `OTFCompileWorkflow` is not parameterized over the code spec, so its `bindings` field is typed for `ProgramSource[Any]` while `ExtensionGenerator` accepts only C++-like specs. Parameterizing the pipeline is the real fix and belongs with the pipeline rework. `make_dace_backend` deliberately keeps its translator-local keyword arguments, so external callers are unaffected. See ADR 0028.
egparedes
force-pushed
the
otf-split-1b-plain-builders
branch
from
September 1, 2026 18:30
0cc6d16 to
428816a
Compare
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.
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.
Replace the seven 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).
Builders follow two rules: a builder takes cross-cutting configuration only
(device, caching, build type, auto-optimize) and configures the steps it
creates from it; an injected sub-component is used verbatim. The second rule
needs a guard, so
workflow.check_device_agreementraises when an injectedstep's device disagrees with the pipeline being built. It inspects only steps
that structurally declare a device (
workflow.DeviceConfigurable) and is usedto check, never to mutate.
Behavior change:
run_gtfn_imperativebecomes imperative for the first time.It was declared with
otf_workflow__translation__use_imperative_backend=True,but
translationis aLazyAttributerather than aSubFactory, sofactory-boy dropped the deep path silently. (The sibling override on
run_gtfn_no_transformsdid take effect precisely becausebare_translationis a
SubFactory— that asymmetry is the whole bug.) The backend was aduplicate of
run_gtfnunder another name, so theGTFN_CPU_IMPERATIVEtest-matrix entry had never exercised imperative code generation.
Switching it on exposes a pre-existing IR defect: the imperative code path
leaves CSE temporaries undeclared, so symbol validation rejects the IR. That
is tracked as issue #2810 and is out of scope for a construction refactor, so
the two call sites that hit it --
test_hdiffandtest_concat_where::test_lap_like[static_domains]-- are xfailed against it.test_lap_like[dynamic_domains]does not fold enough to trigger the defectand is deliberately left running.
Second latent bug:
run_gtfn_no_transforms.namewasrun_gtfn_cpu,colliding with
run_gtfn. It is nowrun_gtfn_cpu_no_transforms. Thisrotates no cache -- the build cache keys on the entry-point name plus a
fingerprint of the
ExtensionSource, and the translation-cache directory iskeyed on the literal backend family (
gtfn/dace).Backend.namereachesonly 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 enoughsuppressions insrc/, which had beenmasking real typing problems. Three are fixed here (device-type narrowing in
both backend builders, and
CachedStep.persistent's unsolvedHashTat twocall sites). One remains as a scoped, documented
type: ignore:OTFCompileWorkflowis not parameterized over the code spec, so itsbindingsfield is typed forProgramSource[Any]whileExtensionGeneratoraccepts only C++-like specs. Parameterizing the pipeline is the real fix and
belongs with the pipeline rework.
make_dace_backenddeliberately keeps its translator-local keywordarguments, so external callers are unaffected.
See ADR 0028.