Hello @CatherineSue, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

Hello, I'm Gemini, your friendly code assistant. I've reviewed this pull request by CatherineSue which focuses on improving the tool call detection logic, specifically for the Pythonic format detector. The primary goals are to correctly handle the tool_index when parsing Pythonic tool calls and to fix an issue where the detector failed to find tool calls if they were embedded within other text (mixed output) in non-streaming scenarios. The changes involve modifying the PythonicDetector to use regex search instead of match, allowing it to find tool calls anywhere in the input string, and adjusting how the tool_index is determined for Pythonic calls. Additionally, a helper method in the base detector class was made private.

Highlights

• Pythonic Detector Improvements: The PythonicDetector's detect_and_parse method is updated to correctly extract tool calls even when they are surrounded by other text. This is achieved by switching from regex.match (which only matches from the start of the string) to regex.search (which finds the pattern anywhere).
• Tool Index Calculation: For Pythonic tool calls, the tool_index in the resulting ToolCallItem is now set to the index of the call within the parsed list of calls, rather than attempting to map the function name to an index from the provided tools list. This aligns with how Pythonic tool calls are typically structured as a list of function calls.
• Undefined Function Handling: A check is added in the PythonicDetector to log a warning if the model attempts to call a function name that is not present in the list of provided tools.
• Internal Method Visibility: A helper method ends_with_partial_token in the base detector class is renamed to _ends_with_partial_token to indicate it's intended for internal use.

Changelog

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

A list of calls,
In Python's guise,
Parsed from text,
Before your eyes.
No longer matched, but searched.

Code Review

This pull request aims to fix two main issues: the tool_index in PythonicDetector for streaming mode and the handling of mixed text/tool_call outputs in detect_and_parse for non-streaming scenarios. The changes in PythonicDetector are significant and generally move in the right direction, especially the use of regex.search for more flexible tool call detection and the updated logic for tool_index.

I've identified a critical issue regarding a potential AttributeError if the regex search doesn't find a match, and a couple of medium-severity suggestions related to error handling and test coverage. Addressing the critical issue is essential before merging.

Summary of Findings

• Potential AttributeError in PythonicDetector: A critical issue was found in python/sglang/srt/function_call/pythonic_detector.py where match.start() and match.end() could be called on a None object if self.tool_call_regex.search(text) doesn't find a match. This needs to be addressed to prevent runtime errors.
• Normal Text Handling in Error Cases: In PythonicDetector.detect_and_parse, when a tool call is matched by regex but subsequent AST parsing or other processing fails, the method currently returns the original input string as normal_text. It's suggested to consider returning the already-parsed normal_text (text surrounding the attempted tool call) to better reflect the parsing progress.
• Test Coverage for detect_and_parse: The PR description mentions fixing issues with mixed outputs in detect_and_parse. While new tests are added, dedicated unit tests for PythonicDetector.detect_and_parse specifically covering various mixed-content scenarios (e.g., text before tool call, text after, text both before and after, text only, tool call only) would be beneficial to directly validate this aspect of the fix.
• Method Renaming for Clarity: The renaming of ends_with_partial_token to _ends_with_partial_token in base_format_detector.py is a good practice, aligning with PEP 8 conventions for internal helper methods. This was a low severity finding and not commented on directly due to review settings.
• Tool Index Logic: The change in PythonicDetector to set ToolCallItem.tool_index to call_index (the index of the call within the response's list of calls) is a clear and intentional change, aligning with the PR's goal. This was a low severity finding and not commented on directly due to review settings.

Merge Readiness

This pull request makes important improvements to tool call parsing. However, there is a critical issue identified (potential AttributeError) that must be resolved before merging. Additionally, there are a couple of medium-severity suggestions regarding error handling and test coverage that would improve the robustness and maintainability of the code. I recommend addressing these points, especially the critical one. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by team members once the changes are made.