[Bugfix] Fix Kimi-K2 tool parser concatenated tool calls parsing#24847
[Bugfix] Fix Kimi-K2 tool parser concatenated tool calls parsing#24847yiyeguhu wants to merge 7 commits into
Conversation
- Fix regex pattern to handle concatenated tool calls without spacing - Use negative lookahead to prevent over-matching across tool call boundaries - Add comprehensive test cases for edge cases including angle brackets in JSON - Consolidate test cases into parametrized test for better maintainability - Resolves GitHub issue vllm-project#24478 Signed-off-by: Thomas Mao <yiyeguhu@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in the Kimi-K2 tool parser where concatenated tool calls were not parsed correctly. The fix involves updating the regular expression to be more specific and using a negative lookahead to prevent over-matching. The changes are well-supported by new, comprehensive test cases that cover various edge scenarios.
My review includes two main points:
- A suggestion to make the main regex robust to newlines in function arguments by adding the
re.DOTALLflag. - A suggestion to improve the new test cases by using
json.dumpsfor creating expected argument strings, which enhances maintainability and ensures JSON validity, consistent with existing tests.
chaunceyjiang
left a comment
There was a problem hiding this comment.
/cc @MoyanZitto PTAL.
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run You ask your reviewers to trigger select CI tests on top of Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add If you have any questions, please reach out to us on Slack at https://slack.vllm.ai. 🚀 |
- Add re.DOTALL flag to tool_call_regex to ensure . matches newlines - Add test case for pretty-printed JSON with newlines - Addresses reviewer feedback about newline handling in function arguments Signed-off-by: Thomas Mao <yiyeguhu@gmail.com>
- Replace hardcoded JSON strings with json.dumps() calls for better maintainability - Use default json.dumps format to match actual parser output (with spaces) Signed-off-by: Thomas Mao <yiyeguhu@gmail.com>
- Improve code formatting consistency - Address pre-commit formatting requirements Signed-off-by: Thomas Mao <yiyeguhu@gmail.com>
Signed-off-by: Thomas Mao <yiyeguhu@gmail.com>
|
LGTM |
|
Why is this not being merged? |
|
any update on this? |
|
@yiyeguhu Could you fix the pre-commit issues, all other tests seem to have passed! |
|
@bbartels Could you help submit a new PR based on this one? |
@chaunceyjiang Have a look at #28831 |
|
Hi @yiyeguhu, the pre-commit checks have failed. Please run: uv pip install pre-commit
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
|
This pull request has been automatically marked as stale because it has not had any activity within 90 days. It will be automatically closed if no further activity occurs within 30 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
This pull request has been automatically closed due to inactivity. Please feel free to reopen if you intend to continue working on it. Thank you! |
[Bugfix] Fix Kimi-K2 tool parser concatenated tool calls parsing
Purpose
Test Plan
pytest tests/tool_use/test_kimi_k2_tool_parser.py
Test Result
All 11 tests pass, including 4 new test cases that validate the regex fix:
concatenated_tool_calls_bug_fix: Exact GitHub issue reproductionthree_concatenated_tool_calls: Scalability testmixed_spacing_tool_calls: Robustness testangle_brackets_in_json: Edge case with HTML in JSON