Skip to content

fix: handle multiple GLiNER labels share the same span - #238

Open
asteier2026 wants to merge 3 commits into
mainfrom
asteier2026/bugfix/gliner-duplicate-span-score-tiebreak
Open

fix: handle multiple GLiNER labels share the same span#238
asteier2026 wants to merge 3 commits into
mainfrom
asteier2026/bugfix/gliner-duplicate-span-score-tiebreak

Conversation

@asteier2026

Copy link
Copy Markdown
Contributor

Summary

  • resolve_overlaps previously used alphabetical label order as the final tiebreaker when two GLiNER detections shared the exact same character span, causing higher-confidence labels to be silently dropped in favour of lower-confidence ones.
  • Added -item.score to the sort key (before item.label) so the highest-scoring label wins on exact-span ties.

Example: "Mum" was tagged as both relationship (score 0.941) and last_name (score 0.719). Before this fix, last_name won because l < r alphabetically. After, relationship correctly wins.

Test plan

  • Existing test_detection_postprocess.py parametrized test covers the new behaviour — verify it passes with make test
  • Spot-check a rewrite run on a text with relationship/family terms to confirm relationship entities surface correctly

🤖 Generated with Claude Code

@asteier2026
asteier2026 requested a review from a team as a code owner August 6, 2026 15:24
@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR scopes confidence-based overlap resolution to raw GLiNER detections and preserves original detector provenance during occurrence expansion.

  • Adds an opt-in highest-score tie-breaker for identical detector spans.
  • Leaves mixed-source overlap resolution on its previous label-based ordering.
  • Skips synthetic propagation copies at positions already covered by original entities.
  • Adds regression coverage for GLiNER ties, augmentation, and propagation provenance.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
src/anonymizer/engine/detection/postprocess.py Correctly scopes score precedence to detector-only parsing and prevents propagation copies from replacing original spans.
tests/engine/test_detection_postprocess.py Adds focused regression tests covering exact-span score selection and mixed-source provenance preservation.

Reviews (5): Last reviewed commit: "fix: scope score-based tiebreak to parse..." | Re-trigger Greptile

-(item.end_position - item.start_position),
item.start_position,
item.end_position,
-item.score,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Avoid comparing mixed-source scores

The shared resolver applies this score precedence to detector, augmented, and propagated entities, although the latter two receive synthetic scores of 1.0. On identical spans, those synthetic scores always displace detector labels regardless of detector confidence, changing the entity type and its downstream replacement strategy.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a broader consequence here: expand_entity_occurrences() creates a synthetic score=1.0 propagation copy at every original span. With this new sort key, an ordinary detector entity such as (id="detector-id", score=0.93, source="detector") is replaced by (id="first_name_0_5", score=1.0, source="propagation"), even when the label is identical. This systematically corrupts the documented final_entities provenance, rather than affecting only mixed-label collisions. The confidence tiebreak should be scoped to detector-vs-detector spans, or original spans should be preserved against synthetic copies. A regression test should assert that expansion retains the original ID, score, and source at the detected occurrence.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and this is a real regression. Looking at expand_entity_occurrences() (lines 287–315):

expanded.append(
    EntitySpan(
        entity_id=entity_id,
        ...
        score=1.0,
        source="propagation",
    )
)
all_entities = [*entities, *expanded]
return resolve_overlaps(all_entities)  # ← score tiebreak now fires here

Every original span appears twice in all_entities — once as the detector entity with its real score/source, and once as a propagation copy with score=1.0. With -item.score in the sort key, the propagation copy always wins the tiebreak on identical spans, silently replacing entity_id, score, and source on the original detection positions. That corrupts the provenance trail even when the label is identical.

The simplest scoped fix is to add a source priority tier to the sort key before the score, so detector entities beat synthetic ones on equal spans:

Suggested change
-item.score,
-(item.end_position - item.start_position),
item.start_position,
item.end_position,
{"detector": 0, "name_split": 1, "augmenter": 2, "propagation": 3}.get(item.source, 9),
-item.score,

This keeps the original intent (highest-confidence label wins on exact-span ties among detector results) without letting synthetic copies displace real detector entities. A regression test asserting that expand_entity_occurrences retains the original entity_id, score, and source at the detected position would also be a strong addition here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete suggestion: make score precedence opt-in on resolve_overlaps() and enable it only from parse_raw_entities(), where every candidate is a directly comparable GLiNER detection.

def resolve_overlaps(
    entities: list[EntitySpan],
    *,
    prefer_highest_score: bool = False,
) -> list[EntitySpan]:
    sorted_entities = sorted(
        entities,
        key=lambda item: (
            -(item.end_position - item.start_position),
            item.start_position,
            item.end_position,
            -item.score if prefer_highest_score else 0.0,
            item.label,
        ),
    )
    ...

Then use:

return resolve_overlaps(parsed, prefer_highest_score=True)

Other callers—particularly apply_augmented_entities() and expand_entity_occurrences()—retain their existing ordering and do not compare real detector confidence against synthetic score-1.0 values.

The new test should exercise parse_raw_entities() with two GLiNER labels sharing a span rather than the generic resolver without the scoped option. Please also extend test_expand_preserves_original_entities() to assert the original entity_id, score, and source, not only the values.

This is narrower than adding a global source-priority policy, which would introduce new precedence semantics unrelated to this bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — parse_raw_entities() at line 100 is the only call site where all inputs are source="detector" with real GLiNER scores. Both apply_augmented_entities() (line 201) and expand_entity_occurrences() (line 314) mix in synthetic score=1.0 entities before calling resolve_overlaps, so they must not receive the flag.

Your proposed API is the right fix. The only implementation detail worth checking: in the fallback branch (prefer_highest_score=False), using 0.0 as the constant means the item.label tiebreaker still applies identically to the current pre-PR behavior for all other callers — that's exactly correct.

For the tests, here's what should be covered:

  1. parse_raw_entities() with a shared span — two GLiNER detections at identical positions, higher-score label wins:
# relationship (0.941) beats last_name (0.719) on same span
assert result[0].label == "relationship"
assert result[0].score == 0.941
  1. expand_entity_occurrences() retains original detector entity — the propagation copy at the original span must not displace the detector entity:
original = entities[0]  # source="detector", real entity_id, real score
expanded = expand_entity_occurrences(text, entities)
at_original_span = [e for e in expanded if e.start_position == original.start_position and e.end_position == original.end_position]
assert len(at_original_span) == 1
assert at_original_span[0].entity_id == original.entity_id
assert at_original_span[0].score == original.score
assert at_original_span[0].source == "detector"

The scoped opt-in approach is strictly better than the global source-priority map I suggested — it's narrower, self-documenting at the call site, and doesn't couple resolve_overlaps to knowledge of synthetic source names.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks—409fdb4 fixes the propagation-provenance case. However, the other mixed-source caller remains unchanged: apply_augmented_entities() combines validated detector spans with augmenter spans carrying a synthetic score=1.0, then passes them to this resolver.

I can still reproduce the issue:

detector = EntitySpan(
    "email_0_5", "Alice", "email", 0, 5, 0.95, "detector"
)
result = apply_augmented_entities(
    "Alice",
    [detector],
    {"entities": [{"value": "Alice", "label": "last_name"}]},
)

The current result is last_name, score 1.0, source augmenter. Before this PR, the validated email span won using the existing label tie-breaker.
Could we scope score precedence to parse_raw_entities(), as proposed above? That would resolve the GLiNER ambiguity without changing overlap behavior for mixed detector/augmenter inputs.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct — expand_entity_occurrences was fixed by commit 409fdb4 (the if (start, end) in original_positions: continue guard), but apply_augmented_entities still passes a mixed detector+augmenter list to the unscoped resolve_overlaps, so augmenter spans at score=1.0 still displace detector spans on identical positions.

The opt-in approach from the earlier discussion is the right fix. Here's the concrete implementation:

1. Add the flag to resolve_overlaps:

Suggested change
-item.score,
def resolve_overlaps(entities: list[EntitySpan], *, prefer_highest_score: bool = False) -> list[EntitySpan]:
"""Resolve span conflicts by preferring longer spans, then earlier starts."""
sorted_entities = sorted(
entities,
key=lambda item: (
-(item.end_position - item.start_position),
item.start_position,
item.end_position,
-item.score if prefer_highest_score else 0.0,
item.label,
),
)

2. Enable it only in parse_raw_entities (line 100):

return resolve_overlaps(parsed, prefer_highest_score=True)

All other callers — apply_augmented_entities and expand_entity_occurrences — keep the default False, so the label tiebreaker restores the pre-PR behavior for mixed-source inputs. With your example, email (detector) would win over last_name (augmenter) via label sort, same as before the PR, which is the correct outcome since a validated detector span should not be displaced by an augmenter suggestion.

@lipikaramaswamy lipikaramaswamy Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 409fdb4 preserves the original entity during propagation. One PR-introduced cross-source change still remains in apply_augmented_entities(): augmenter spans are assigned a synthetic score=1.0, so the new generic score tie-break can replace an already validated detector span at the same position.

So the original scope of the PR is multiple GLiNER labels sharing a span. Could we keep overlap deduplication unchanged for all callers, but make score precedence opt-in only when parse_raw_entities() resolves raw GLiNER candidates? That addresses the reported "Mum" case in tests without introducing a new detector-vs-augmenter precedence policy in this PR.

The pre-existing alphabetical tie-break for detector/augmenter conflicts can then be handled separately if we want to define an explicit provenance policy. (Maybe something like: New stages may add coverage, but they should not silently rewrite the provenance of an entity already adjudicated by an earlier stage.) We can do a follow up issue / PR for this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — scoping it to parse_raw_entities() is the right call, and leaving the detector/augmenter precedence policy as a follow-up is cleaner.

Here's the minimal implementation:

resolve_overlaps signature change:

Suggested change
-item.score,
def resolve_overlaps(entities: list[EntitySpan], *, prefer_highest_score: bool = False) -> list[EntitySpan]:
"""Resolve span conflicts by preferring longer spans, then earlier starts."""
sorted_entities = sorted(
entities,
key=lambda item: (
-(item.end_position - item.start_position),
item.start_position,
item.end_position,
-item.score if prefer_highest_score else 0.0,
item.label,
),
)

parse_raw_entities call site (line ~100):

return resolve_overlaps(parsed, prefer_highest_score=True)

All other callers — apply_augmented_entities and expand_entity_occurrences — stay as resolve_overlaps(...) with no flag, so the pre-PR label alphabetical tiebreak is fully restored for mixed-source inputs. The "Mum" case is fixed, and no new detector-vs-augmenter precedence semantics are introduced in this PR.

For the follow-up issue, the provenance policy you sketched ("new stages may add coverage but must not rewrite already-adjudicated entities") is worth capturing explicitly — it would also cover the augmenter case correctly without relying on score comparison at all.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — the scoped opt-in is exactly right for this PR's scope. The current code still has -item.score in resolve_overlaps unconditionally, so the apply_augmented_entities regression remains.

Here's the complete fix:

resolve_overlaps signature + sort key:

Suggested change
-item.score,
def resolve_overlaps(entities: list[EntitySpan], *, prefer_highest_score: bool = False) -> list[EntitySpan]:
"""Resolve span conflicts by preferring longer spans, then earlier starts."""
sorted_entities = sorted(
entities,
key=lambda item: (
-(item.end_position - item.start_position),
item.start_position,
item.end_position,
-item.score if prefer_highest_score else 0.0,
item.label,
),
)

parse_raw_entities call site (line 100):

return resolve_overlaps(parsed, prefer_highest_score=True)

The other two callers — apply_augmented_entities (line 201) and expand_entity_occurrences (line 314) — stay as resolve_overlaps(...) with no flag, restoring the pre-PR alphabetical tiebreaker for mixed-source inputs.

The detector-vs-augmenter provenance policy is worth a follow-up issue, but it's out of scope here. Tracking it separately keeps this PR focused on the GLiNER label ambiguity it set out to fix.

@asteier2026 asteier2026 changed the title fix: prefer highest-scoring label when multiple GLiNER labels share the same span fix: handle multiple GLiNER labels share the same span Aug 10, 2026
@asteier2026
asteier2026 force-pushed the asteier2026/bugfix/gliner-duplicate-span-score-tiebreak branch from 5bb4fd6 to 06e5aaa Compare August 10, 2026 18:28
@asteier2026
asteier2026 requested a review from a team as a code owner August 10, 2026 18:28
@asteier2026

Copy link
Copy Markdown
Contributor Author

Changes made per Greptile suggestions

@lipikaramaswamy lipikaramaswamy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two branch-level items still need cleanup before merge:

  1. The latest commit (409fdb4) is missing a Signed-off-by trailer, so the DCO check is currently failing.
  2. The branch includes the already-merged PR #236 commit (da244c5), which causes this PR to show unrelated W&B measurement changes across eight files. Can we rebase onto the current main so this PR contains only the intended detection post-processing change and its tests?

asteier2026 and others added 3 commits August 14, 2026 09:39
…he same span

resolve_overlaps previously used alphabetical label order as a tiebreaker for
exact-span matches, causing higher-scoring labels like relationship (0.941) to
be dropped in favour of lower-scoring ones like last_name (0.719). Adding
-score before label in the sort key ensures the highest-confidence label wins.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
The score-based tiebreak (added to prefer highest-scoring GLiNER label
for same-span conflicts) caused expand_entity_occurrences to emit
propagation copies with score=1.0 over original detector spans with
lower scores, replacing their source and score metadata in final_entities.

Fix: skip creating a propagation copy when the position is already
covered by an original entity span, preserving detector provenance.
Adds regression test confirming source and score are retained.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
Make prefer_highest_score opt-in on resolve_overlaps() and enable it
only in parse_raw_entities(), where all inputs are raw GLiNER detections
with real confidence scores. All other callers (apply_augmented_entities,
expand_entity_occurrences) use the default False, restoring the pre-PR
alphabetical label tiebreaker for mixed-source inputs so synthetic
score-1.0 augmenter/propagation spans cannot displace validated detector
entities. Adds tests for the scoped flag and the augmenter non-displacement.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: asteier2026 <asteier@nvidia.com>
@asteier2026
asteier2026 force-pushed the asteier2026/bugfix/gliner-duplicate-span-score-tiebreak branch from 409fdb4 to f7c16ea Compare August 14, 2026 16:39
@asteier2026

Copy link
Copy Markdown
Contributor Author

Made suggested changes. PR #238 now has:

  • 3 clean commits on top of current main — no stray da244c5
  • prefer_highest_score scoped to parse_raw_entities only — augmenter/propagation callers use alphabetical tiebreak as before
  • Sign-offs on all commits (added via --signoff rebase)
  • 4 new tests: scoped flag behavior, default alphabetical fallback, parse_raw_entities regression with the "Mum" case, and apply_augmented_entities non-displacement

@lipikaramaswamy lipikaramaswamy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good though you'll need to run make format to merge.

Comment on lines +404 to +405
def test_augmented_entities_does_not_displace_detector_on_same_span() -> None:
"""Augmenter spans (score=1.0) must not overwrite a validated detector span at the same position."""

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def test_augmented_entities_does_not_displace_detector_on_same_span() -> None:
"""Augmenter spans (score=1.0) must not overwrite a validated detector span at the same position."""
def test_augmented_entities_does_not_use_synthetic_score_precedence() -> None:
"""Mixed-source merging retains the default label tie-break instead of comparing scores."""

Small wording suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants