feat!: one Relation primitive for every component link - #321
Open
aaaaahaaaaa wants to merge 30 commits into
Open
feat!: one Relation primitive for every component link#321aaaaahaaaaa wants to merge 30 commits into
aaaaahaaaaa wants to merge 30 commits into
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…field to upstreams The relation type dependency becomes upstream (a role name, like target and watch) and Asset.dependencies becomes Asset.upstreams, across core, store, toolkit, scheduler, app, docs and specs. Migration 017 renames persisted rows and narrows the per-slot unique index to resources. By Digitl
… flags The slot a class declares and the slot the catalog publishes are one class. The catalog JSON carries optional (inverted from required) and many. By Digitl
…a source wildcard By Digitl
depends_on holds asset keys and il.Dependency values (optional, many) and replaces requires and optional_requires; upstreams is always a list. Every reader goes through Asset.declared_upstreams(), and sibling wiring through Asset.sibling_upstreams() in both the source and the store. By Digitl
… contract at build By Digitl
…as None By Digitl
…ead of raising KeyError By Digitl
…n of declared keys By Digitl
… name the ambiguity error By Digitl
The whole-branch review flagged that validate_upstreams enforced non-optional presence and identity but never cardinality: a single-valued slot (many=False) silently accepted several wired upstreams and handed data() whichever list _read_upstreams happened to read last. Raise DependencyContractError when a single-valued slot has more than one wired upstream present in the DAG, and pin the missing-data case for a non-optional single slot (Upstream(asset, data=None) still flows through when the upstream never materialized for that partition). By Digitl
… names The review found the guide and docstrings over-generalized "no data for the partition" to None, when only a destination with nothing materialized for that scope (no table or object at all) produces it; an existing but empty scope returns whatever the destination gives back for an empty read. Reword guide/dependencies.md, Upstream.data, and _read_upstreams with that precision, and sweep stale relation/dependency naming left over from the upstream-relation rename: relations.py and hydration.py module docstrings, DependencyContractError's docstring, _destination_read's summary, the reference/errors.md table (new DAGError/DependencyNotFoundError/AssetError/ DependencyContractError cases), and two clarifying lines on DAG._resolve_declared about reused instances and empty-list wiring. By Digitl
…nd descriptor By Digitl
…ion map By Digitl
…Component By Digitl
…d trickle By Digitl
…ances and Upstream legs By Digitl
…he run join read-only By Digitl
…se as references By Digitl
…il.Upstream By Digitl
… spec references By Digitl
…tired names By Digitl
…e relations repoint Attribute assignment wrote _bound directly, so it bound without cascading: setting a source's destinations left its assets with none and a run wrote nothing. Both write paths now land in Component._replace_binding, which checks, deduplicates, refuses to empty a non-optional relation, and calls the new _rebound hook that Source and Job override to trickle. A second bind on a single-valued relation now replaces its target instead of demanding an unbind. By Digitl
…ion kwargs; reject uncollected Component annotations A deep copy rebuilds a source's own bindings and its assets' separately, so the copy's assets hold distinct objects carrying the same ids: comparing by identity read every trickled binding on a copy as the asset's own, and a chained copy never repointed its assets. Compare target ids instead. Relation now forbids extra kwargs, so a misspelled flag is a validation error rather than a silently dropped declaration. And an annotation naming a component class that pydantic kept as a plain field, or that resolves nowhere, is refused at class definition instead of leaving the component with no relation, no trickle and no reference. By Digitl
… name replaces slot A provider naming a relation declared by kind and key (no class to read the method off) crashed with an AttributeError while formatting its own error. Both that case and an undeclared relation now raise the intended TypeError. The module's vocabulary follows the relation model: the provider form is "<name>.<method>", and the prose and locals say name rather than slot. By Digitl
aaaaahaaaaa
force-pushed
the
feat/many-upstreams-core
branch
from
September 7, 2026 23:40
00b023b to
9f1c2b8
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.
Summary
This PR replaces every link between components (destinations, connections, upstream assets, job
targets, hook watches) with one primitive:
Relation. Today those links are five separatemechanisms with five separate names (
relation_types,RelationDefinition,resource_types,ResourceRef,depends_on,Dependency,RelationSlot), each with its own selection rule,fallback behaviour and serialisation quirks.
Relationcollapses all of them into one class,declared the same way on every kind (an annotation, a typed or bare
il.Relationattribute, or thedecorator's
relations=), selected the same way (an exact key,source.asset,*.asset, or a listof keys), and read the same way (bound value if wired, else a fallback resolved at read time, never
at bind time).
The point of unifying this is the north star the whole redesign serves: a campaign matcher asset
that fans in every
campaignsasset an organisation has configured, with the set of upstreamsdecided by wiring at the instance level, not hardcoded into the class. Assets get one addition on
top of the shared model:
data()receivesil.Upstream(orlist[il.Upstream]) for assetrelations instead of the bound component, since an asset's data is what downstream assets actually
need, not the component itself. This PR is core (
interloper-core,interloper-assetsconnectors), plus the
interloper-appandinterloper-dbfiles the earlierdependency->upstreamrename touched; the platform, matcher and app follow as stacked PRs on top of it.Decisions
Relationclass declares every link, on every kind. It replacesrelation_types,RelationDefinition,resource_types,ResourceRef,depends_on,DependencyandRelationSlot.source.asset,*.asset, or a list of keys. A class in a declaration is shorthand for its key.default=first, otherwise the declared class when it is aResource(settings may come from the environment, so it is constructed at read time and a validation error surfaces then, as today) or when all its fields are defaulted. An unbound non-optional relation with no fallback is a build error.data=None, for every relation.on_deleteis declared per relation (blockordetach). Unbinding is derived: refused when it would empty a non-optional relation, detach otherwise.on_unbindis dropped.connection: Conn), a typedil.Relationattribute (destinations: list[Destination] = il.Relation("destination", many=True, optional=True), the annotation is for the type checker), a bareil.Relationattribute, or@il.asset(relations={...}). Explicit beats annotation; a subclass replaces its parent's declaration of the same name.Relationis its own descriptor (class access gives theRelation, instance access the bound value); aTYPE_CHECKING-only__new__returningAnymakes the typed form check under ty.data()receives the bound instance for resource relations andil.Upstream(single) orlist[il.Upstream](many) for asset relations.to_spec()output. A target that has a parent is emitted under its parent and referenced as{ref: id}everywhere else. A target without a parent is inline at the first relation that reaches it and referenced afterwards. No flag, no kind knowledge.parent_id(assets under sources).parentmoves ontoComponentso the serialisation rule is generic. To revisit once this lands (containment as an owned relation).component_relations(src_id, name, dst_id, ...), primary key(src_id, name, dst_id).typeandslotcollapse intoname.Execution rulings, finalized during implementation
A few points the design left open were settled while building this PR:
Relationis its own descriptor and also carries aTYPE_CHECKING-only__new__returningAny, so the typed attribute form (x: list[Destination] = Relation(...))type-checks under
tyas ifxwere assigned the target type directly.Relation.__set__ownsrebinding, and
Component.__setattr__routes relation-name assignment to it, since pydantic's own__setattr__only special-cases properties.Resource(BaseSettings) target is always self-filling. Its fallback constructsat read time and a
ValidationErrorsurfaces then if a required, env-backed field is missing; anunbound connection is no longer a build-time error, matching today's behaviour.
validate_relationsat construction time skips an unbound,non-optional asset relation whose key is not source-local (cross-source or wildcard keys can only
be filled by the DAG, not by the source building its own assets). The DAG's own
_check_relationsstill validates every live node once resolution has run.non-transitive pass: it is read, not run, so its own upstreams are not pulled in behind it.
on_unbindhook: refused whenit would empty a non-optional relation, a plain detach otherwise.
What changes for connector authors
Before:
After:
The connection moves from a decorator kwarg to a plain annotation on the source; every asset that
needs it gets it trickled down automatically. The upstream moves from
depends_on={...}with anexplicit
il.Dependencyto a bareil.Upstream-annotated parameter: the parameter's own name is thebare key, so
campaignsoncampaigns_statsresolves to the siblingcampaignsasset with nofurther declaration. Cross-source or wildcard upstreams, and any relation that needs a non-default
key, still go through
relations={...}on the decorator, unchanged in spirit fromdepends_on=.All 24 connectors in
interloper-assetshave been moved onto this form.Stack
This PR is phase 1 (core:
interloper-core,interloper-assetsconnectors). Phase 2(platform:
interloper-db,interloper-api,interloper-toolkit,interloper-agent,interloper-scheduler) is stacked on top of this branch as its own PR, followed by phase 3(matcher) and phase 4 (app), each stacked on the one before.
It is not, however, strictly core-only: the branch also carries the earlier
dependency->upstreamrename commits, which touchinterloper-app's catalog and component types andinterloper-db(migration 017 in the oldtype/slotshape, plus the relation store and itstests). Phases 2 and 4 rewrite both of those, so what lands here is an intermediate state of
files those phases own, not their final shape.
Until phase 2 merges, the platform packages' test suites are red against this core: they still
reference the retired names (
RelationDefinition,Dependency,resource_types,.upstreams,.slots) that phase 1 removes. Alone, this branch leaves 37 platform tests failing(
interloper-api4,interloper-scheduler33) and 3interloper-dbtest modulesuncollectable. This is expected and owned by phase 2, not a regression to chase down here.
Because of that, this PR must merge together with, or immediately before, phase 2 so
mainis never left on a red platform for longer than the merge itself takes.
Verification
Counts below are after the review fix wave (one write path for bindings, id-based trickle
detection on copies, forbidden unknown
Relationkwargs, refused uncollected componentannotations, fetch-provider validation on kind-declared relations).
uv run --frozen ruff check: all checks passed. The pre-existing import-order issue ininterloper-db/src/interloper_db/store/relations.pyintroduced by this branch's commit77598513 (an identity rename) has been fixed with
ruff check --fix.uv run --frozen ty check: 18 diagnostics repo-wide, all in phase-2-owned packages and allexpected (old relation names not yet ported):
interloper-db15,interloper-slack2,interloper-scheduler1. Zero ininterloper-core,interloper-assets,interloper-google-cloud,interloper-pandas,interloper-docker,interloper-k8s.uv run --frozen pytest packages/interloper-core packages/interloper-assets packages/interloper-google-cloud packages/interloper-pandas packages/interloper-docker packages/interloper-k8s -q:1734 passed.
lands): 37 failures and 3 uncollectable modules in total.
interloper-db: 3 collection errors (AttributeErroron retired names intest_components.py,test_hydration.py,test_relations.py).interloper-api: 4 failed, 380 passed.interloper-toolkit: 10 passed (no coverage of the retired surface yet).interloper-agent: 11 passed (no coverage of the retired surface yet).interloper-scheduler: 33 failed, 78 passed.resource_types,relation_types,RelationDefinition,Dependency,ResourceRef,depends_on,RelationSlot,AssetIdentity, and related old names) overinterloper-core,interloper-assets,interloper-google-cloud,interloper-pandas,interloper-docker,interloper-k8s,docs,plugins,examplesyields 12 hits across fourcategories, all deliberate:
tests/component/test_base.py:845): asserts the old names are gone from the public API.tests/resource/test_fields.py:98-99,109andresource/fields.py:365): explain the current design by contrasting it with the retired one.plugins/interloper/skills/interloper-upgrade/SKILL.md:65,73-75,81): legitimate reference in the "Old" column of the 0.7x migration table.examples/telemetry/docker-compose.yml:16,61): false positive on Docker's owndepends_on:key.interloper-core/interloper-assetssource and the guide/extending/reference/uidocs: all em-dashes in files this branch touched were rewritten; the remaining hits are all in
files this branch never touched (pre-existing house style elsewhere in the codebase, left alone).
Follow-ups
rpartition; the semantics are unstated and untested.Relation.__new__returningAnymeanstyno longer checksRelation(...)call sites, and anon-optional relation annotated with the bare target type reads as always-present to
tyeventhough it can be unbound at runtime.
tests/source/test_base.py'sFinance.Revenuedocstring rationale foroptional=Trueis stale;tests/dagmixesMatcherandFakeMatcherfixture naming inconsistently.itself.
By Digitl