refactor(core)!: make an operation a component - #363
Merged
Merged
Conversation
`Operation` declared, under `if TYPE_CHECKING`, attributes it does not own (`id`, `kind`, `key`, `relations`, `materializable`, `source`, `partitioning`, `to_spec`, `bound`) and supplied runtime stand-ins for four of them. That block was never a protocol: it was a promise that `Operation` would only ever be mixed into a `Component`, written in the form the type checker accepts. Both implementors already kept it (`Asset`, and `Connection` through `Resource`), and the platform depends on it: events, the executions view and the failed-scope retry walk are all keyed by component row id. `Operation` is now `Component, Workload` and `Asset` a plain subclass of it, so those attributes are inherited and real. `materializable` moves up as a hidden field, and `qualified_key` goes: it returned the bare key as a stand-in for the qualified form `Component` already builds, which the old base order hid and the new one would have let win, silently unqualifying every asset key. Kinds become explicit where derivation no longer reaches: `Operation` declares an empty kind to opt out, and `Asset` declares its own now that `Component` is not one of its direct bases. Guarding derivation on abstractness instead would be wrong, because `Destination` is abstract and is itself a kind. By Digitl
Two specs and four plans for making failed work heal on its own. The diagnosis is that the execution hierarchy (batch, run, operation, request) equips its levels inconsistently: retry exists at one level and by hand, contention is bounded by an anonymous per-process number that cannot name the resource it protects, and hooks observe attempts rather than verdicts. The retry spec covers attempts and verdicts; capacity and request-level retry are named as the two sibling specs rather than folded in. The prerequisite spec is the refactor this commit's parent implements. By Digitl
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
`Asset.source` is `cast("Source | None", self.parent)`, so a `source` on
`Operation` was a second name for `Component.parent`, living on a contract
where it is false for every implementor but one, returning a `None` that is a
null object rather than an answer. The same stand-in shape as the
`qualified_key` its parent commit removed.
Neither generic reader needed source-ness. `DAG.to_spec` reads it to decide
what to serialize, which is the ownership rule `Component` already defines, and
`RunState` stamps the owner's id into event metadata. Both read `parent` now.
`Asset.source` stays: a typed accessor narrowing `parent` to `Source` on the
one class where that holds, and part of the authoring surface, since
`@il.asset` injects `self.source` into `data()`.
The event key stays `source_id`, correct while assets are the only owned
operations; renaming it to `parent_id` touches the event stream telemetry reads
and is recorded as its own change.
By Digitl
`source_id` named a guarantee the model does not make: the key carries the id of the component that owns the one an event is about, and only assets happen to be owned by a source today. It is now `parent_id`, and the telemetry attribute `interloper.source.id` is `interloper.parent.id`. All three producers move together (node lifecycle metadata, asset-level metadata, the component log emitter). Renaming only the generic one would have left two keys for the same value, which is worse than a loose name. The same line divides the plumbing as divides `Asset.source` from `Operation.parent`: `EventLogger` is component-generic and takes `parent_id`, while `ExecutionContext` is asset-specific and keeps `source_id`. Events written before this keep the old key. They are history and are not rewritten, so a consumer reading across the boundary sees both, and any saved telemetry query on `interloper.source.id` needs updating. By Digitl
`materializable` named an asset concept (computed and written to a
destination) on a flag the whole graph reads: the DAG's generations and
satisfied edges, `RunState`'s skipped-versus-queued, the runner's partition
preflight, `Source.select`, and the failed-scope retry, which marks any
operation that already succeeded, a connection renewal included. For a
connection renewal there is nothing to materialize.
`enabled` is the word the framework already uses for "this will run", on
`Job` ("Job will run on the configured schedule") and `Hook` ("Hook will fire
on matching events"). `Operation` is the third: a disabled operation stays in
the graph, ordering whatever depends on it, and does not execute. `Asset` adds
the half only an asset has, that a disabled one is read-only and its
downstreams read its data from its destination.
The rename is user-visible: run manifests, the `source(enabled=False)` copy
keyword, the docs, the examples, the app helper and the agent tool all move
with it.
Two defects go with it. The field carried `json_schema_extra={"x-hidden":
True}`, which is the wrong mechanism (config fields are hidden through
`internal_fields`, applied by `strip_internal_fields`; `x-hidden` is a state
convention the app reads for state columns) and the wrong intent, since the
app and the agent treat this flag as user-facing. And the pydantic
shadowing-warning filter in `asset/base.py` was dead once `Asset` stopped
redeclaring the field over `Operation`'s plain default.
By Digitl
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
1 participant
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.
What
OperationbecomesComponent, Workload, andAsseta plain subclass of it. Theif TYPE_CHECKINGblock and the runtime stand-ins it backed are deleted, sooperation/base.pyloses 40 lines net.Plus the design docs for the retry system this refactor unblocks: two specs and four implementation plans under
docs/superpowers/. No retry code lands here.Why
Operationdeclared, underif TYPE_CHECKING, attributes it does not own:id,kind,key,relations,materializable,source,partitioning,to_spec()andbound(), with runtime stand-ins for four of them. That was never a protocol. It was a promise thatOperationwould only ever be mixed into aComponent, written in the formtyaccepts, and every implementor already kept it:Asset(Component, Operation)Connection(Resource, Operation), whereResource(BaseSettings, Component)The platform already depends on it too: events carry
component_id/component_kind/component_keyper operation, theexecutionsview is keyed by(run_id, component_id), andRunExecutor._prior_successesmatches by component row id. There is no non-component operation anywhere in the system, so the indirection bought a generality nothing uses and cost a block of attributes that drift.A
typing.Protocolwould not have fixed it: it describes what the DAG and runner read, carries neither defaults nor behaviour, and would have leftOperationwith its ABC and the same stand-ins.For the reviewer
Three things worth a closer look, each forced by a failing test rather than chosen:
Operation.qualified_keyis deleted, and this is the refactor justifying itself. It returned the barekeyas a stand-in for the qualifiedsource.assetform thatComponent.qualified_keyalready builds from a component's identity. Under the old base orderComponent's won; underAsset(Operation)the stand-in wins and silently unqualifies every asset key.Connection, having no owner, reads identically either way.Kinds are explicit where derivation no longer reaches.
kindis auto-derived only for a direct child ofComponentthat declares none, soOperationdeclares an empty kind to opt out andAssetdeclares its own. Guarding the derivation oninspect.isabstractinstead was tried and is wrong:Destinationis abstract and is a kind, so that unregistersdestinationand breaks every relation declared against it (9 tests).Operation.sourceis a property returningNone, not a class attribute. Pydantic rejects a bare un-annotated attribute, and aClassVarthatAssetoverrides with a property is an invalid override underty. The default has to stay because both readers (dag/base.py:466,runner/state.py:368) run against any node, including aConnectionthat no source owns. CollapsingsourceintoComponent.parentis recorded as a follow-up in the spec.The
BaseSettings/BaseModeldiamond the spec flagged as the main risk linearizes cleanly:Connection, Resource, BaseSettings, Operation, Component, Serializable, ....Four test fixtures needed updating, each because it leaned on something that has legitimately gone: two declared
(il.Component, il.Operation), which is now an inconsistent MRO; one monkeypatchedmaterializableas a class attribute that only existed as the stand-in; andFakePlumbingderived its kind from being a directComponentchild.Breaking
Assetno longer listsComponentas a direct base, and any class declaring(Component, Operation)must dropComponent. AnOperationsubclass that introduces a new kind must now declare it explicitly.Verification
uv run ruff check,uv run ty checkanduv run pytestall pass: 2855 passed, 3 skipped.By Digitl