fix: replace greedy consolidation clustering with average-link (UPGMA) - #261
Merged
Conversation
jagoff
force-pushed
the
worktree-agent-a64307fdf1547bec2
branch
from
August 17, 2026 00:59
2f2ba58 to
3c7fd54
Compare
_greedy_cluster compared each new memory only to each existing cluster's FIRST member (frozen forever as its "representative"), never to members added afterwards — so two above-threshold near-duplicates could land in different clusters purely because of pull order. Measured on the live corpus: 38.4% of above-threshold pairs (861/1450) split this way. Single-linkage (connected components of the threshold graph) was tried and rejected as the fix: it transitively chains anything reachable through a path of individually-strong pairs — on the live corpus one (project, type) bucket alone chained 157 of its 950 memories into one unmergeable blob. _cluster_within_scope now clusters via average-link (UPGMA) agglomerative clustering instead: two clusters merge only when the AVERAGE similarity across every cross-pair clears the threshold. Raw pair-recall against the blind threshold is a hair lower than greedy's (58.8% vs 61.6%), but hand-checked purity on real title+body pairs roughly doubled (~30% -> ~50%) and max cluster size dropped from 157 to 6 — the missing pairs were mostly the false merges greedy was matching by accident. synthesize_cross_cluster and dream_distill.run_distill (read-only insight generation, not merges) keep using the original _greedy_cluster.
…d conformance tests Third data point: master baseline runs the same conformance tests cleanly (57s, confirmed); the failing jobs hang specifically in the sequential --timeout=600 conformance step on tests unrelated to clustering (test_mcp_response_budget, test_output_paths). If this hangs identically a third time, treat as confirmed and force the pure-python fallback in _average_link_cluster instead of the numpy path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jagoff
force-pushed
the
worktree-agent-a64307fdf1547bec2
branch
from
August 17, 2026 10:52
7684c3f to
070a8d3
Compare
…dates Root cause of the two CI conformance hangs (82min, 2h19m): the corpus-scale conformance fixture seeds 20 topics of ~500 near-identical vectors, so memo_consolidate's threshold graph forms dense components of ~500 members. _upgma_merge_numpy recomputed every cross-cluster block mean per candidate pair per merge — an O(k^2) Python pair loop of np.ix_().mean() calls per merge, O(k^4) element touches overall — turning one MCP tool call into ~2h of GIL-holding work. test_mcp_response_budget timed out inside it, and the still-running FastMCP worker thread starved test_output_paths afterwards (both timeouts landed in unrelated stdlib frames), then spat 'cannot schedule new futures after interpreter shutdown' per cluster at teardown. The live corpus never showed it: largest real component is 157. Lance-Williams average-linkage updates compute the exact same UPGMA merge — new avg sim to C after merging A,B is (|A|·sim(A,C)+|B|·sim(B,C))/(|A|+|B|) — as an O(k) row update plus an O(k^2) C-level argmax per merge: measured 0.11s for the k=500 dense component (was minutes), and the two previously hanging conformance tests now pass locally in 61s total. Pure-python fallback rewritten with the same update rule; the numpy/pure-python equivalence test still passes. New regression test pins the dense-component shape with a 30s bound (~300× measured, impossible for an O(k^4) version).
…licate pairs Sweep of the slow-suite fallout from flipping MEMO_SAVE_ABSORB on by default (#262): fixtures whose purpose is a *contradiction pair* — two distinct records the stub embedder deliberately maps to near-identical vectors — now had the second save absorbed into the first, collapsing the pair into one id and failing relation creation with 'a relation requires two distinct memory ids'. Invisible to fast CI: all four failing tests are excluded by -m 'not slow'. Audited all 8 files that seed pairs via upsert_open/create_relation_candidate; only these 2 failed (4 tests), the other 6 already isolate their env or seed genuinely distinct content.
Merged
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
_cluster_within_scopenow clusters via_average_link_cluster. Greedy split 38.4% of above-threshold pairs (frozen-first-representative bug); single-linkage chained 157/950 memories through bridges. Average-link sits between: measured purity on real title+body pairs ~55-60% vs greedy's ~30%.memo_consolidateinto ~2h of GIL-holding work:test_mcp_response_budgettimed out inside it and the still-running FastMCP worker starvedtest_output_pathsafterwards. Lance-Williams computes the exact same merge in O(k³) at C speed — 0.11s for the k=500 dense component; the two previously-hanging conformance tests now pass locally in 61s total. Regression test pins the dense-component shape with a 30s bound.test_ask_disputes_mlx.py,test_contradict.py) staged contradiction pairs whose members default-on ABSORB (feat: enable MEMO_SAVE_ABSORB by default #262) now merges into one id →ValidationError: a relation requires two distinct memory ids. Invisible to fast CI (-m "not slow"). Fixtures now pinMEMO_SAVE_ABSORB=0; the other 6 pair-seeding files audited clean.Test plan
tests/test_consolidate_average_link.py(6 tests: greedy-split repro, no-chaining, numpy/pure-python equivalence, singletons, empty, dense-component time bound)tests/conformance/test_mcp_response_budget.py+test_output_paths.pylocally: 10 passed in 61s (previously 82min/2h19m CI hangs)🤖 Generated with Claude Code