perf(computed-attribute): batch Python transform recompute - #10034
Conversation
Recompute a Python-transform computed attribute for a batch of nodes by initializing the transform's git repository once for the whole batch instead of once per node, and persisting the results in a single bulk write (chunked node.save) rather than a GraphQL mutation per node. A node whose recomputed value is unchanged is skipped, so it neither writes nor fans out a further recompute. A node whose transform raises or returns a non-string value is isolated: it is skipped with its previous value left in place, so one failing node no longer aborts the batch and drops the other nodes' writes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Records that recomputing a Python computed attribute fans out to every node of the kind today. This is a baseline the follow-up scoping work will flip to an affected-only count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
update_flow_run replaces the full tag list, and add_tags built that list from the prefect runtime snapshot, which is frozen at flow start. Any flow that called add_tags more than once silently lost the earlier call's tags: the bulk recompute dispatcher's database-change tag wiped the branch and node tags the process flow had just applied, making its runs invisible to the task API's branch-filtered queries. Read the current tags from the API before merging so successive calls compose instead of overwrite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This reverts commit de6664d.
…t submission The task API surfaces a flow run only when it carries the branch tag, and a tag update inside the flow rebuilds the tag list from the tags known at flow start: the bulk dispatcher's database-change tag update was rebuilding from the creation tags and dropping the branch tag the flow had added mid-run, hiding the recompute from branch-filtered task queries. Bake the branch tag into the submission itself so it is part of the creation tags and survives every later update. An earlier attempt fixed this inside add_tags by re-reading the run's tags from the API before merging; that doubled the Prefect API round-trips of every tagged flow and measurably slowed storm-sized recomputes, so it was reverted in favor of tagging at the two submission sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ummary log Cover the oversized-fan-out path directly: a fan-out beyond the submission limit splits into bounded submissions, every id submitted exactly once, and each submission carries the branch tag that branch-filtered task queries match on. The workflow recorder now records submission tags so adapters can assert them. The process flow ends with one aggregate line (submitted/written/skipped) so partial failure is greppable from the flow log alone until a proper recompute-failure surface exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d attributes Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…in async-tasks Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…he why Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
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 auto-approve. Performance optimization batching Python transform recompute with shared repo init and bulk write, tested at scale. The change is bounded and uses existing bulk writer machinery.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed
Shadow auto-approve: would not auto-approve because issues were found.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
… fixtures - carry the branch tag at creation on lifecycle-path submissions too - qualify the skip-unchanged doc claim to per-node effective changes - dedupe the transform dataset setup into a shared component helper - drop a review-facing comment on the summary log line Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 17 files (changes from recent commits).
Confidence score: 5/5
- In
dev/specs/004-batch-python-recompute/data-model.md, theAttributeValueWrite.valuetype omits thelist[str]case defined inbackend/infrahub/core/recompute/bulk_write.py, which could lead contributors to build against the wrong contract and introduce avoidable validation/serialization mismatches in HFI-related paths — align the spec type union with the implementation (str | list[str] | None).
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="dev/specs/004-batch-python-recompute/data-model.md">
<violation number="1" location="dev/specs/004-batch-python-recompute/data-model.md:13">
P3: AttributeValueWrite.value type in the table shows `str | None` but the referenced source (`backend/infrahub/core/recompute/bulk_write.py`) defines it as `str | list[str] | None`. The `list[str]` variant is used by HFID writes and this path only enqueues `str` values, so the constraint note is right — but the type column should match the source type for readers who cross-reference.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| |---|---|---| | ||
| | node_id | str | target node | | ||
| | field | str | computed attribute name | | ||
| | value | str \| None | recomputed value; only `str` reaches the writer on this path | |
There was a problem hiding this comment.
P3: AttributeValueWrite.value type in the table shows str | None but the referenced source (backend/infrahub/core/recompute/bulk_write.py) defines it as str | list[str] | None. The list[str] variant is used by HFID writes and this path only enqueues str values, so the constraint note is right — but the type column should match the source type for readers who cross-reference.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At dev/specs/004-batch-python-recompute/data-model.md, line 13:
<comment>AttributeValueWrite.value type in the table shows `str | None` but the referenced source (`backend/infrahub/core/recompute/bulk_write.py`) defines it as `str | list[str] | None`. The `list[str]` variant is used by HFID writes and this path only enqueues `str` values, so the constraint note is right — but the type column should match the source type for readers who cross-reference.</comment>
<file context>
@@ -0,0 +1,32 @@
+|---|---|---|
+| node_id | str | target node |
+| field | str | computed attribute name |
+| value | str \| None | recomputed value; only `str` reaches the writer on this path |
+
+Source: `backend/infrahub/core/recompute/bulk_write.py`. Validation: this feature only enqueues writes whose `value` is `str` (R4).
</file context>
| | value | str \| None | recomputed value; only `str` reaches the writer on this path | | |
| | value | str \| list[str] \| None | recomputed value (only `str` reaches the writer on this path) | |
ajtmccarty
left a comment
There was a problem hiding this comment.
looks good to me. @gmazoyer might want to look at it too
| results: list[tuple[str, AttributeValueWrite | Exception]] = [ | ||
| (oid, result) async for oid, result in batch.execute() | ||
| ] | ||
| writes, skipped = _partition_transform_results(results) |
There was a problem hiding this comment.
looks like _partition could handle logging the failures and only return the writes instead of splitting them and returning both.
There was a problem hiding this comment.
I would keep it returning both. The function is probably easier to unit-test as it stands. The flow also still needs len(skipped) for the summary line. So moving the logging inside would probably not help much.
| "context": context, | ||
| }, | ||
| # Must be a creation tag: in-flow tag updates drop tags added mid-run. | ||
| tags=[WorkflowTag.BRANCH.render(identifier=branch_name)], |
There was a problem hiding this comment.
do we need a task to cover the rest of the places where we add tag(s) mid-run?
At `large` dataset scale the computed-attribute recompute fan-out (query-computed-attribute-transform-targets + graphql-query-group-update + process_transform) can saturate Neo4j's Bolt connector thread pool, which defaults to 400, producing Neo.TransientError.Request.NoThreadsAvailable and taking the API servers down mid-run before the measured scenario is reached. Expose NEO4J_server_bolt_thread__pool__max__size in both testcontainers compose stacks via the new INFRAHUB_TESTING_DB_BOLT_THREAD_POOL_MAX_SIZE env var, sized alongside the existing heap/pagecache knobs. The default is 400 (Neo4j's own default), so normal test runs are unchanged; heavy dataset/perf runs raise it from the workflow environment. Providing an explicit default also avoids an empty value reaching an integer Neo4j setting when the var is unset. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 4 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
The wide computed-attribute recompute fan-out on large datasets exhausts the default 1024 file-descriptor limit on the task-worker (and, to a lesser degree, the API server and database), surfacing as `ConnectError: [Errno 24] Too many open files` and taking the stack down during warmup. Set nofile soft/hard to 1048576 on the database, infrahub-server, and task-worker services. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (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
| log.info( | ||
| f"Recompute of '{attribute_name}' complete: submitted={len(results)} " | ||
| f"written={len(writes)} skipped={len(skipped)}" | ||
| ) |
There was a problem hiding this comment.
written=len(writes) counts what we send to the writer, not what really got saved. The writer skips unchanged nodes, but dispatch() throws away its return value. So the number of skipped no-ops never shows up. The plan asked for written/unchanged/skipped, but that is not what we log. Maybe return the written list from dispatch() and log written and unchanged. Or just rename this to dispatched=.
| for skipped_id, reason in skipped: | ||
| log.warning(f"Skipping recompute of '{attribute_name}' for node {skipped_id}: {reason}") | ||
|
|
||
| dispatcher = await build_bulk_recompute_dispatcher(schema_branch=schema_branch) |
There was a problem hiding this comment.
schema_branch does not change in the loop. So you can build the dispatcher once, above the loop, like process_jinja2 does.
| results: list[tuple[str, AttributeValueWrite | Exception]] = [ | ||
| (oid, result) async for oid, result in batch.execute() | ||
| ] | ||
| writes, skipped = _partition_transform_results(results) |
There was a problem hiding this comment.
I would keep it returning both. The function is probably easier to unit-test as it stands. The flow also still needs len(skipped) for the summary line. So moving the logging inside would probably not help much.
| await add_tags(branches=[branch_name], nodes=all_ids) | ||
| if not all_ids: | ||
| return |
There was a problem hiding this comment.
The if not all_ids: return is after add_tags. So an empty batch still tags. Moving the guard up matches what T008 wanted.
| ulimits: | ||
| # Raise the file-descriptor ceiling so a wide recompute fan-out doesn't hit | ||
| # "[Errno 24] Too many open files" on the API server's outbound connections. | ||
| nofile: | ||
| soft: 1048576 | ||
| hard: 1048576 |
There was a problem hiding this comment.
@fatih-acar's input about this could be nice to have. I wonder if this needs to be documented somewhere for production environments too.
00ee47a to
e67ebee
Compare
Why
When a change touches data that Python-transform computed attributes read, every affected node was recomputed through its own full path: a git repository init per node, one GraphQL update mutation per node, and every mutation's events re-triggered target queries and further recompute dispatches. One device-type rename could keep an instance churning for minutes, and at scale it takes the instance down. This is the "merge leaves the instance unusable" pain reported by large deployments.
Goal: absorb a wide fan-out as one bounded pass per chunk, with the same final values and the same events for real changes.
Non-goals: scoping the fan-out itself (every node of the kind vs only the affected ones) and a recovery surface for failed nodes, both are follow-ups.
The improvements
The scenario for all of them is a Python computed attribute whose source data changes with many reader nodes, most visibly a branch merge or rebase, but the same path fires on a direct data change or a transform update that fans out to a whole kind.
InfrahubUpdateComputedAttributemutation, no per-node client round-trip.NodeUpdatedevents even when the recomputed value was identical to the stored one, and those events re-triggered thequery-computed-attribute-transform-targetsandgraphql-query-group-updateautomations, which dispatched further recomputes, a self-sustaining storm. Now a node whose save produces no effective change emits nothing and dispatches nothing, so a wave of recomputes that mostly lands on already-correct values simply stops.submitted/written/skippedsummary line.Plus one visibility fix: the branch tag is set on each submission at creation so the recompute runs stay visible in branch-filtered task views, tags added mid-run can be dropped by later in-flow tag updates.
How we know it is an improvement
Measured, not inferred. The
TestMergeRecomputePythonsuite in infrahub-private-tests restores the same dataset backup, renames a device type on a branch, merges, waits until every affected device is recomputed, and measures how long the instance keeps churning afterwards and how many flow runs the merge caused. We ran it twice ondevelop_x-small, once with develop (baseline run) and once with this branch (run with this PR), same dataset, same scenario, the only difference between the two runs is the commits in this PR.query-computed-attribute-transform-targetsrunsAt scale develop does not degrade gracefully, it collapses: on
develop_large, the same scenario on develop spawned 73,149 flow runs in 110 minutes (99% dispatch echo, 53k group updates plus 19k target queries, near-zero actual transform work) and died with Neo4j bolt-thread and file-descriptor exhaustion. The same scenario with this branch, on a deliberately smaller database because the shared CI hosts kept OOM-killing full-size setups, survived without taking the host down. The churn that remains there is the reverse-index bookkeeping from dispatching to every node of the kind, which this PR deliberately does not touch, so the at-scale data confirms fan-out scoping as the necessary follow-up rather than a nice-to-have.How we know nothing broke
NodeUpdatedevents for real changes, the publicInfrahubUpdateComputedAttributemutation stays (just no longer used internally), and the flow parameters are unchanged so the Prefect deployment contract is stable.Built on the Jinja2 work, and what Python transforms still cannot do
This deliberately reuses the bulk recompute machinery Guillaume built for the Jinja2 computed attributes (
BulkRecomputeDispatcher/BulkRecomputeWriter), no second write path, and it is verified with hisTestMergeRecomputePythonsuite from infrahub-private-tests #29.The part we cannot inherit: Jinja2 values are template renders the backend computes directly, so on merge they are recomputed in one coalesced in-process pass. Python transforms execute user code from a git repository on the task workers, they cannot join that pass, so they remain event-dispatched flows and a merge still dispatches a recompute for every node of the kind rather than only the changed ones. This PR makes that dispatch cheap and non-amplifying, scoping it to the affected nodes is the next big lever (see next steps).
Suggested review order
backend/infrahub/computed_attribute/tasks.py, theprocess_transformrewrite plus the tagged submission sites.add_tagscommit pair (de6664dedreverted by58680ba2e) documents a rejected fix, kept for history, the final approach tags at submission instead.The design artifacts (spec, plan, critique, tasks) live in
dev/specs/004-batch-python-recompute/.How to test
At-scale validation runs through infrahub-private-tests
test-dataset.ymlwithtest_filter=TestMergeRecomputePython, links above.Impact & rollout
Next steps
Handle failed/skipped nodes more gracefully, likely raise after the dispatch so the flow run shows failed while healthy siblings stay persisted:
A queryable recompute-failure surface with a one-click re-run (the
RecomputeComputedAttributemutation is the existing lever).Scope the merge fan-out to affected nodes instead of every node of the kind, that is the next big lever for giant-branch merges and the one thing the Jinja2 path also still needs.
Phase-level spans/counters (repo-init, read, execute, write) to restore the per-node observability the batch path removed.
Checklist
dev/knowledge/backend/computed-attributes.md)🤖 Generated with Claude Code