Skip to content

feat(graph): deterministic key-based entity dedup via merged_into edges (SDK-208)#4136

Draft
rshkarin wants to merge 3 commits into
devfrom
feature/sdk-208-deterministic-key-based-entity-dedup-rework-of-pr-4028-gh
Draft

feat(graph): deterministic key-based entity dedup via merged_into edges (SDK-208)#4136
rshkarin wants to merge 3 commits into
devfrom
feature/sdk-208-deterministic-key-based-entity-dedup-rework-of-pr-4028-gh

Conversation

@rshkarin

Copy link
Copy Markdown
Contributor

Description

Review + rework of community hackathon PR #4028 (by @Darshan3690) for hackathon issue #3627Approach A: deterministic, key-based dedup (no LLM). Linear: SDK-208.

Approach A is 1 of 5 parallel approach tickets (A–E) exploring contradiction & dedup resolution; the goal is to compare them on shared fixtures and adopt the best (or a hybrid). This PR keeps the original author's commit as the base (attribution intact) and layers the rework as separate commits on top.

What the original PR did (all runtime-verified)

The normalisation logic itself is reasonable, but the feature did not work end-to-end:

  • The alias/canonicalization is a no-op for dedup. deduplicate_nodes_and_edges keys on str(node.id); the canonicalization rewrote node.name after ids were assigned and never recomputed the id. Two surface variants kept distinct ids and were not merged — while both display names were destructively mangled to a slug (e.g. both became "united_states" yet stayed two nodes). (verified: 2 nodes in → 2 nodes out, 0 merges.)
  • The type-aware id change silently drops entity→entity edges. _create_entity_node computed Entity.id_for(name, type_node.name), but _process_graph_edges / _resolve_node still resolve endpoints by name-only Entity.id_for(name), so no edge matched a node key and every extracted relationship was dropped in _populate_node_relations. (verified: inter-entity edge count 1 → 0.) It also changed all entity ids (orphaning re-cognified graphs) and other single-arg id_for call sites, with no migration.
  • Provenance never persists / dead code. MergeRecords were computed then discarded; merge_with_existing and ContradictionEdge had no call sites.
  • Breaks existing tests. The 3-tuple return raises too many values to unpack (expected 2) in the four delete tests, and the new alias_map= / merge_policy= kwargs break the ~12 add_data_points tests that mock a 2-arg 2-tuple.

The rework

  • Keep the sound normaliser. The author's canonicalize_entity_name (NFKC + diacritics + lowercase + alias lookup + punctuation/whitespace collapse + leading-article strip) and default_aliases.json are kept — but used purely as a grouping key, never to mutate a node's display name (added a cached alias-map load).
  • New resolve_deterministic_duplicates(nodes) matching the sibling utils (resolve_structural_duplicates, resolve_fuzzy_duplicate_entities): group same-type nodes by canonical key; within each group the longest original name is kept canonical (ties → smaller id) and every other node emits a merged_into edge (duplicate → canonical) carrying resolution="deterministic_key" and the canonical_key.
  • Persist the merge as graph data. Edge properties are already persisted by cognee, so this is provenance-preserving (what merged into what + the key why), non-destructive (both nodes and all their edges kept), and reversible (drop the edge to undo) — the same pattern as sibling Approaches B/D, differing only in the detection signal.
  • Extend the existing path. Wired into add_data_points right after deduplicate_nodes_and_edges, opt-in via add_data_points(..., deterministic_dedup=True) (default off; a disabled run does zero extra work). deduplicate_nodes_and_edges and expand_with_nodes_and_edges are reverted to their dev versions byte-for-byte — no core-algorithm drift, no caller breakage.
  • Drop the type-aware id change, MergePolicy, MergeRecord / FieldResolution / ContradictionEdge, and merge_with_existing entirely — dead or broken, and redundant with the merged_into edge.

Net change vs dev: additive only (canonicalization util + alias map + one new module + a 17-line opt-in hook + tests); the two core-algorithm files are unchanged.

Requirements (issue #3627)

  • Extends the existing dedup path (no parallel pipeline)
  • Provenance preserved on merge (what merged into what, and why) — via the merged_into edge properties
  • Actionable, reversible merges (drop the edge to undo; both nodes retained)
  • Deterministic, no LLM — reproducible under the mocked-LLM CI harness (Hackathon: build LLM-mocked tests for all examples in the repo #3601)
  • Test coverage: canonicalization, the resolver (case/punctuation + alias variants, type-gating, negatives, non-mutation), and add_data_points wiring

Out of scope (deferred)

Retrieval-side consumption of merged_into (collapsing duplicates at query time), cross-batch / already-persisted duplicate detection, threading deterministic_dedup through the public cognify() API, and destructive property-conflict merging (kept reversible via edges instead of a last-write / non-null merge policy) — deferred until siblings A–E are compared on a shared fixture and the best (or a hybrid) is adopted (per #3627's plan). Coordinate the audit trail with #3604.

Testing

  • cognee/tests/unit/modules/graph/test_deterministic_dedup.py, test_canonicalization.py, and the new add_data_points wiring tests pass; the full test_add_data_points.py suite stays green (44 passed).
  • End-to-end against a real Ladybug graph: three variants ("USA" / "U.S.A." / "United States") produce two merged_into edges that persist and read back with their properties intact, both pointing at the canonical node.
  • ruff format / ruff check clean.

Type of Change

  • New feature (non-breaking, opt-in)
  • Code refactoring

DCO Affirmation

I affirm that all code in every commit of this pull request conforms to the terms of the Topoteretes Developer Certificate of Origin.

🤖 Generated with Claude Code

Darshan3690 and others added 3 commits July 19, 2026 23:07
- Fold EntityType name into Entity.id_for at construction time
- Add canonicalization phase to in-batch deduplication
- Add cross-run fetch and MergePolicy for conflict resolution
- Add MergeRecord, FieldResolution, ContradictionEdge logs
- Fix edge key delimiter to prevent collisions
…lization as a pure key [SDK-208]

The original PR did not work end-to-end (all runtime-verified):

- The canonicalization mutated `node.name` *after* ids were fixed, but
  `deduplicate_nodes_and_edges` keys on `str(node.id)`; surface variants
  ("USA"/"United States") were never merged while their display names were
  destructively mangled to slugs.
- `Entity.id_for(node_id, type_node.name)` in `_create_entity_node` broke node
  lookup: `_process_graph_edges`/`_resolve_node` still resolve endpoints by
  name-only `Entity.id_for(name)`, so every extracted entity->entity edge was
  silently dropped. It also changed all entity ids (orphaning re-cognified
  graphs) and other single-arg `id_for` call sites.
- The 3-tuple return broke every un-updated caller (four delete tests unpack
  two values; ~12 add_data_points tests mock a 2-arg 2-tuple).
- `MergeRecord`/`FieldResolution` provenance was computed then discarded;
  `merge_with_existing` and `ContradictionEdge` were dead code.

This commit reverts the two core-algorithm files to their dev versions, removes
the dead/broken modules, and keeps only the sound piece — the name
normaliser — as a pure grouping-key function (cached alias-map load, no name
mutation). The author's misplaced/red tests are removed; real coverage follows.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…208]

Approach A of gh #3627: resolve name-variant duplicate entities with cheap,
deterministic string rules — no LLM, no embeddings.

`resolve_deterministic_duplicates(nodes)` groups same-type nodes by a canonical
key (NFKC + diacritics + lowercase + alias map + punctuation/whitespace collapse
+ leading-article strip); within each group the longest original name is kept
canonical (ties -> smaller id) and every other node emits a `merged_into` edge
(duplicate -> canonical) carrying `resolution="deterministic_key"` and the
`canonical_key`. Edge properties are already persisted by cognee, so the merge
is recorded in the graph — provenance-preserving (what merged into what + why),
non-destructive (both nodes and all their edges kept), and reversible (drop the
edge to undo) — matching the sibling Approaches B/D, differing only in the
detection signal.

Wired into `add_data_points` right after `deduplicate_nodes_and_edges` (extends
the existing path, not a parallel pipeline), opt-in via `deterministic_dedup=True`
(default off; a disabled run does zero extra work).

Deterministic unit tests import the real resolver: case/punctuation and alias
variants -> merged_into edges to one canonical, type-gating, single-node /
same-id / no-name negatives, input non-mutation, corrected canonicalization
tests; plus add_data_points wiring tests proving the default path is unchanged
and the merged_into edge reaches the graph write when enabled.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rshkarin rshkarin added core-team hackathon Hackathon-scoped task labels Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-team hackathon Hackathon-scoped task

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants