Skip to content

feat(types): carry optional entity indices on Relation for span attribution - #262

Open
sachinynaik wants to merge 1 commit into
superlinked:mainfrom
sachinynaik:relation-entity-index
Open

sachinynaik wants to merge 1 commit into
superlinked:mainfrom
sachinynaik:relation-entity-index

Conversation

@sachinynaik

@sachinynaik sachinynaik commented Sep 8, 2026

Copy link
Copy Markdown

Problem

Relation's head and tail are plain strings, so a consumer cannot tell which mention a relation came from when the same surface text occurs more than once in the input.

That is not cosmetic — it breaks span-based anchoring. In a measured fixture, "Dr Chen" occurs at offsets 0 and 24; a relation naming "Dr Chen" is unattributable, and a consumer that anchors relations to caller-supplied entity spans has to either guess or drop it.

The asymmetry is already visible in the types:

class Entity(TypedDict, total=False):
    text: str
    label: str
    score: float
    start: int | None      # <- offsets present
    end: int | None
    bbox: list[int] | None

class Relation(TypedDict):
    head: str              # <- bare text, no way back to a span
    tail: str
    relation: str
    score: float

Why indices rather than nested objects or duplicated offsets

ExtractOutput already carries the per-item entities list, and Entity already carries start/end. So the linkage exists on both sides and only the join is missing. Two optional integers restore it by reference:

  • No duplication of span data, so the two copies cannot drift apart.
  • No change to the type of an existing field, so every current consumer keeps working — including GLiRELAdapter, which continues to emit four keys and stays valid.
  • Nothing is required to populate them. Adapters that cannot supply indices simply omit them.

Nesting full objects under head/tail would be the alternative and is strictly worse: it changes the type of an existing field and breaks current consumers.

NotRequired rather than flipping the class to total=False — the four existing fields are required and should stay that way. NotRequired is already imported in this module.

Prior art in the ecosystem

knowledgator/gliner-relex-base-v1.0 already returns entity_idx on both the head and tail of every relation, so for that model family the information exists at the adapter boundary and is currently discarded on the way out.

Compatibility

  • No existing adapter is modified.
  • No HTTP response changes for any current model.
  • Purely additive, optional fields.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Relation responses now optionally include head and tail entity indexes.
    • Consumers can use these indexes to identify the correct entities when surface text is repeated and recover their character offsets.

A relation's head and tail are plain strings, so a consumer cannot tell
WHICH mention a relation came from when the same surface text appears
more than once in the input. That breaks span-based anchoring: in a
measured fixture "Dr Chen" occurs at offsets 0 and 24, and a relation
naming "Dr Chen" is unattributable.

Entity already carries start/end, and ExtractOutput already carries the
per-item entities list, so the linkage exists on both sides and only the
join is missing. Two optional integers restore it by reference:

  - no duplication of span data, so the copies cannot disagree
  - head/tail keep their type, so every existing consumer is unaffected
  - adapters that cannot supply indices simply omit them

NotRequired rather than total=False: the four existing fields are
required and should stay that way.

Measured on knowledgator/gliner-relex-base-v1.0, which already returns
entity_idx on both sides of each relation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sachinynaik
sachinynaik requested a review from a team as a code owner September 8, 2026 11:46
@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2d2b6264-96a7-470f-82ea-3aa00d5044b4

📥 Commits

Reviewing files that changed from the base of the PR and between 3b16ffb and 25f368b.

📒 Files selected for processing (1)
  • packages/sie_server/src/sie_server/types/responses.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The Relation response type now includes optional head_idx and tail_idx fields. These fields reference entries in the same item's entities list and support disambiguation when surface text repeats.

Changes

Relation response contract

Layer / File(s) Summary
Add relation entity indexes
packages/sie_server/src/sie_server/types/responses.py
The Relation TypedDict adds optional head_idx and tail_idx integer fields. The docstring describes their relationship to the entities list and repeated surface text.

Suggested reviewers: dragosboca

Merge Risk: ⚪ Minimal · up to 25f36

Relation responses can now optionally identify their endpoint entities by index, allowing repeated entity text to be disambiguated while remaining compatible with producers that omit the new fields. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding optional entity indices to the Relation type for span attribution.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

relation: str
score: float
head_idx: NotRequired[int]
tail_idx: NotRequired[int]

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.

Thanks for this. The problem is real and index-by-reference is the right shape: no duplicated span data, and it also links bbox-only image entities that carry no character offsets at all. Happy to take it, but it needs to land as a working slice rather than a type declaration alone. As submitted nothing produces the fields, and every boundary between an adapter and a consumer rebuilds relations from the four existing keys, so the indices cannot reach a caller.

What the PR needs to be complete:

  1. Server API layer. api/extract.py rebuilds each relation with head, tail, relation and score only. Carry head_idx and tail_idx through when present. This also keeps the direct path consistent with the queue path, which passes adapter dicts through unchanged.
  2. Published contract. Add the fields to RelationModel in types/openapi.py and to the Relation struct in sie_gateway/src/openapi.rs, then regenerate both committed specs with mise run openapi.
  3. SDKs. Both Relation types (sie_sdk/types.py, sie_ts_sdk/src/types.ts) and both parsers (client/_shared.py, internal/parsing.ts) drop the keys today.
  4. A producer. GLiREL is the natural first one. _relation_entity_text already finds the matching entity by walking zip(entities, ner_input), and the adapter echoes input entities in order, so the index into the response's entities list is well-defined. GLiNER2 will have to omit the fields because upstream returns text only, which is worth stating in the description.
  5. Tests. A GLiREL case with duplicated surface text (the "Dr Chen" fixture from the description) asserting the indices, plus coverage that the fields survive the API layer and both SDK parsers.

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