fix(chunking): measure combine thresholds in tokens when max_tokens is set - #4528
Open
joaquinhuigomez wants to merge 1 commit into
Open
joaquinhuigomez wants to merge 1 commit into
joaquinhuigomez wants to merge 1 commit into
Conversation
`PreChunk.can_combine()` sized its text with `len()` and compared the result to `combine_text_under_n_chars` and to `hard_max`. Both of those are token counts when `max_tokens` is given, so the comparison was characters against a token budget. Every sibling computation in the class -- `.will_fit()`, `._text_length`, `_TextSplitter` -- already routes through `ChunkingOptions.measure()`. A section of a few tokens is many more characters than the token budget, so `can_combine()` answered `False` for every short pre-chunk and `chunk_by_title()` emitted one chunk per section. With `max_tokens=60`, twelve ten-token sections came out as twelve chunks at 17% of the requested size, where `chunk_elements()` on the same elements produced two full ones. Raising `combine_text_under_n_chars` is not a way around it: values above `max_tokens` are rejected by option validation. Send both measurements through `measure()`. Character mode is unaffected, because `measure()` is `len()` there; the existing character-mode `can_combine()` cases still pass unchanged, and chunk output over a 3,000-case character-mode corpus is byte-identical.
This branch has not been deployed
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
1 participant
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.
PreChunk.can_combine()sizes its text withlen(), so thecombine_text_under_n_charsthreshold and the hard-max check count characters even whenmax_tokenshas selected token counting. Every sibling computation in the class —will_fit,_text_length— goes throughChunkingOptions.measure(), which branches on the counting mode;can_combineis the one that doesn't. In token mode a section of a few tokens is many more characters than the token budget, so every section looks too large to combine and becomes its own chunk. Withmax_tokens=60,chunk_elementsfills the window whilechunk_by_titleproduces twelve ten-token chunks — about 17% utilization — and the obvious workaround,combine_text_under_n_chars=240, is rejected because it exceeds the hard max.The fix is the two
measure()substitutions. Character-mode behavior is unchanged (measure()islen()there): a 3,000-chunk-list corpus across both chunkers and five option sets hashes identically before and after, and the existing character-modecan_combinecases pass untouched.Tests: a deterministic offline token counter fixture (no tiktoken download), a token-mode
can_combineparametrization mirroring the character-mode sibling, achunk_by_titlecase asserting the token budget is filled and the chunk texts equalchunk_elements()on the same input, and a discriminating character-mode case that would flip if the measure were swapped. The first two fail onmain.test_unstructured/chunking/test_base.py+test_title.py: 391 passed, 16 skipped; ruff clean. CHANGELOG entry under a new0.27.18section with the matching__version__bump, following the one-patch-per-PR pattern of the recent entries.Related but different: #4487 changes what the
combine_text_under_n_charsthreshold is (caps it atnew_after_n_chars); this changes which unit it's compared in. No textual overlap.