Skip to content

refactor[next]: replace factory-boy factories with plain builders, fixing run_gtfn_imperative - #2808

Open
egparedes wants to merge 1 commit into
otf-split-1-stages-artifactsfrom
otf-split-1b-plain-builders
Open

refactor[next]: replace factory-boy factories with plain builders, fixing run_gtfn_imperative#2808
egparedes wants to merge 1 commit into
otf-split-1-stages-artifactsfrom
otf-split-1b-plain-builders

Conversation

@egparedes

@egparedes egparedes commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

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
egparedes force-pushed the otf-split-1b-plain-builders branch from 65c6143 to a0e589a Compare August 20, 2026 16:51
@egparedes
egparedes force-pushed the otf-split-1b-plain-builders branch 2 times, most recently from e059173 to 967b309 Compare August 26, 2026 11:05
@egparedes
egparedes force-pushed the otf-split-1b-plain-builders branch from 967b309 to 5ec40db Compare August 28, 2026 13:06
@egparedes
egparedes force-pushed the otf-split-1b-plain-builders branch from 5ec40db to 0cc6d16 Compare August 28, 2026 13:09
@egparedes 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
…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
egparedes force-pushed the otf-split-1b-plain-builders branch from 0cc6d16 to 428816a Compare September 1, 2026 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant