-
Notifications
You must be signed in to change notification settings - Fork 349
fix: preserve Unicode token chunk offsets #646
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,6 +275,30 @@ def test_token_chunker_indices_complex_md(sample_complex_markdown_text: str) -> | |
| verify_chunk_indices(chunks, sample_complex_markdown_text) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "tokenizer_fixture", ["tiktokenizer", "tokenizer", "transformers_tokenizer"] | ||
| ) | ||
|
Comment on lines
+278
to
+280
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Check for any existing tokie-backed test coverage of encode_with_offsets / byte-offset conversion.
rg -n -C5 'tokie' --type=py -g '!*/node_modules/*'Repository: feyninc/chonkie Length of output: 153 Add No Python tests reference 🤖 Prompt for AI Agents |
||
| def test_token_chunker_preserves_unicode_source_offsets( | ||
| request: pytest.FixtureRequest, tokenizer_fixture: str | ||
| ) -> None: | ||
| """Test that token boundaries cannot split a multi-byte character.""" | ||
| text = "a🩺 hello world" | ||
| tokenizer = request.getfixturevalue(tokenizer_fixture) | ||
| chunker = TokenChunker(tokenizer=tokenizer, chunk_size=2, chunk_overlap=0) | ||
|
|
||
| chunks = chunker.chunk(text) | ||
|
|
||
| assert [chunk.text for chunk in chunks] == ["a", "🩺", " hello world"] | ||
| assert [chunk.token_count for chunk in chunks] == [1, 3, 2] | ||
| assert all(chunk.end_index > chunk.start_index for chunk in chunks) | ||
| verify_chunk_indices(chunks, text) | ||
|
|
||
| for batch_chunks in chunker.chunk_batch([text, text]): | ||
| assert [chunk.text for chunk in batch_chunks] == ["a", "🩺", " hello world"] | ||
| assert all(chunk.end_index > chunk.start_index for chunk in batch_chunks) | ||
| verify_chunk_indices(batch_chunks, text) | ||
|
|
||
|
|
||
| def test_token_chunker_token_counts(tiktokenizer: Encoding, sample_text: str) -> None: | ||
| """Test that the TokenChunker correctly calculates token counts.""" | ||
| chunker = TokenChunker(tokenizer=tiktokenizer, chunk_size=512, chunk_overlap=128) | ||
|
|
||
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.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Offset-supporting tokenizers now tokenize the text twice per chunk, and batching is lost for the offset path.
chunk()(Line 194) always callsself.tokenizer.encode(text)first. Then, when the tokenizer supports offsets,_chunk_tokenscallsself.tokenizer.encode_with_offsets(text)again (Line 156). Every offset-aware adapter re-runs a full tokenization internally:TiktokenAutoTokenizer.encode_with_offsetscallsself.encode(text)plus a fulldecode_with_offsets;TransformersAutoTokenizer.encode_with_offsetsandTokenizersAutoTokenizer.encode_with_offsetseach re-invoke the underlying tokenizer. The first-passtokensvalue is then only used for an equality check, not as the working token list on the offset path.This doubles tokenization cost for exactly the adapters this PR targets (tiktoken, tokenizers, transformers fast). For batch chunking it is worse:
_process_batchcallsself.tokenizer.encode_batch(texts)once (batched), then_chunk_tokenscallsencode_with_offsetsper text individually — the batching benefit ofencode_batchis lost for every text that takes the offset path.Consider restructuring so
encode_with_offsetsis tried first as the primary tokenization step, and the plainencode/encode_batchcall is only made when offsets are unavailable or fail, instead of always doing both.♻️ Illustrative restructuring (adjust call sites accordingly)
🤖 Prompt for AI Agents