Add the Python transform target deriver by gmazoyer · Pull Request #10416 · opsmill/infrahub

gmazoyer · 2026-08-26T09:20:00Z

Why

A merge or a rebase recomputes Jinja2 computed attributes, display labels and human-friendly ids
through one coalesced pass. Python transform computed attributes are excluded from it, so their
automations start one task per changed node: 68 tasks against 3 coalesced ones on a site merge,
148 on a rebase.

This is the first of three PRs (IFC-3017). It adds
the missing piece and nothing else. IFC-3018 then runs the coalesced Python pass alongside the
per-node automations, and IFC-3019 gates those automations on origin == live, which is what
removes the fan-out.

What changed

Nothing calls the resolver yet, so behaviour does not change.

A resolver that maps a change signature (kind, action, changed fields) to the affected Python
(kind, attribute) pairs and to the nodes to recompute for each. Read sets come from the
transform GraphQL queries, readers from the query-group subscriber index.
A whole_kind flag on the affected target. A widened target carries no node ids, and chunking an
empty id set produces no submission, so without the flag the fallback becomes a silent skip.
The setting INFRAHUB_COALESCE_PYTHON_RECOMPUTE_AFTER_MERGE, on by default so the scenario
harness in the private test repository takes the coalesced path with no change to its CI setup.
Expect the switch to go away once the three PRs are in and the measured counts prove the pass
works.

Chain plumbing on process_transform stays out of scope, it belongs to IFC-3018, so the Python
family carries no recompute_depth yet.

How to review

Is the narrowing correct, and does it fail safe when the reader lookup fails? Over recompute is
acceptable, under recompute is not. Start with python_target_resolution.py and its unit tests.
Two calls worth a look: reader lookups are memoised per set of changed ids rather than shared
across every attribute, and a deleted node has no query-group membership left on the destination
branch, so its readers have to arrive as updated nodes with a changed relationship field.

The changes to the generated files all come from the new setting.

How to test

uv run pytest backend/tests/unit/core/merge -q
INFRAHUB_USE_TEST_CONTAINERS=1 uv run pytest backend/tests/component/merge_recompute_coalescing -q
INFRAHUB_USE_TEST_CONTAINERS=1 uv run pytest backend/tests/component/computed_attribute -q

Green locally. The two component suites are there to show that behaviour is unchanged.

Checklist

Tests added/updated
Changelog entry: one fragment covers the whole stack, on IFC-3019
I have reviewed AI generated content

A merge or a rebase recomputes Jinja2 computed attributes, display labels and human-friendly ids through one coalesced pass. Python transform computed attributes are excluded from it, so their automations start one task per changed node. Add the resolver that pass is missing. It maps a change signature (kind, action, changed fields) to the affected Python attributes and to the nodes to recompute for each: change signature -> read-set index what each transform query reads -> subscriber index which nodes read the changed nodes -> one target per (kind, attribute) Nothing calls the resolver yet, so behaviour does not change. The new switch INFRAHUB_COALESCE_PYTHON_RECOMPUTE_AFTER_MERGE is on by default. It has no effect until a later change wires the resolver into the coalesced pass. The default is on so that the scenario harness in the private test repository takes the coalesced path without a change to its CI environment. The rules follow the coalesced builder. A created node is its own target. An update selects the readers of the changed fields. A deletion selects the readers too. Imprecision is held per kind, so a query that reads a derived field of one kind still rejects an unread field of another kind. Over-recompute is acceptable, under-recompute is not. An undeterminable read set and a failed reader lookup both widen to the whole target kind. The new whole_kind flag carries that case to the submission planner; without it the widening becomes a silent skip, because chunking an empty set of ids produces no submission.

Say where the verdict comes from and what it ties the test to, in one sentence instead of two paragraphs.

codspeed-hq · 2026-08-26T09:31:55Z

Merging this PR will not alter performance

✅ 13 untouched benchmarks

_{Comparing gma-20260824-ifc-3017 (595df1a) with develop (5219d6d)}

cubic-dev-ai

1 issue found across 14 files

Confidence score: 5/5

In backend/tests/adapters/python_target_sources.py, RecordingSubscriberSource drops the branch argument, so tests do not verify that the resolver passes the intended branch to subscriber sources; record and assert the branch to close this coverage gap.

Prompt for AI agents (unresolved issues)


Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/tests/adapters/python_target_sources.py">

<violation number="1" location="backend/tests/adapters/python_target_sources.py:33">
P3: RecordingSubscriberSource drops the branch argument, so the resolver's branch-passing to the subscriber source is never masked or asserted, unlike StaticPythonReadSetSource which records and checks the branch. Record the branch in calls so a regression that passes the wrong branch to the subscriber lookup is caught.</violation>
</file>

_{Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic}

cubic-dev-ai · 2026-08-26T09:35:23Z

+        self.calls: list[tuple[str, ...]] = []
+
+    async def subscribers(self, *, node_ids: list[str], branch: str) -> list[SubscriberRef]:
+        self.calls.append(tuple(node_ids))


P3: RecordingSubscriberSource drops the branch argument, so the resolver's branch-passing to the subscriber source is never masked or asserted, unlike StaticPythonReadSetSource which records and checks the branch. Record the branch in calls so a regression that passes the wrong branch to the subscriber lookup is caught.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/adapters/python_target_sources.py, line 33: <comment>RecordingSubscriberSource drops the branch argument, so the resolver's branch-passing to the subscriber source is never masked or asserted, unlike StaticPythonReadSetSource which records and checks the branch. Record the branch in calls so a regression that passes the wrong branch to the subscriber lookup is caught.</comment> <file context> @@ -0,0 +1,49 @@ + self.calls: list[tuple[str, ...]] = [] + + async def subscribers(self, *, node_ids: list[str], branch: str) -> list[SubscriberRef]: + self.calls.append(tuple(node_ids)) + return [ + SubscriberRef(id=subscriber_id, kind=kind) </file context>

The resolver put updated and deleted ids in one subscriber lookup. A deleted node id empties that lookup, so the readers of the live changes went with it. Resolve deleted ids on their own. An updated node of the target kind was only a source, never a target of its own. The reverse lookup reaches it through the query group its last compute subscribed it to, so a node that never computed stayed stale. The live path has no such dependency, so this closed a gap it would have opened. Neither hole was visible to the tests. The subscriber double now reproduces the empty lookup a deleted id causes. Correct two docstrings that promised more than the code does: no process flow acts on the whole-kind flag yet, and the read-set index raises rather than degrades when a transform peer is missing.

cubic-dev-ai

0 issues found across 5 files (changes from recent commits).

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.

Re-trigger cubic}

Name the static source's attribute after what it holds, and assert the precision of each target rather than a predicate over the collection.

cubic-dev-ai

0 issues found across 2 files (changes from recent commits).

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.

Re-trigger cubic}

A widened target went to the per-id transform flow with an empty id list, and that flow returns early when it has no ids. Send it to the fan-out flow instead, which resolves every node of the kind itself. The read-set gather resolves its repository and query peers strictly, so one missing peer took the whole pass down. Catch it and fall back to an imprecise read set for every attribute the schema declares, so the resolver widens rather than skipping everything.

cubic-dev-ai

0 issues found across 3 files (changes from recent commits).

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic}

polmichel

The test suite looks exhaustive and has interesting helpers and fakes. I just posted comments regarding code maintainability, nothing from correctness perspective.
I am obviously open to discuss those as some of them may be subjective.
This PR certainly helped me regarding selective artifacts/generators regeneration, I've open two items under my Epic.

Thanks for the Confluence document which was pretty useful to give an overview of the feature. This has helped the review

polmichel · 2026-08-27T09:41:51Z

+class PythonSubscriberSource(Protocol):
+    """The nodes subscribed to a query group that holds any of ``node_ids`` as a member."""
+
+    async def subscribers(self, *, node_ids: list[str], branch: str) -> list[SubscriberRef]: ...


Regarding the work I've done on generator part, I'll replace my ProposedChangeSubscriber by your object as this looks pure duplication
https://opsmill.atlassian.net/browse/IFC-3070

polmichel · 2026-08-27T09:43:01Z

+
+def derived_reads_are_scopable(*, schema_branch: SchemaBranch, kind: str, read_fields: frozenset[str]) -> bool:
+    """Whether every derived field read on one kind can be held against that kind alone."""
+    derived_reads = read_fields & IMPRECISE_READ_FIELDS


I've not captured this imprecise field concept inside the code I've done regarding generators and artifacts, I'll see if I need this concept in this release https://opsmill.atlassian.net/browse/IFC-3071

polmichel · 2026-08-27T11:06:38Z

+        """
+        ids_by_signature: dict[ChangeSignature, set[str]] = {}
+        for change in changes:
+            signature = ChangeSignature(kind=change.kind, action=change.action, changed_fields=change.changed_fields)


It looks like a derivative of a MergeChange object, do you think that would be a good idea to create a property inside MergeChange returning this object?

Moreover this code looks duplicated over here infrahub.core.merge.recompute_coalescing.CoalescedRecomputeBuilder.build wdyt about factorising it somewhere?

Both done. MergeChange has a signature property now, and one group_ids_by_signature helper does the grouping for the builder and the resolver. Good catch, it was copy paste.

polmichel · 2026-08-27T11:28:38Z

+@dataclass(frozen=True)
+class _Selection:
+    """Why one change signature selects one attribute, and how exactly."""
+
+    self_target: bool
+    widen: bool
+    precise: bool


Optional comment, maybe it'd be worth to define what these attributes - and the future reader_lookup/self_ids - mean inside the docstring

I would rather fix this with your other suggestion. If _Selection becomes a union then each case has a name and the fields explain themselves. If we keep the booleans I will document them instead.

polmichel · 2026-08-27T11:35:30Z

+def _select(*, signature: ChangeSignature, attribute: PythonAttributeReadSet) -> _Selection | None:
+    """Decide whether one change signature affects one attribute, or return None when it cannot.
+
+    Raises:
+        ValueError: on a change action the narrowing has no rule for, since guessing one would risk
+            leaving a value stale.
+
+    """
+    if signature.action == CREATED:
+        # A created node subscribes to no query group yet, so it can only be its own target.
+        return _Selection(self_target=True, widen=False, precise=True) if attribute.kind == signature.kind else None
+
+    if signature.action not in {UPDATED, DELETED}:
+        raise ValueError(f"Unknown change action: {signature.action!r}")
+
+    return _select_reader(signature=signature, read_set=attribute.read_set)


Could signature.action be a StrEnum instead of a raw string? It would allow a match case/assert_never structure here. Wdyt from maintainbility perspective?

Agree. MutationAction already exists, so it is mostly about typing MergeChange.action with it. It is not only my code though, post_merge and the rebase flow build those changes too, and the Jinja2 builder reads the same field. I prefer a follow up ticket so this PR stays about the deriver.

polmichel · 2026-08-27T12:14:15Z

+async def test_a_failing_reader_lookup_widens_instead_of_skipping() -> None:
+    subscribers = FailingSubscriberSource()
+    resolver = _resolver(read_sets=[SUMMARY], subscriber_source=subscribers)
+
+    targets = await resolver.resolve(
+        changes=[MergeChange(node_id="s1", kind=SITE, action="updated", changed_fields=frozenset({"name"}))]
+    )
+
+    assert subscribers.calls == [("s1",)]
+    assert _identities(targets) == [(DEVICE, "summary")]
+    assert targets[0].whole_kind is True
+    assert targets[0].precise is False
+    assert targets[0].reader_lookups == frozenset()


Nice test, this is key 👍

polmichel · 2026-08-27T12:18:14Z

+
+    assert _identities(targets) == [(DEVICE, "label"), (DEVICE, "summary")]
+    for target in targets:
+        assert target.family == PYTHON_COMPUTED_ATTRIBUTE


RecomputeFamily is a Literal[str] but since there are constant re-referencing its values, I think it looks like a StrEnum. Wdyt?

Same idea as the one on the action field, and I agree. It would drop the four constants. It touches the 3 other families and their tests, so I would put it in the same follow up as MergeChange.action.

polmichel · 2026-08-27T12:21:07Z

+    assert _ids(by_identity[DEVICE, "summary"]) == frozenset({"d1", "d2"})
+    assert by_identity[DEVICE, "summary"].precise is True
+    # The site is read through a derived field, so the site change is a deliberate over-selection.
+    assert _ids(by_identity[ROUTER, "tag"]) == frozenset({"r1"})
+    assert by_identity[ROUTER, "tag"].precise is False


Nitpick comment which can be ignored, encapsulate by_identity[DEVICE, "summary"] and the second one which looks alike in a local variable could help from a readability standpoint

Done. I did the same in the unscoped update test just below, it had the same repetition.

polmichel · 2026-08-27T12:57:03Z

+
+    A node that never computed is in no group, so relying on the lookup alone leaves it stale.
+    """
+    subscribers = RecordingSubscriberSource(subscribers={})


The key is just there right? Maybe it would worth a comment to highlight this.

Yes, the empty map is the whole point. I added one line saying the node is in no query group.

polmichel · 2026-08-27T13:01:05Z

+
+    assert _identities(targets) == [(DEVICE, "summary")]
+    assert _ids(targets[0]) == frozenset({"d1"})
+    assert targets[0].precise is True


Maybe it'd be worth to also assert other booleans since they can all be modified and set?

Added whole_kind is False. The id assert already fails if a deletion widens, so this one is mostly saying it out loud but better to be explicit.

Both the builder and the Python resolver rebuilt the same signature and grouped the ids the same way. A change knows its own signature now, and one helper does the grouping for both. Name the two targets the readers test asserts on, mark why the subscriber map of the self-target test is empty, and pin that a deletion does not widen.

cubic-dev-ai

0 issues found across 3 files (changes from recent commits).

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.

Re-trigger cubic}

gmazoyer added 2 commits August 25, 2026 15:15

Condense the automation-coverage helper docstring

75ce5ad

Say where the verdict comes from and what it ties the test to, in one sentence instead of two paragraphs.

github-actions Bot added type/documentation Improvements or additions to documentation group/backend Issue related to the backend (API Server, Git Agent) group/frontend Issue related to the frontend (React) labels Aug 26, 2026

cubic-dev-ai Bot reviewed Aug 26, 2026

View reviewed changes

Tighten two test details

3d9f790

Name the static source's attribute after what it holds, and assert the precision of each target rather than a predicate over the collection.

cubic-dev-ai Bot reviewed Aug 26, 2026

View reviewed changes

gmazoyer force-pushed the gma-20260824-ifc-3017 branch from f1c7f0d to 160ebde Compare August 26, 2026 19:37

gmazoyer marked this pull request as ready for review August 26, 2026 20:02

gmazoyer requested review from a team as code owners August 26, 2026 20:02

polmichel reviewed Aug 27, 2026

View reviewed changes

cubic-dev-ai Bot reviewed Aug 27, 2026

View reviewed changes

gmazoyer requested a review from polmichel August 27, 2026 17:11

Uh oh!

Conversation

gmazoyer commented Aug 26, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

How to review

How to test

Checklist

Uh oh!

codspeed-hq Bot commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot Aug 26, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

polmichel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gmazoyer commented Aug 26, 2026 •

edited by cubic-dev-ai Bot

Loading

codspeed-hq Bot commented Aug 26, 2026 •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading