Python: fix paragraph merge comparing word count against token budget - #14238
Python: fix paragraph merge comparing word count against token budget#14238ErenAta16 wants to merge 1 commit into
Conversation
_split_text_paragraph folds a short trailing paragraph back into the one
before it. The guard for that merge counted words on both paragraphs
(len(text.split(" "))) and compared the sum to max_tokens, which is
measured with token_counter. Those are different units, so with few long
words the guard passed and produced a paragraph over the limit.
With the default counter (len(text) // 4) and max_tokens=20, the input
["a" * 80, "b" * 16] merged 20 + 4 tokens into a single 24-token
paragraph, because the check saw 1 + 1 = 2 words.
Build the merged paragraph first and check it with token_counter. The
split/join round-trip the old code did was an identity for single-space
splits, so dropping it changes nothing else.
There was a problem hiding this comment.
Pull request overview
Fixes paragraph “short trailing stub” merge logic in the Python text chunker by ensuring the merge decision is made using the same token-counting unit (token_counter) as the rest of the chunking code, preventing merges that would exceed max_tokens.
Changes:
- Update
_split_text_paragraphto build the merged paragraph and validate it withtoken_counterbefore applying the merge. - Remove redundant split/join word-count logic that didn’t affect the output.
- Add unit tests covering the regression case (and a control case intended to ensure merging still occurs when it fits).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/semantic_kernel/text/text_chunker.py | Fixes the last-paragraph merge guard to use token_counter on the merged candidate paragraph. |
| python/tests/unit/text/test_text_chunker.py | Adds tests for the merge behavior relative to the max_tokens budget. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| max_tokens = 20 | ||
| lines = ["a" * 20, "b" * 8] | ||
|
|
||
| paragraphs = _split_text_paragraph(lines, max_tokens) | ||
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 95%
✓ Correctness
The fix is correct and well-motivated. It replaces a word-count-based guard (comparing word counts against a token budget) with a proper token_counter measurement of the merged paragraph. The paragraph construction is semantically equivalent to the old code, and the tests appropriately cover both the rejection and acceptance paths of the merge logic.
✓ Security Reliability
This is a clean, well-scoped bug fix that corrects a unit mismatch (word count vs. token count) in the paragraph merge guard. The new code constructs the merged paragraph first, measures it with the same token_counter used everywhere else in the file, and only accepts the merge if it fits within max_tokens. No security or reliability issues are introduced. The stored paragraph is consistent with what was measured (both already-stripped inputs joined by NEWLINE, with .strip() being a no-op since neither component has leading/trailing whitespace). The tests properly validate both the fix and the continued functionality of the merge path.
✓ Test Coverage
The production fix is correct and well-motivated. The first test (
test_short_last_paragraph_merge_respects_max_tokens) properly exercises the merge-rejection path. However, the second test (test_short_last_paragraph_still_merges_when_it_fits) does NOT actually exercise the merge code path — the input lines are too short to produce separate paragraphs in the main loop, so they naturally combine without the merge logic ever firing. This means the 'control' test would pass even if the merge code were deleted entirely.
✓ Failure Modes
The fix is correct and complete. It replaces a word-count guard (comparing units of words against a budget in tokens) with the actual token_counter, preventing over-budget merges. The merged string is measured before being accepted, and the stored value (after .strip()) is always ≤ what was measured since both component paragraphs are already stripped. No silent failure paths are introduced or preserved by this change.
✓ Design Approach
I did not find a design-approach issue in this diff. The production change aligns the last-paragraph merge guard with the same
token_counter-based budget the rest oftext_chunker.pyalready uses, and the added tests capture both the regression case and the intended merge-when-it-fits behavior without conflicting with existing paragraph-format expectations.
Automated review by ErenAta16's agents
| def test_short_last_paragraph_still_merges_when_it_fits(): | ||
| """A trailing paragraph that does fit is still folded back.""" | ||
| max_tokens = 20 | ||
| lines = ["a" * 20, "b" * 8] |
There was a problem hiding this comment.
This input doesn't actually exercise the merge path. With _token_counter("a" * 20) = 5 and _token_counter("b" * 8) = 2, the paragraph-builder loop sees 5 + 2 + 1 = 8 < 20 and keeps both lines in the same current_paragraph. Only one paragraph is ever created, so the merge block is never reached. This test would pass even if the merge code were deleted.
To properly test the merge-acceptance path, the first line must be large enough to force a paragraph split. For example, ["a" * 72, "b" * 8] produces two paragraphs (18 and 2 tokens) which then successfully merge to exactly 20 tokens.
| lines = ["a" * 20, "b" * 8] | |
| lines = ["a" * 72, "b" * 8] |
Motivation and Context
_split_text_paragraphinpython/semantic_kernel/text/text_chunker.pyfolds a short trailing paragraph back into the one before it, so a chunked document doesn't end with a stub. The guard for that merge counted words:max_tokenseverywhere else in this file is measured withtoken_counter. Word counts and token counts are different units, so the guard can pass while the paragraph it produces is over the limit — which is the one thing the function exists to prevent. Text with few long words (URLs, base64, minified JSON, agglutinative languages, code identifiers) hits this immediately.The split/join dance around the merge is also dead weight:
" ".join(s.split(" "))returnssunchanged.Description
Build the merged paragraph first, then measure it with
token_counterbefore accepting it.Repro with the module default counter (
len(text) // 4) andmax_tokens=20, input["a" * 80, "b" * 16]— 20 tokens plus 4 tokens:The old check saw
1 + 1 = 2words against a budget of 20 and merged.Two tests in
python/tests/unit/text/test_text_chunker.py:test_short_last_paragraph_merge_respects_max_tokens— the case above; fails onmain(24 > 20), passes with the change.test_short_last_paragraph_still_merges_when_it_fits— control, so the merge isn't quietly disabled.["a" * 20, "b" * 8]still comes back as one paragraph.Confirmed the first test is not vacuous by stashing only
text_chunker.py:1 failed, 29 passed. With the change:30 passed.ruff checkandruff format --checkare clean on both files.Contribution Checklist