Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions python/semantic_kernel/text/text_chunker.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,12 @@ def _split_text_paragraph(text: list[str], max_tokens: int, token_counter: Calla
sec_last_para = paragraphs[-2]

if token_counter(last_para) < max_tokens / 4:
last_para_tokens = last_para.split(" ")
sec_last_para_tokens = sec_last_para.split(" ")
last_para_token_count = len(last_para_tokens)
sec_last_para_token_count = len(sec_last_para_tokens)

if last_para_token_count + sec_last_para_token_count <= max_tokens:
sec_last_para = " ".join(sec_last_para_tokens) + NEWLINE
last_para = " ".join(last_para_tokens)
new_sec_last_para = sec_last_para + last_para
# Compare against `token_counter`, not `len(text.split(" "))`. Word counts and token
# counts are different units, so the old check could pass while the merged paragraph
# was over `max_tokens` -- exactly the limit this function exists to keep.
new_sec_last_para = f"{sec_last_para}{NEWLINE}{last_para}"

if token_counter(new_sec_last_para) <= max_tokens:
paragraphs[-2] = new_sec_last_para.strip()
paragraphs.pop()

Expand Down
23 changes: 23 additions & 0 deletions python/tests/unit/text/test_text_chunker.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
split_plaintext_lines,
split_plaintext_paragraph,
)
from semantic_kernel.text.text_chunker import _split_text_paragraph, _token_counter

NEWLINE = os.linesep

Expand Down Expand Up @@ -532,3 +533,25 @@ def test_split_md_on_newlines():
max_token_per_line = 15
split = split_markdown_paragraph(test, max_token_per_line)
assert expected == split


def test_short_last_paragraph_merge_respects_max_tokens():
"""Folding the last paragraph back must not push it over `max_tokens`."""
max_tokens = 20
# The default counter is len(text) // 4, so these are 20 and 4 tokens but one word each.
lines = ["a" * 80, "b" * 16]

paragraphs = _split_text_paragraph(lines, max_tokens)

assert all(_token_counter(p) <= max_tokens for p in paragraphs), [_token_counter(p) for p in paragraphs]


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]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
lines = ["a" * 20, "b" * 8]
lines = ["a" * 72, "b" * 8]


paragraphs = _split_text_paragraph(lines, max_tokens)

assert len(paragraphs) == 1
assert _token_counter(paragraphs[0]) <= max_tokens
Loading