Skip to content

fix(tokenize): include the last character in the final sentence span - #6866

Open
uuzzrm wants to merge 1 commit into
livekit:mainfrom
uuzzrm:fix/basic-sentence-final-span
Open

fix(tokenize): include the last character in the final sentence span#6866
uuzzrm wants to merge 1 commit into
livekit:mainfrom
uuzzrm:fix/basic-sentence-final-span

Conversation

@uuzzrm

@uuzzrm uuzzrm commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

What this does

The basic (rule-based) sentence splitter returned len(text) - 1 as the end index of the final sentence, so the last character of the input was always excluded from its span. That is inconsistent with the blingfire implementation (which uses len(text) for the tail) and with the documented return contract ("start and end indices of the original text").

It is observable in the xml-aware tokenizer wrapper used for expressive TTS: with the span stopping one character short, the final period of the last sentence was remapped into its own sentence token, e.g. "<expr type='expression' label='happy'/>Hello world." became "<expr .../>Hello world" + ".".

Fix: use len(text) for the trailing sentence's end index, matching the in-loop spans.

How to test

  • pytest tests/test_tokenize_sentence_spans.py - added three regression tests: the final span covers the whole text, the final span of a multi-sentence input ends at len(text), and the xml-aware wrapper no longer splits the final period into its own token.
  • The existing test_tokenizer.py expectations (token text) are unaffected; verified against the same fixture with the public basic.SentenceTokenizer.
  • ruff check and ruff format --check pass on the changed files.

@uuzzrm
uuzzrm requested a review from a team as a code owner August 14, 2026 23:58

@devin-ai-integration devin-ai-integration Bot left a comment

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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

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.

1 participant