fix: named tool choice should filter tool calls.#7808
fix: named tool choice should filter tool calls.#7808huitianbai wants to merge 3 commits intoai-dynamo:mainfrom
Conversation
|
👋 Hi huitianbai! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
WalkthroughModified tool visibility handling in OpenAI prompt template rendering. Added a new helper function to filter tools by function name and updated the render method to conditionally strip, keep, or filter tools based on the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/llm/src/preprocessor/prompt/template/oai.rs (1)
448-473: Add regression tests for namedtool_choicefiltering path.The new named-choice branch is the core behavior change, but current tests near Line 1403-1437 only cover
"none"and"auto". Please add cases for:
- named function that exists (tools filtered to 1), and
- named function that does not exist (tools omitted / expected fallback behavior).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/template/oai.rs` around lines 448 - 473, Add regression tests covering the named `tool_choice` branch so the new filtering path is exercised: write tests that call the code path which uses req.tool_choice() and the logic in the block that may call filter_tools_by_name(name_str) — one test where the named function exists (assert tools list is filtered to the single matching tool) and one where the named function does not exist (assert expected fallback/omitted behavior, e.g., original tools or None per current logic). Target the behavior controlled by exclude_tools_when_tool_choice_none and the variable tools, invoking the same code paths currently covered for "none" and "auto" to ensure parity for the named-case branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/llm/src/preprocessor/prompt/template/oai.rs`:
- Around line 448-473: Add regression tests covering the named `tool_choice`
branch so the new filtering path is exercised: write tests that call the code
path which uses req.tool_choice() and the logic in the block that may call
filter_tools_by_name(name_str) — one test where the named function exists
(assert tools list is filtered to the single matching tool) and one where the
named function does not exist (assert expected fallback/omitted behavior, e.g.,
original tools or None per current logic). Target the behavior controlled by
exclude_tools_when_tool_choice_none and the variable tools, invoking the same
code paths currently covered for "none" and "auto" to ensure parity for the
named-case branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 441a411a-c4fa-4dd1-b807-96dd0318a735
📒 Files selected for processing (1)
lib/llm/src/preprocessor/prompt/template/oai.rs
22f6319 to
f45f7a5
Compare
f45f7a5 to
7794aaa
Compare
cbbe1f8 to
f730fd4
Compare
|
@ishandhanani Hi, please review if you have time. |
|
|
||
| fn filter_tools_by_name(origin_tools: Value, name: &str) -> Option<Value> { | ||
| let mut filtered_tools = Vec::new(); | ||
| let tools = serde_json::to_value(origin_tools).unwrap(); |
There was a problem hiding this comment.
We can't have .unwrap() as that would panic and stop the server.
If you're 100% certain that it will never panic, add // Safety: ...why it never panics ... above the line.
| let request: NvCreateChatCompletionRequest = serde_json::from_str(json_str).unwrap(); | ||
| let name = "get_weather"; | ||
| let tools = request.tools().unwrap(); | ||
| let result = serde_json::to_value(filter_tools_by_name(tools, name)).unwrap(); |
There was a problem hiding this comment.
Could you change the test to call filter_tools? That way both get tested.
Signed-off-by: baihuitian <baihuitian.bht@gmail.com>
f730fd4 to
129fbfb
Compare
|
Hi ! |
@vladnosiv Hi, I checked the OpenAI API, it seems having a special "allowed_tools" to maximize savings prompt caching. A named tool choice needs select one. I read this : https://developers.openai.com/api/docs/guides/function-calling#:~:text=When%20to%20use-,allowed_tools,-You%20might%20want |
As a supplement of #7589
When a request given multiple tool functions and a named tool choice, we should only consider the matched one.
For instance a request with :
"tools": [
{
"type": "function",
"function": {
"name": "get_weather",
...
}
},
{
"type": "function",
"function": {
"name": "get_location",
...
}
},
],
"tool_choice": {
"type": "function",
"function": {"name": "get_weather"}
},
....
the input prompt should only use the "get_weather function".
I implement a filter_tools function
closes: #7851