fix(core): stop reprocess accumulation of descriptions and edge weights (#3367)#3373
Open
ysys143 wants to merge 3 commits into
Open
fix(core): stop reprocess accumulation of descriptions and edge weights (#3367)#3373ysys143 wants to merge 3 commits into
ysys143 wants to merge 3 commits into
Conversation
…ts (HKUDS#3367) Re-merging a doc whose entities/relations already exist (any reprocess or resume) accumulated state on each merge: - Descriptions: _handle_entity_relation_summary joined the combined already_stored + newly_extracted list without cross-deduping stored vs new, so a re-extracted description equal to a stored one was appended again (fragment count grew N -> N+1). Dedup the SANITIZED list (stored descriptions are already sanitized; a raw comparison would miss duplicates whose only difference is stripped XML-illegal chars). This is the single choke point both node and edge merges route through, and a no-op for the rebuild callers (already deduped). - Edge weight: _merge_edges_then_upsert summed already_weights with every new edge's weight, so re-feeding an already-stored source double-counted (weight grew 1 -> 2 -> 3). Sum only weights of edges whose source_id is not already reflected in the stored scalar. Each extracted relation contributes 1.0, so weight tracks the distinct-source count; genuinely new sources still add, preserving legitimate multi-document growth. Regression tests (tests/extraction/test_summary_description_dedup.py): node and edge reprocess keep one description fragment; distinct and dirty (sanitized) descriptions are handled; edge weight stays fixed on same-source reprocess, grows across distinct sources, and adds only new sources on partial re-feed. Residual, pre-existing and unchanged by this fix (a complete fix needs per-source weight tracking, out of scope): a single batch with two entries from the same NEW source, and the fallback path under the source-id cap, can still over-count edge weight. An unrelated entity_vdb=None UnboundLocalError in the same edge-merge path is left for a separate fix.
In _merge_edges_then_upsert, when an edge merge adds a new endpoint entity, the entity-vdb upsert call sat OUTSIDE the `if entity_vdb is not None:` guard while `vdb_data` is assigned only inside it. So with entity_vdb=None a new endpoint source raised `UnboundLocalError: vdb_data` (and would have called None.upsert). Move the safe_vdb_operation call inside the guard. Regression test: an edge merge with entity_vdb=None and new sources completes without raising.
Collaborator
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc5d8c44e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [ | ||
| dp["weight"] | ||
| for dp in edges_data | ||
| if dp.get("source_id") not in already_source_id_set |
There was a problem hiding this comment.
When relation_chunks_storage already contains the source IDs but already_edge is missing, this filter treats those sources as already reflected in already_weights; because already_weights is empty, a retry after the earlier relation_chunks_storage.upsert(...) succeeds but before upsert_edge(...) completes will recreate the graph edge with weight 0. Only exclude sources when there is an existing edge/scalar weight for them, otherwise the recovered edge loses its contribution and can be ranked incorrectly.
Useful? React with 👍 / 👎.
Author
There was a problem hiding this comment.
Fixed in 9d90155.
You're right — existing_full_source_ids can come from relation_chunks_storage, which is a separate store from the graph edge. So on a partial write (relation_chunks upserted, upsert_edge not yet), already_weights is empty and excluding those sources recovered the edge with weight 0. The exclusion now only applies when there IS a stored scalar (already_weights non-empty); otherwise all contributions are summed, so a recovered edge keeps its weight. I reproduced the weight-0 case first, then confirmed the fix (recovers with weight 1), and added a regression test.
ysys143
added a commit
to ysys143/LightRAG
that referenced
this pull request
Jul 7, 2026
Follow-up to the edge-weight fix, from a Codex review of HKUDS#3373: the weight filter excluded sources found in existing_full_source_ids, but that set can come from relation_chunks_storage, a SEPARATE store from the graph edge. If a prior attempt upserted relation_chunks but crashed before upsert_edge, there is no stored scalar weight (already_weights is empty), so filtering those sources recovered the edge with weight 0. Only apply the exclusion when there IS a stored scalar (already_weights non-empty); otherwise sum all contributions. Regression test: an edge with relation_chunks holding the source but no graph edge yet recovers with weight 1, not 0.
ysys143
added a commit
to ysys143/LightRAG
that referenced
this pull request
Jul 7, 2026
Follow-up hardening of the edge-weight fix for the reprocess/partial-write path. The weight filter must exclude sources ALREADY reflected in the stored scalar weight. That scalar comes from the graph edge (already_weights), so filter on the edge's own source_ids (already_source_ids) — NOT existing_full_source_ids, which can come from relation_chunks_storage, a SEPARATE store. relation_chunks_storage and the graph edge can diverge on a partial write (chunks upserted, upsert_edge not yet): - chunks present, edge absent -> filtering on chunks recovered the edge with weight 0 (raised in the Codex review of HKUDS#3373); - chunks AHEAD of the edge -> filtering on chunks skipped a genuinely new source, under-counting the weight. Filtering on already_source_ids keeps the exclusion consistent with the stored scalar in both cases: no stored edge -> nothing excluded (a recovered edge keeps its full weight); a re-fed already-stored source -> dropped (no reprocess double-count). Regression tests: partial-write recovery keeps weight 1 (not 0); a new source re-fed while relation_chunks is ahead of the edge still adds weight (2, not 1); same-source reprocess stays fixed; distinct sources still accumulate.
ysys143
force-pushed
the
fix/merge-description-dedup
branch
from
9d90155 to
3c4b538
Compare
ysys143
added a commit
to ysys143/LightRAG
that referenced
this pull request
Jul 7, 2026
Follow-up hardening of the edge-weight fix for the reprocess/partial-write path.
The stored weight should track the number of distinct sources actually persisted
on the edge, so the weight sum must count exactly those and no others.
1. Exclude the edge's OWN already-stored sources, filtering on already_source_ids
(the same origin as already_weights) — NOT existing_full_source_ids, which can
come from relation_chunks_storage, a SEPARATE store. The two diverge on a
partial write (chunks upserted, upsert_edge not yet):
- chunks present, edge absent -> filtering on chunks recovered the edge with
weight 0 (raised in the Codex review of HKUDS#3373);
- chunks AHEAD of the edge -> filtering on chunks skipped a genuinely new
source, under-counting the weight.
Filtering on already_source_ids stays consistent with the stored scalar in
both cases.
2. Ignore a falsy source_id (None / ""). Such an item is already dropped from
new_source_ids (it never persists), so counting its weight would make the
weight outgrow the stored source count. Count only truthy, not-already-stored
sources — symmetric with how the source list is built.
Regression tests: partial-write recovery keeps weight 1 (not 0); a new source
re-fed while relation_chunks is ahead of the edge still adds weight (2, not 1);
a reversed-endpoint re-feed is not double-counted; a KEEP-dropped new source at
the id limit keeps weight consistent with the persisted count; a falsy source_id
adds no weight; same-source reprocess stays fixed; distinct sources accumulate.
ysys143
force-pushed
the
fix/merge-description-dedup
branch
from
3c4b538 to
41edcdc
Compare
Follow-up hardening of the edge-weight fix for the reprocess/partial-write path.
The stored weight should track the number of distinct sources actually persisted
on the edge, so the weight sum must count exactly those and no others.
1. Exclude the edge's OWN already-stored sources, filtering on already_source_ids
(the same origin as already_weights) — NOT existing_full_source_ids, which can
come from relation_chunks_storage, a SEPARATE store. The two diverge on a
partial write (chunks upserted, upsert_edge not yet):
- chunks present, edge absent -> filtering on chunks recovered the edge with
weight 0 (raised in the Codex review of HKUDS#3373);
- chunks AHEAD of the edge -> filtering on chunks skipped a genuinely new
source, under-counting the weight.
Filtering on already_source_ids stays consistent with the stored scalar in
both cases.
2. Ignore a falsy source_id (None / ""). Such an item is already dropped from
new_source_ids (it never persists), so counting its weight would make the
weight outgrow the stored source count. Count only truthy, not-already-stored
sources — symmetric with how the source list is built.
Regression tests cover, at both the unit (_merge_*_then_upsert) and the
orchestrator (merge_nodes_and_edges) level:
- partial-write recovery keeps weight 1 (not 0);
- a new source re-fed while relation_chunks is ahead of the edge still adds
weight (2, not 1);
- a reversed-endpoint re-feed is not double-counted;
- a KEEP-dropped new source at the id limit keeps weight consistent with the
persisted count;
- a falsy source_id adds no weight;
- node-side dedup of an identical description from two sources keeps BOTH
sources (no provenance loss);
- end-to-end: reprocessing the same doc does not accumulate description or
weight, while distinct docs still grow both;
- same-source reprocess stays fixed; distinct sources accumulate.
ysys143
force-pushed
the
fix/merge-description-dedup
branch
from
41edcdc to
640b41a
Compare
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.
Fixes #3367.
Re-merging a doc whose entities/relations already exist (any reprocess or resume) accumulated state on each merge. Two independent accumulations, plus an adjacent crash found while verifying the edge path.
Descriptions (commit 1)
_handle_entity_relation_summary— the single choke point both node and edge merges route theiralready_stored + newly_extracteddescription list through — joined without cross-deduping stored vs new, so a re-extracted description equal to a stored one was appended again (fragment count grew N -> N+1). Dedup the sanitized list: stored descriptions are already sanitized while a re-extracted one is raw, so a raw comparison would miss duplicates whose only difference is a stripped XML-illegal char. No-op for the rebuild callers (already deduped).Edge weight (commit 1)
_merge_edges_then_upsertsummedalready_weightswith every new edge's weight, so re-feeding an already-stored source double-counted (weight grew 1 -> 2 -> 3). Each extracted relation contributes 1.0, so weight tracks the distinct-source count. Sum only weights of edges whosesource_idis not already reflected in the stored scalar — genuinely new sources still add, preserving legitimate multi-document growth.entity_vdb=None crash (commit 2, separate)
In the same edge-merge path, the entity-vdb upsert for a new endpoint entity sat outside its
if entity_vdb is not None:guard while referencingvdb_data(assigned only inside), soentity_vdb=NoneraisedUnboundLocalError. Moved the call inside the guard. Kept as a separate commit — an adjacent robustness fix, not part of the accumulation bug.Tests
tests/extraction/test_summary_description_dedup.py: node/edge reprocess keep one description fragment; distinct and dirty (sanitized) descriptions handled; edge weight stays fixed on same-source reprocess, grows across distinct sources, adds only new sources on partial re-feed; edge merge withentity_vdb=Nonecompletes without raising.Scope / residual
source_id/file_pathwere already deduped and unaffected. Two pre-existing edge-weight over-count corners remain, unchanged by this PR (a complete fix needs per-source weight tracking, out of scope): a single batch with two entries from the same new source, and the fallback path under the source-id cap.