Repository navigation
Python: Keep parallel function call/result groups together in extract_range - #14488
Pujitha Paladugu (pujitha24) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A critical ordering issue remains when results are interleaved with other messages.
Review effort: Lite
Findings: 1
What changed in this PR
Updates Python extract_range to preserve parallel function-call/result groups during filtering and adds regression coverage.
Changes:
- Maps calls to all associated results.
- Preserves grouped filtering behavior.
- Adds parallel-call tests.
| File | Summary |
|---|---|
python/tests/unit/contents/test_chat_history_reducer_utils.py |
Adds regression tests for parallel-call filtering. |
python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py |
Updates call/result pairing logic; result ordering still requires correction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for paired_idx in paired_indices: | ||
| if paired_idx > idx and paired_idx in sliced: | ||
| # We'll skip the pair in the normal iteration by removing from slice | ||
| # but add it to extracted right now | ||
| extracted.append(history[paired_idx]) | ||
| sliced.remove(paired_idx) |
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
This is a community review, not a maintainer approval, so it does not clear the merge gate.
I read the full diff and traced extract_range in chat_history_reducer_utils.py by hand against all four scenarios in the new test test_extract_range_preserve_pairs_parallel_calls_filtered_result.
The fix itself: pair_map changes from call_idx -> result_idx (a single int, so a second parallel result silently overwrote the first) to call_idx -> set[result_idx], with the reverse entries (result_idx -> {call_idx}) unchanged. The keep/skip decision then checks any(filter_func(history[p]) for p in paired_indices) across every result tied to the call, and appends/removes all of them together instead of just the one that used to survive the dict overwrite. That directly fixes the root cause described in #14485: pair_map[cidx] = ridx losing every result but the last when a message has more than one FunctionCallContent.
I traced all 4 assertions in the new test against history = [user, call, result_paris, result_tokyo, done]:
- filtering only
result_tokyo: the call'spaired_indicesbecomes[2, 3], and sinceany(filter_func(...))is true for index 3, the whole group (call + both results) is skipped, giving[user, done]. Matches. - no filter: both results get appended right after the call when the loop reaches
idx=1, giving back the original list in order. Matches (this fixture happens not to have anything interleaved between the call and its results, so this doesn't exercise the separate ordering issue). - filtering the call itself: the call is skipped by the earlier
filter_funccheck before the pair block runs, but each result independently re-checks its ownpaired_indices -> filter_func(history[call_idx]), which is stillTrue, so both results are also dropped via the reverse map entry. That symmetry is what makes this work even though the call's own skip happens in a different branch than the results' skip. - call outside the range (
start=2):paired_indicesforresult_tokyofilters out index 1 (outside[2,5)), so it's treated as a standalone message and kept, giving[result_tokyo, done]. Matches.
One thing worth surfacing for discussion, not a blocker: the issue's "Expected behavior" section asked for the call to be kept together with the surviving result (call_paris kept, only the Tokyo result dropped). This PR instead drops the entire group whenever any one of the parallel results is filtered, per the "if any of them is filtered, the whole group is skipped" design in the PR description. I think that's actually the more defensible choice, since extract_range can only include or exclude whole messages, not strip a single FunctionCallContent item out of the assistant message, so keeping the call while dropping the Tokyo result would leave call_tokyo as an unanswered tool call, which most LLM provider APIs (OpenAI included) will reject as an invalid message sequence. Worth a maintainer confirming the "skip the whole group" behavior is intended for ChatHistorySummarizationReducer's use case, since it is a slightly different (stricter) outcome than the issue literally asked for.
Also noting, since this PR's own description already flags it: PR #14165 rewrites the same function for a separate reordering bug and does not touch the pair_map structure, so whichever of the two merges second will need a small rebase. No conflict in intent between the two fixes as far as I can tell.
Local validation (ruff, mypy, full contents test suite) is described in the PR body; I did not re-run it, just traced the logic and the fixture by hand.
eda11b9 to
9b3578e
Compare
9b3578e to
dd534ad
Compare
dd534ad to
63bd0b9
Compare
63bd0b9 to
9d163ed
Compare
9d163ed to
1ead17d
Compare
1ead17d to
f0eff0b
Compare
f0eff0b to
017febb
Compare
…_range Motivation: extract_range(preserve_pairs=True) built its pair lookup as a plain dict of call_index -> result_index. When one assistant message holds several parallel function calls, get_call_result_pairs() returns one pair per call sharing the same call index, so every result but the last was overwritten. As a result: - calling extract_range directly with both filter_func and preserve_pairs, and filtering out one of several parallel results, dropped the assistant tool_calls message while keeping a sibling result, leaving an orphaned TOOL message; - without a filter, the last result was emitted right after the call, so parallel results came out of history order. In-repo, ChatHistorySummarizationReducer never passes filter_func and preserve_pairs together, so the only change the reducers see is that parallel results keep their history order. Approach: Map a call index to the set of all its result indices (a result still maps to its single call). A call is kept or skipped together with all of its in-range results; if any of them is filtered, the whole group is skipped, since keeping the call with an unanswered tool_call id is also an invalid history. Kept results are appended in history order. Only indices after the current one are removed from the working slice; previously an already-processed index could be removed, shifting the slice and skipping a later, unrelated message. Validation (run from python/): - uv run pytest tests/unit/contents/test_chat_history_reducer_utils.py: the new test fails on the original code and passes with this change. - uv run pytest tests/unit/contents: all passed. - uv run ruff check / ruff format --check on changed files: clean. - uv run mypy -p semantic_kernel.contents --config-file mypy.ini: clean. Report: microsoft#14485 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-opus-5-5 (via Claude Code)
… group Per review: the keep path still appended later results right after the call and removed them from the working slice, so a history such as [call, user, result] came back as [call, result, user]. Append only the current message and let each result be appended when its own index is reached. The skip path still drops the whole group. Extends test_extract_range_preserve_pairs_parallel_calls_filtered_result with a message interleaved between a parallel call and its results. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
017febb to
3711f32
Compare

Motivation and Context
Fixes #14485.
extract_range(preserve_pairs=True)built its call/result lookup as a plaindict(call_index -> result_index). When one assistant message holds several parallel function calls, every result but the last got overwritten. So:extract_rangewith bothfilter_funcandpreserve_pairsand filtering out one parallel result dropped the assistanttool_callsmessage but kept a sibling result, which left an orphanedTOOLmessage (the repro in the issue);ChatHistorySummarizationReducernever passesfilter_funcandpreserve_pairstogether. For the reducers, the only visible change is that parallel results keep their history order.Description
tool_callid is also an invalid history.start, results behave as they did before.Added
test_extract_range_preserve_pairs_parallel_calls_filtered_result, which also covers a message interleaved between a parallel call and its results. It fails on the currentmaincode and passes with this change.Validation run locally (from
python/):uv run pytest tests/unit/contents: all passuv run ruff check/uv run ruff format --checkon changed files: cleanuv run mypy -p semantic_kernel.contents --config-file mypy.ini: cleanNote: the open PR #14165 fixes the interleaved-message reordering for single call/result pairs in the same function. This PR now also covers that ordering (per review), so the two overlap and whichever lands second will need a rebase (or can drop the overlapping part).
Contribution Checklist
🤖 Generated with Claude Code
Fixes #14485