fix: keep inter-sentence whitespace when a sentence ends with a closing quote - #12431
Open
winklemad wants to merge 1 commit into
Open
Conversation
…ng quote With keep_white_spaces=True the custom Punkt pattern only allowed closing brackets after a sentence ending, so for `He said "Hi." Bye.` the whitespace sits behind the closing quote and the pattern cannot reach it. The space ends up in none of the returned spans: it is dropped from the chunk text and shifts the split_idx_start offsets of every following chunk. Match closing quotes as well, and compare the split rules against the end of the sentence itself, since a span now carries the trailing whitespace.
|
@winklemad is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
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
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.
Related Issues
Proposed Changes:
SentenceSplitter(keep_white_spaces=True)silently drops the whitespace between two sentences when the first one ends with a closing quote:Root cause.
CustomPunktLanguageVars.period_context_reextendsSentEndCharswith closing brackets only (self._re_sent_end_chars + r"[\)\]}]*"). For."the whitespace sits behind the closing quote, so the\s*in_period_context_fmtnever reaches it; nltk's boundary realignment then ends the sentence at the quote but moves the next break past the whitespace, and those characters end up in none of the returned spans.He said (hi.) Then left.round-trips correctly today, which is exactly the bracket case that is already covered — closing quotes are simply missing from that character class.Impact. The characters are lost from the chunk text and every following
split_idx_startshifts, so chunks no longer index back into the source. This reachesDocumentSplitter(split_by="sentence", andsplit_by="word"withrespect_sentence_boundary=True),RecursiveDocumentSplitter,MarkdownHeaderSplitterandEmbeddingBasedDocumentSplitter— any prose with a quoted sentence. It also breaks the assumption stated inembedding_based_document_splitter.pythat "chunks are contiguous substrings of the original text ... so the character offset is simply accumulated".Fix. Match closing quotes as well as closing brackets after a sentence ending. Because a span now carries the trailing whitespace,
_needs_joincompares the split rules against the end of the sentence itself (text[start:end].rstrip()); without that line the existing "a cited question is not a sentence boundary" rule would silently stop firing, so there is a test guarding it.Chunk boundaries change for text containing quoted sentences, so re-indexing an existing corpus produces different chunks — this is called out in the release note.
I deliberately did not touch
recursive_splitter.py: the overlap problem in #12281 / #12284 has a different root cause and is already being worked on.How did you test it?
Unit tests, added first and confirmed failing on unmodified code (7 failed, 2 passed), then passing with the fix (9 passed):
test_split_sentences_keeps_white_spaces_after_a_closing_quote— parametrized over",',“ ”,‘ ’,« »and!", asserting no character is lost and that the spans still tile the text. It includes the bracket caseHe said (two.) Three.as a control that already passed before the fix.test_split_sentences_keeps_a_cited_question_joined— guards the existing split rule for cited questions.test_run_split_by_sentence_quoted_text_keeps_offsets_aligned— checks atDocumentSplitterlevel that everysplit_idx_startstill indexes its own chunk in the source text.Also ran
hatch run test:unit test/components/preprocessors/(367 passed, 358 before the new tests), the fullhatch run test:unit,hatch run test:types(clean) andhatch run fmt. Since the pattern is version-sensitive, I re-ran the preprocessor tests against the declared floornltk==3.9.1as well — 73 passed there too.Notes for the reviewer
) ] } " ' ’ ” ». If apostrophe-heavy languages are a concern I'm happy to drop'from it — the remaining characters still fix the common."and”cases, just say the word and I'll push the change.re_boundary_realignmentalso lists the opening‘ “ «; including them made no difference on any case I tried, so I left them out._needs_joinis required by the fix, not a drive-by: it keeps the existing quote/numeration rules working now that a span can end with whitespace.Checklist
fix:.