-
Notifications
You must be signed in to change notification settings - Fork 14
fix: handle multiple GLiNER labels share the same span #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
asteier2026
wants to merge
3
commits into
main
Choose a base branch
from
asteier2026/bugfix/gliner-duplicate-span-score-tiebreak
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
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 syntheticscore=1.0propagation 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 documentedfinal_entitiesprovenance, 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.There was a problem hiding this comment.
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):Every original span appears twice in
all_entities— once as the detector entity with its real score/source, and once as a propagation copy withscore=1.0. With-item.scorein the sort key, the propagation copy always wins the tiebreak on identical spans, silently replacingentity_id,score, andsourceon the original detection positions. That corrupts the provenance trail even when the label is identical.The simplest scoped fix is to add a
sourcepriority tier to the sort key before the score, so detector entities beat synthetic ones on equal spans: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_occurrencesretains the originalentity_id,score, andsourceat the detected position would also be a strong addition here.There was a problem hiding this comment.
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 fromparse_raw_entities(), where every candidate is a directly comparable GLiNER detection.Then use:
Other callers—particularly
apply_augmented_entities()andexpand_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 extendtest_expand_preserves_original_entities()to assert the originalentity_id,score, andsource, not only the values.This is narrower than adding a global source-priority policy, which would introduce new precedence semantics unrelated to this bug.
There was a problem hiding this comment.
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 aresource="detector"with real GLiNER scores. Bothapply_augmented_entities()(line 201) andexpand_entity_occurrences()(line 314) mix in syntheticscore=1.0entities before callingresolve_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), using0.0as the constant means theitem.labeltiebreaker 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:
parse_raw_entities()with a shared span — two GLiNER detections at identical positions, higher-score label wins:expand_entity_occurrences()retains original detector entity — the propagation copy at the original span must not displace the detector entity: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_overlapsto knowledge of synthetic source names.There was a problem hiding this comment.
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:
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.There was a problem hiding this comment.
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_occurrenceswas fixed by commit 409fdb4 (theif (start, end) in original_positions: continueguard), butapply_augmented_entitiesstill passes a mixed detector+augmenter list to the unscopedresolve_overlaps, so augmenter spans atscore=1.0still 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:2. Enable it only in
parse_raw_entities(line 100):All other callers —
apply_augmented_entitiesandexpand_entity_occurrences— keep the defaultFalse, so the label tiebreaker restores the pre-PR behavior for mixed-source inputs. With your example,email(detector) would win overlast_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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 syntheticscore=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
There was a problem hiding this comment.
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_overlapssignature change:parse_raw_entitiescall site (line ~100):All other callers —
apply_augmented_entitiesandexpand_entity_occurrences— stay asresolve_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.
There was a problem hiding this comment.
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.scoreinresolve_overlapsunconditionally, so theapply_augmented_entitiesregression remains.Here's the complete fix:
resolve_overlapssignature + sort key:parse_raw_entitiescall site (line 100):The other two callers —
apply_augmented_entities(line 201) andexpand_entity_occurrences(line 314) — stay asresolve_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.