fix(markdown): use a real tokenizer for the heading-split length check - #1394
Open
MsfPablo wants to merge 1 commit into
Open
fix(markdown): use a real tokenizer for the heading-split length check#1394MsfPablo wants to merge 1 commit into
MsfPablo wants to merge 1 commit into
Conversation
TextToEntries.tokenizer() approximates length via text.split(), which undercounts CJK text (no whitespace between words). This made the heading-split decision in MarkdownToEntries.process_single_markdown_file think CJK documents were short enough to keep as a single entry, even when they had multiple headed sections well over the token budget — degrading search quality since embeddings average the whole document. Add TextToEntries.token_count(), a tiktoken-based counter, and use it only at this specific gate. Left the existing word-split tokenizer() untouched everywhere else (including the general chunk-size splitter in split_entries_by_max_tokens), since swapping it globally would change chunk sizing for every file type and language, not just this bug. Fixes khoj-ai#1354
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.
Summary
Fixes #1354. Markdown files with CJK (Chinese, Japanese, Korean) content are indexed as a single entry instead of being chunked by heading, because the heading-split decision in
MarkdownToEntries.process_single_markdown_fileusesTextToEntries.tokenizer()(text.split()) to estimate length.text.split()relies on whitespace between words, which CJK text doesn't have, so it drastically undercounts length and the<= max_tokenscheck passes when it shouldn't — the doc never reaches the heading-splitting logic. This degrades search quality since the bi-encoder ends up averaging the semantics of the whole document.Note on approach: the issue's suggested fix was to swap
TextToEntries.tokenizer()itself for a tiktoken-based implementation. I went with a narrower fix instead —tokenizer()is also used as thelength_functionforRecursiveCharacterTextSplitterinsplit_entries_by_max_tokens, which controls chunk sizing for every content type (markdown, org, plaintext, PDF, etc.), not just this heading-split check. Swapping it globally would change chunk sizes for all users and file types, since tiktoken's subword counts don't map 1:1 totext.split()'s word counts even for English. Instead I addedTextToEntries.token_count()(tiktokencl100k_base) and used it only at the specific gate described in the issue, leaving the rest of the chunking pipeline untouched.Test plan
Verified directly against
MarkdownToEntries.extract_markdown_entries(couldn't run the full suite locally — it needs a Postgres instance for Django's test DB setup, unrelated to this change):