[rollout] feat: improve error messages for malformed tool calls#6055
[rollout] feat: improve error messages for malformed tool calls#6055xiefan46 wants to merge 1 commit intoverl-project:mainfrom
Conversation
…gentLoop Split the catch-all exception handler in _call_tool() into specific error cases so the LLM receives actionable feedback for self-correction: - Unknown function name: lists available tools - Invalid JSON arguments: reports parse error with tool name - Tool execution failure: includes tool name in error message Added CPU unit tests that call the real _call_tool method via mock.
There was a problem hiding this comment.
Code Review
This pull request improves the error handling in the ToolAgentLoop._call_tool method by adding explicit validation for tool names and JSON arguments, providing more descriptive error messages to the agent. It also includes a new set of unit tests to verify these error paths. A potential issue was identified where an exception during the tool release process in the finally block could mask the original execution error; it is recommended to wrap the cleanup logic in its own try-except block to ensure robust error reporting.
| except Exception as e: | ||
| logger.warning(f"Error when executing tool: {e}") | ||
| return ( | ||
| ToolResponse( | ||
| text=f"Error when executing tool: {e}", | ||
| ), | ||
| 0.0, | ||
| {}, | ||
| ) | ||
| logger.warning(f"Error executing tool '{tool_name}': {e}") | ||
| return ToolResponse(text=f"Error executing tool '{tool_name}': {e}"), 0.0, {} | ||
| finally: | ||
| if tool and instance_id: |
There was a problem hiding this comment.
The except Exception as e block catches all exceptions during tool creation and execution. However, the finally block at line 470, which calls await tool.release(instance_id), is executed before the return statement in the except block. If tool.release itself raises an exception, it will propagate and suppress the original error message being returned to the LLM. It is recommended to wrap the tool.release call in its own try-except block to ensure that cleanup failures do not mask tool execution errors.
| except Exception as e: | |
| logger.warning(f"Error when executing tool: {e}") | |
| return ( | |
| ToolResponse( | |
| text=f"Error when executing tool: {e}", | |
| ), | |
| 0.0, | |
| {}, | |
| ) | |
| logger.warning(f"Error executing tool '{tool_name}': {e}") | |
| return ToolResponse(text=f"Error executing tool '{tool_name}': {e}"), 0.0, {} | |
| finally: | |
| if tool and instance_id: | |
| except Exception as e: | |
| logger.warning(f"Error executing tool '{tool_name}': {e}") | |
| return ToolResponse(text=f"Error executing tool '{tool_name}': {e}"), 0.0, {} | |
| finally: | |
| if tool and instance_id: | |
| try: | |
| await tool.release(instance_id) | |
| except Exception as release_error: | |
| logger.error(f"Failed to release tool '{tool_name}': {release_error}") |
Split the catch-all exception handler in _call_tool() into specific error cases so the LLM receives actionable feedback for self-correction:
What does this PR do?
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,veomni,sglang,vllm,vllm_omni,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,fully_async,one_step_off,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
Added CPU unit tests that call the real _call_tool method via mock.
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)recipesubmodule, please also update the reference to the submodule commit viagit submodule update --remoteorcd recipe && git pull origin main.