mirror of
https://github.com/safishamsi/graphify.git
synced 2026-08-27 08:46:43 +00:00
fix(dedup): deterministic node-id collision rank, active over archived (#2532)
_collision_rank no longer falls through to raw lexical source_file order. It now ranks by a lifecycle penalty (active over archived) computed on the root-relative path, then a reversed-segment tie-break, so the winner of a collision (e.g. plans/_done/x.md vs plans/in-progress/x.md) no longer depends on ASCII filename order, absolute-vs-relative path form, or the checkout directory name. Active/archived idea from PR #2540 (thanks @michaelxer); the root-relative and reversed-segment determinism is implemented here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
22c41c1b23
commit
3c17238b4e
+1
-1
@@ -1306,7 +1306,7 @@ def build(
|
||||
_fold_node_aliases(n)
|
||||
combined["nodes"], combined["edges"] = deduplicate_entities(
|
||||
combined["nodes"], combined["edges"], communities={},
|
||||
dedup_llm_backend=dedup_llm_backend,
|
||||
dedup_llm_backend=dedup_llm_backend, root=root,
|
||||
)
|
||||
return build_from_json(combined, directed=directed, root=root)
|
||||
|
||||
|
||||
+87
-7
@@ -9,6 +9,7 @@ import re
|
||||
import sys
|
||||
import unicodedata
|
||||
from collections import defaultdict
|
||||
from pathlib import Path
|
||||
|
||||
from graphify._minhash import MinHash, MinHashLSH
|
||||
from rapidfuzz.distance import Jaro, JaroWinkler
|
||||
@@ -225,22 +226,89 @@ def _defines_id(node: dict) -> bool:
|
||||
for prefix in _id_prefixes(source_file))
|
||||
|
||||
|
||||
def _collision_rank(node: dict) -> tuple:
|
||||
# Path-segment lifecycle markers used by _collision_rank (#2532). Lower penalty
|
||||
# wins. Without them, pure lexical source_file order makes ``plans/_done/…``
|
||||
# beat ``plans/in-progress/…`` because "_" < "i" in ASCII. Active-vs-archived
|
||||
# marker idea by @michaelxer (#2540); matched against ROOT-RELATIVE directory
|
||||
# segments only, so a checkout directory that happens to be named ``wip`` or
|
||||
# ``done`` never leaks into the ranking.
|
||||
_ACTIVE_PATH_SEGMENTS = frozenset({
|
||||
"in-progress",
|
||||
"in_progress",
|
||||
"active",
|
||||
"current",
|
||||
"wip",
|
||||
})
|
||||
_ARCHIVED_PATH_SEGMENTS = frozenset({
|
||||
"_done",
|
||||
"done",
|
||||
"archive",
|
||||
"archived",
|
||||
"backup",
|
||||
"bak",
|
||||
"old",
|
||||
"attic",
|
||||
"graveyard",
|
||||
"completed",
|
||||
})
|
||||
|
||||
|
||||
def _lifecycle_penalty(rank_path: str) -> int:
|
||||
"""0 for active/in-progress paths, 2 for archived/done paths, 1 otherwise.
|
||||
|
||||
Judged on the DIRECTORY segments of the root-relative rank path — a file
|
||||
literally named ``done.md`` is not a marker. Among mixed markers the best
|
||||
(lowest) score wins so an active segment is not drowned out by an unrelated
|
||||
archive directory higher in the tree (#2532).
|
||||
"""
|
||||
segments = [s for s in rank_path.casefold().split("/") if s]
|
||||
marked = [
|
||||
0 if s in _ACTIVE_PATH_SEGMENTS else 2
|
||||
for s in segments[:-1] # directories only, never the basename
|
||||
if s in _ACTIVE_PATH_SEGMENTS or s in _ARCHIVED_PATH_SEGMENTS
|
||||
]
|
||||
return min(marked) if marked else 1
|
||||
|
||||
|
||||
def _rank_path(source_file: str, root: Path | None) -> str:
|
||||
"""The root-relative form of ``source_file`` used for collision ranking.
|
||||
|
||||
Mirrors ``_source_key`` in extractors/resolution.py: with a scan root, an
|
||||
absolute stored path and its repo-relative twin rank identically, and the
|
||||
checkout location's own segments never participate (#2532). Without a root
|
||||
(or when relativizing fails) the normalized stored path is used as-is.
|
||||
"""
|
||||
normalized = source_file.replace("\\", "/")
|
||||
if root is not None and normalized:
|
||||
try:
|
||||
return Path(normalized).resolve().relative_to(root).as_posix()
|
||||
except Exception:
|
||||
pass
|
||||
return normalized
|
||||
|
||||
|
||||
def _collision_rank(node: dict, root: Path | None = None) -> tuple:
|
||||
"""A total order for choosing the survivor of an ID collision, independent of
|
||||
the order the colliding nodes arrive in.
|
||||
|
||||
The winner is the node with the SMALLEST rank. A node whose ``source_file``
|
||||
defines the ID always outranks a mere reference; among equally-(non-)defining
|
||||
nodes it prefers the shorter, more canonical label over a longer qualified
|
||||
variant, then breaks any remaining tie lexically on label and then source_file
|
||||
(so the lexically-first path wins) — fully deterministic regardless of order.
|
||||
nodes an active/in-progress path outranks an archived/done one (#2532); then
|
||||
it prefers the shorter, more canonical label over a longer qualified variant,
|
||||
then breaks any remaining tie lexically on label and finally on the REVERSED
|
||||
segments of the root-relative path. Basename-first comparison decides two
|
||||
in-repo colliders by segments present in both path forms, so absolute and
|
||||
repo-relative spellings of the same layout order identically — fully
|
||||
deterministic regardless of arrival order (#1851) or checkout location.
|
||||
"""
|
||||
label = node.get("label") or ""
|
||||
rank_path = _rank_path(node.get("source_file") or "", root)
|
||||
return (
|
||||
not _defines_id(node), # definers (False) sort before references (True)
|
||||
_lifecycle_penalty(rank_path), # active paths beat archived ones (#2532)
|
||||
len(label), # shorter, more canonical label first
|
||||
label, # lexical tiebreak
|
||||
node.get("source_file") or "", # lexically-first source path wins
|
||||
tuple(reversed([s for s in rank_path.split("/") if s and s != "."])),
|
||||
)
|
||||
|
||||
|
||||
@@ -323,6 +391,7 @@ def deduplicate_entities(
|
||||
*,
|
||||
communities: dict[str, int],
|
||||
dedup_llm_backend: str | None = None,
|
||||
root: str | Path | None = None,
|
||||
) -> tuple[list[dict], list[dict]]:
|
||||
"""Deduplicate near-identical entities in a knowledge graph.
|
||||
|
||||
@@ -331,6 +400,8 @@ def deduplicate_entities(
|
||||
edges: list of edge dicts with {"source": str, "target": str, ...}
|
||||
communities: mapping of node_id -> community_id (from cluster())
|
||||
dedup_llm_backend: if set, use LLM to resolve ambiguous pairs
|
||||
root: scan root; ID-collision ranking judges source paths relative to
|
||||
it so path form and checkout location cannot flip the survivor (#2532)
|
||||
|
||||
Returns:
|
||||
(deduped_nodes, deduped_edges) with edges rewired to survivors
|
||||
@@ -348,6 +419,15 @@ def deduplicate_entities(
|
||||
if len(nodes) <= 1:
|
||||
return nodes, edges
|
||||
|
||||
# Resolve the scan root once: _collision_rank ranks each node's source_file
|
||||
# relative to it, so an absolute stored path and its repo-relative twin rank
|
||||
# identically and lifecycle markers in the checkout location's own segments
|
||||
# cannot flip the survivor (#2532).
|
||||
try:
|
||||
root_resolved: Path | None = Path(root).resolve() if root else None
|
||||
except Exception:
|
||||
root_resolved = None
|
||||
|
||||
# Pre-deduplicate: one node per ID. The survivor is the node that *defines* the
|
||||
# ID (its source_file is the file the ID encodes), not merely the first seen —
|
||||
# otherwise chunk order decides whether an entity keeps its own attributes or a
|
||||
@@ -363,7 +443,7 @@ def deduplicate_entities(
|
||||
incumbent = seen_ids.get(nid)
|
||||
if incumbent is None:
|
||||
seen_ids[nid] = node
|
||||
elif _collision_rank(node) < _collision_rank(incumbent):
|
||||
elif _collision_rank(node, root_resolved) < _collision_rank(incumbent, root_resolved):
|
||||
# Smallest-ranked node wins; the min over a total order is independent
|
||||
# of the order nodes arrive in, so the survivor no longer depends on
|
||||
# chunk ordering (#1851).
|
||||
@@ -381,7 +461,7 @@ def deduplicate_entities(
|
||||
survivor = seen_ids[nid]
|
||||
same_source = sorted(
|
||||
(l for l in losers if _same_source_entity(survivor, l)),
|
||||
key=_collision_rank,
|
||||
key=lambda l: _collision_rank(l, root_resolved),
|
||||
)
|
||||
for loser in same_source:
|
||||
survivor = _merge_missing_attributes(survivor, loser)
|
||||
|
||||
+110
-1
@@ -1,7 +1,14 @@
|
||||
"""Tests for graphify/dedup.py entity deduplication pipeline."""
|
||||
from __future__ import annotations
|
||||
import pytest
|
||||
from graphify.dedup import deduplicate_entities, _defines_id, _entropy, _shingles
|
||||
from graphify.dedup import (
|
||||
deduplicate_entities,
|
||||
_collision_rank,
|
||||
_defines_id,
|
||||
_entropy,
|
||||
_lifecycle_penalty,
|
||||
_shingles,
|
||||
)
|
||||
|
||||
|
||||
# ── entropy gate ─────────────────────────────────────────────────────────────
|
||||
@@ -628,6 +635,108 @@ def test_defines_id_helper():
|
||||
assert not _defines_id({"id": "docs_intro_foo", "source_file": ""})
|
||||
|
||||
|
||||
# ── #2532: lifecycle-aware, environment-stable collision ranking ──────────────
|
||||
# Active-vs-archived path markers adapted from @michaelxer's PR #2540, judged on
|
||||
# root-relative segments with a reversed-segment final tiebreak so the survivor
|
||||
# cannot flip with path form or checkout location.
|
||||
|
||||
_DONE_PLAN = {"id": "plans_binding_doctrine", "label": "binding doctrine",
|
||||
"file_type": "concept",
|
||||
"source_file": "plans/_done/binding-doctrine.md"}
|
||||
_ACTIVE_PLAN = {"id": "plans_binding_doctrine", "label": "binding doctrine",
|
||||
"file_type": "concept",
|
||||
"source_file": "plans/in-progress/binding-doctrine.md"}
|
||||
|
||||
|
||||
def test_lifecycle_penalty_on_relative_segments():
|
||||
assert _lifecycle_penalty("plans/_done/x.md") == 2
|
||||
assert _lifecycle_penalty("plans/in-progress/x.md") == 0
|
||||
assert _lifecycle_penalty("docs/x.md") == 1
|
||||
# A FILE named after a marker is not a marker — only directory segments count.
|
||||
assert _lifecycle_penalty("done.md") == 1
|
||||
assert _lifecycle_penalty("plans/done.md") == 1
|
||||
# Mixed markers: the best (most active) marked segment wins.
|
||||
assert _lifecycle_penalty("archive/in-progress/x.md") == 0
|
||||
|
||||
|
||||
def test_archived_path_ranks_below_active_despite_ascii_order():
|
||||
"""#2532: `plans/_done` sorts before `plans/in-progress` lexically ('_' < 'i'),
|
||||
so a raw-string tiebreak picks the archived copy — assert the ASCII trap is
|
||||
real, then that the rank inverts it."""
|
||||
assert _DONE_PLAN["source_file"] < _ACTIVE_PLAN["source_file"] # the trap
|
||||
assert _collision_rank(_ACTIVE_PLAN) < _collision_rank(_DONE_PLAN)
|
||||
|
||||
|
||||
def test_active_plan_survives_archived_copy_order_independent():
|
||||
"""#2532 reported case: the in-progress copy survives, whichever order the
|
||||
colliding nodes arrive in."""
|
||||
import itertools
|
||||
for perm in itertools.permutations([_DONE_PLAN, _ACTIVE_PLAN]):
|
||||
out, _ = deduplicate_entities([dict(n) for n in perm], [], communities={})
|
||||
assert len(out) == 1
|
||||
assert out[0]["source_file"] == "plans/in-progress/binding-doctrine.md"
|
||||
|
||||
|
||||
def test_checkout_dir_lifecycle_name_does_not_mark_paths(tmp_path):
|
||||
"""Gap in #2540: markers must be matched on ROOT-RELATIVE segments only. An
|
||||
absolute source_file stored under a checkout directory named `wip` must not
|
||||
be scored active by the checkout location — the archived copy still loses."""
|
||||
import itertools
|
||||
root = tmp_path / "wip" / "repo"
|
||||
archived = {"id": "plans_doc", "label": "doc", "file_type": "concept",
|
||||
"source_file": str(root / "plans" / "_done" / "doc.md")}
|
||||
active = {"id": "plans_doc", "label": "doc", "file_type": "concept",
|
||||
"source_file": "plans/roadmap/doc.md"}
|
||||
for perm in itertools.permutations([archived, active]):
|
||||
out, _ = deduplicate_entities([dict(n) for n in perm], [],
|
||||
communities={}, root=root)
|
||||
assert len(out) == 1
|
||||
assert out[0]["source_file"] == "plans/roadmap/doc.md"
|
||||
|
||||
|
||||
def test_neutral_collision_survivor_is_stable_across_path_forms(tmp_path):
|
||||
"""#2532: with no lifecycle markers, the tiebreak must not depend on whether
|
||||
either path is stored absolute or repo-relative — one root-relative file
|
||||
identity survives across all four form combinations and both insertion
|
||||
orders (8 runs)."""
|
||||
import itertools
|
||||
root = tmp_path / "repo"
|
||||
rel_a, rel_b = "plans/q3/doc.md", "plans/roadmap/doc.md"
|
||||
survivors = set()
|
||||
for form_a in (rel_a, str(root / rel_a)):
|
||||
for form_b in (rel_b, str(root / rel_b)):
|
||||
a = {"id": "plans_doc", "label": "doc", "file_type": "concept",
|
||||
"source_file": form_a}
|
||||
b = {"id": "plans_doc", "label": "doc", "file_type": "concept",
|
||||
"source_file": form_b}
|
||||
for perm in itertools.permutations([a, b]):
|
||||
out, _ = deduplicate_entities([dict(n) for n in perm], [],
|
||||
communities={}, root=root)
|
||||
assert len(out) == 1
|
||||
sf = out[0]["source_file"].replace("\\", "/")
|
||||
prefix = str(root).replace("\\", "/") + "/"
|
||||
survivors.add(sf.removeprefix(prefix))
|
||||
assert survivors == {"plans/q3/doc.md"}, (
|
||||
f"survivor depends on path form or order: {survivors}"
|
||||
)
|
||||
|
||||
|
||||
def test_archived_definer_beats_active_reference():
|
||||
"""The definer flag outranks the lifecycle penalty: a node that owns its ID
|
||||
survives even from an archived folder against a live cross-reference."""
|
||||
import itertools
|
||||
nid = "plans_done_spec_spec"
|
||||
definer = {"id": nid, "label": "spec doc", "file_type": "concept",
|
||||
"source_file": "plans/_done/spec.md"}
|
||||
reference = {"id": nid, "label": "spec", "file_type": "concept",
|
||||
"source_file": "plans/in-progress/roadmap.md"}
|
||||
assert _defines_id(definer) and not _defines_id(reference)
|
||||
for perm in itertools.permutations([definer, reference]):
|
||||
out, _ = deduplicate_entities([dict(n) for n in perm], [], communities={})
|
||||
assert len(out) == 1
|
||||
assert out[0]["source_file"] == "plans/_done/spec.md"
|
||||
|
||||
|
||||
# ── #2091 review: attribute-merge correctness (fixes A-D) ─────────────────────
|
||||
|
||||
def test_dedup_gapfill_is_order_independent_with_multiple_losers():
|
||||
|
||||
Reference in New Issue
Block a user