perf: filter resources before building dependency graph#524
Merged
michael-richey merged 4 commits intomainfrom Apr 14, 2026
Merged
perf: filter resources before building dependency graph#524michael-richey merged 4 commits intomainfrom
michael-richey merged 4 commits intomainfrom
Conversation
When syncing with filters (e.g., 5 dashboards out of 1000 in state), get_dependency_graph() was iterating ALL resources and deepcopying each via _resource_connections(). Now it checks the resource filter first and skips non-matching resources, avoiding 995 unnecessary deepcopy calls. Also strips phantom dependency references (nodes in dep sets but not graph keys) to prevent TopologicalSorter from yielding implicit nodes that waste worker cycles, and fixes the return type annotation from List to Set. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The blanket `dependency_graph[key] & graph_keys` stripped ALL deps not explicitly in the graph, including cross-type phantom deps (e.g., users → roles when --resources=users). Those phantom nodes are essential for TopologicalSorter to yield dependencies before dependents. Replace with targeted `dependency_graph[key] - filtered_out` that only removes deps pointing to resources explicitly excluded by --filter. Also removes 2 redundant tests and moves test_cross_type_phantom_deps_preserved to the GREEN section since it passes on main too. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Return filtered_count from get_dependency_graph() and set it on worker.counter so the "Filtered: N" log line is accurate for sync - Fix reset_counter() to also zero the filtered field (pre-existing bug) - Remove dead filter checks in _apply_resource_cb and _diffs_worker_cb (filtered resources are now excluded at the graph level) - Rename test_filtered_resources_emitted to test_filtered_resources_excluded_from_graph - Fix mock using both wraps= and return_value= (return_value wins, wraps was silently ignored) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
heyronhay
previously approved these changes
Apr 14, 2026
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
heyronhay
approved these changes
Apr 14, 2026
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
2 participants
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
get_dependency_graph()before calling_resource_connections(), skipping deepcopy and connection computation for non-matching resources (e.g., 995 of 1000 dashboards when syncing 5)TopologicalSorter. Cross-type phantom deps (e.g.,users → roleswhen--resources=users) are intentionally preserved —TopologicalSorteryields them first, ensuring dependencies are synced before dependents.filtered_countfromget_dependency_graph()so theFiltered: Nlog line remains accurate now that filtering happens at graph construction time_apply_resource_cband_diffs_worker_cb(now dead code since filtered resources never enter the graph)_dependency_graphtype annotation fromListtoSetand correct the assignment-vs-annotation bug on line 41Counter.reset_counter()not resetting thefilteredfieldPerformance example
Scenario: 1000 dashboards in state depending on 500 SLOs, syncing with
--resources=dashboards --filtermatching 5 dashboards (which depend on 8 unique SLOs).Before this change, all 500 SLOs were synced as phantom deps because every dashboard's SLO references entered the graph. After, only the 8 SLOs that the 5 filtered dashboards actually depend on become phantom nodes. The 492 unrelated SLOs are never touched.
Context
When syncing with filters (e.g., 5 dashboards out of 1000 in state),
get_dependency_graph()was iterating ALL resources in source state. For each, it called_resource_connections()which deepcopies the resource. All 1000 then entered the topological sorter and worker queue, where 995 were immediately filtered out at_apply_resource_cb. This change moves the filter check earlier so filtered resources never enter the graph.Builds on #518 which moved the filter check before deepcopy in
_apply_resource_cb.Test plan
test_dependency_graph.py(8 GREEN invariant tests + 6 RED behavioral tests)test_report_e2e.py::TestFilteredOutcome::test_filtered_resources_excluded_from_graphto reflect filtered resources no longer emitting "filtered" NDJSON events🤖 Generated with Claude Code