-
-
Couldn't load subscription status.
- Fork 10.8k
Fix a robust parsing issue in KimiK2ToolParser that causes IndexError #27565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a critical IndexError in the KimiK2ToolParser by replacing fragile parsing logic with a more robust approach. The changes include modifications in kimi_k2_tool_parser.py and updates to test assertions in test_kimi_k2_tool_parser.py to accommodate both function ID formats. The review focuses on ensuring the correctness and robustness of the new parsing logic.
| # assert tool call id format | ||
| assert actual_tool_call.id.startswith("functions.") | ||
| # assert tool call id format: should contain function name and numeric index | ||
| # Format can be either "functions.func_name:0" or "func_name:0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should accurately reflect the format being asserted. It should mention that the format can also include a function name with multiple dot separated prefixes, such as a.b.c.func_name:0.
It's important that comments are accurate and reflect the code's functionality, as they serve as documentation for others (and your future self!).
| function_name = function_id.split(".")[1].split(":")[0] | ||
| # function_id: functions.get_weather:0 or get_weather:0 | ||
| # Extract function name: split by ':' first, then take last part after '.' | ||
| function_name = function_id.split(":")[0].split(".")[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change improves the robustness of the function name extraction. However, it's crucial to ensure that this new logic doesn't inadvertently introduce any regressions or unexpected behavior with other function ID formats that might exist but weren't covered in the initial bug report. Consider adding a comment to highlight the formats that are now supported, and a test case to cover the a.b.c.func:1 format mentioned in the description.
| if current_tool_call_matches: | ||
| tool_id, tool_args = current_tool_call_matches.groups() | ||
| tool_name = tool_id.split(".")[1].split(":")[0] | ||
| tool_name = tool_id.split(":")[0].split(".")[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change improves the robustness of the tool name extraction. However, it's crucial to ensure that this new logic doesn't inadvertently introduce any regressions or unexpected behavior with other tool ID formats that might exist but weren't covered in the initial bug report. Consider adding a comment to highlight the formats that are now supported, and a test case to cover the a.b.c.func:1 format mentioned in the description.
| if current_tool_call_name_matches: | ||
| (tool_id_str,) = current_tool_call_name_matches.groups() | ||
| tool_name = tool_id_str.split(".")[1].split(":")[0] | ||
| tool_name = tool_id_str.split(":")[0].split(".")[-1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change improves the robustness of the tool name extraction. However, it's crucial to ensure that this new logic doesn't inadvertently introduce any regressions or unexpected behavior with other tool ID formats that might exist but weren't covered in the initial bug report. Consider adding a comment to highlight the formats that are now supported, and a test case to cover the a.b.c.func:1 format mentioned in the description.
f58f9d7 to
3587e33
Compare
… when function_id format doesn't match the hardcoded assumption. Signed-off-by: wangln19 <[email protected]> Signed-off-by: wangln19 <[email protected]>
3587e33 to
2de35a1
Compare
… when function_id format doesn't match the hardcoded assumption. Signed-off-by: wangln19 [email protected]
Purpose
Fix a robust parsing issue in KimiK2ToolParser that causes IndexError when function_id format doesn't match the hardcoded assumption.
Problem:
The current implementation assumes function_id is always in the format
functions.xxx:0, using fragile parsing logic:split('.')[1].split(':')[0]. This causes anIndexErrorcrash when the function_id is in the formatxxx:0(without thefunctions.prefix).Solution:
Replace the fragile parsing logic with a more robust approach:
split(':')[0].split('.')[-1]. This works correctly for both formats:functions.get_weather:0→get_weather✅get_weather:0→get_weather✅a.b.c.func:1→func✅This change follows the principle of eliminating special cases - the code now handles all format variations uniformly without conditional branches.
Changes:
kimi_k2_tool_parser.py(line 98, 247, 257)test_kimi_k2_tool_parser.pyto remove overly strict format requirements and use the same robust parsing logicTest Plan
Run existing test suite for KimiK2ToolParser:
Test Result
All existing tests pass with the new parsing logic. The updated parser now gracefully handles both function_id formats:
functions.function_name:indexfunction_name:indexThe test suite validates:
Checklist:
supported_models.mdandexamplesfor a new model.