-
Notifications
You must be signed in to change notification settings - Fork 5.9k
feat: ollama tool shim #1448
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
feat: ollama tool shim #1448
Changes from 27 commits
e1b5ae9
220f926
b9b6fe8
d666bcb
73407cd
05530b3
bba2741
acfbf83
48544e3
92122b6
79aa25c
50a1a5f
dc67538
4b8805d
004d032
6877687
5b035cb
23a00e3
bbdbe66
9ad5d3a
8f76da0
374ede5
255203d
190a26c
ed61236
e6866c5
b22340d
f7f71ee
6de0be6
f0bc9f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,9 @@ use crate::message::{Message, ToolRequest}; | |
| use crate::providers::base::Provider; | ||
| use crate::providers::base::ProviderUsage; | ||
| use crate::providers::errors::ProviderError; | ||
| use crate::providers::toolshim::{ | ||
| augment_message_with_tool_calls, modify_system_prompt_for_tool_json, OllamaInterpreter, | ||
| }; | ||
| use crate::register_agent; | ||
| use crate::session; | ||
| use crate::token_counter::TokenCounter; | ||
|
|
@@ -217,7 +220,17 @@ impl Agent for TruncateAgent { | |
| tools.push(list_resources_tool); | ||
| } | ||
|
|
||
| let system_prompt = capabilities.get_system_prompt().await; | ||
| let config = capabilities.provider().get_model_config(); | ||
| let mut system_prompt = capabilities.get_system_prompt().await; | ||
| let mut toolshim_tools = vec![]; | ||
| if config.interpret_chat_tool_calls { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: it sounds like i missed a version where this was handled in the provider instead? I like that separation of concerns actually. But also this definitely works
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah the original version was implemented in each provider. Moved in this direction of putting logic into the agent as a lot of the code was just duplicated across providers to check if the shim was on, then modify the tools passed, system prompt, and interpret the response received back. It can also be nicer to have in the agent as then you don't have to modify each new provider to add the shim. |
||
| // If tool interpretation is enabled, modify the system prompt to instruct to return JSON tool requests | ||
| system_prompt = modify_system_prompt_for_tool_json(&system_prompt, &tools); | ||
| // make a copy of tools before empty | ||
| toolshim_tools = tools.clone(); | ||
| // pass empty tools vector to provider completion since toolshim will handle tool calls instead | ||
| tools = vec![]; | ||
| } | ||
|
|
||
| // Set the user_message field in the span instead of creating a new event | ||
| if let Some(content) = messages | ||
|
|
@@ -236,7 +249,15 @@ impl Agent for TruncateAgent { | |
| &messages, | ||
| &tools, | ||
| ).await { | ||
| Ok((response, usage)) => { | ||
| Ok((mut response, usage)) => { | ||
| // Post-process / structure the response only if tool interpretation is enabled | ||
| if config.interpret_chat_tool_calls { | ||
| let interpreter = OllamaInterpreter::new() | ||
| .map_err(|e| anyhow::anyhow!("Failed to create OllamaInterpreter: {}", e))?; | ||
|
|
||
| response = augment_message_with_tool_calls(&interpreter, response, &toolshim_tools).await?; | ||
| } | ||
|
|
||
| capabilities.record_usage(usage.clone()).await; | ||
|
|
||
| // record usage for the session in the session file | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,10 @@ pub struct ModelConfig { | |
| pub temperature: Option<f32>, | ||
| /// Optional maximum tokens to generate | ||
| pub max_tokens: Option<i32>, | ||
| /// Whether to interpret tool calls | ||
| pub interpret_chat_tool_calls: bool, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: i'd maybe just have tool_call_interpreter_model be an Option and it being Some/None replaces this field?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I currently have a default tool shim model, mistral-nemo set so you don't have to pass in the model. But I'm open to requiring you to pass a toolshim model also |
||
| /// Model to use for interpreting tool calls (optional) | ||
| pub tool_call_interpreter_model: Option<String>, | ||
|
ahau-square marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| impl ModelConfig { | ||
|
|
@@ -34,12 +38,20 @@ impl ModelConfig { | |
| let context_limit = Self::get_model_specific_limit(&model_name); | ||
| let tokenizer_name = Self::infer_tokenizer_name(&model_name); | ||
|
|
||
| let interpret_chat_tool_calls = std::env::var("GOOSE_TOOLSHIM") | ||
| .map(|val| val == "1" || val.to_lowercase() == "true") | ||
| .unwrap_or(false); | ||
|
|
||
| let tool_call_interpreter_model = std::env::var("GOOSE_TOOLSHIM_OLLAMA_MODEL").ok(); | ||
|
|
||
| Self { | ||
| model_name, | ||
| tokenizer_name: tokenizer_name.to_string(), | ||
| context_limit, | ||
| temperature: None, | ||
| max_tokens: None, | ||
| interpret_chat_tool_calls, | ||
| tool_call_interpreter_model, | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -96,7 +108,19 @@ impl ModelConfig { | |
| self | ||
| } | ||
|
|
||
| // Get the tokenizer name | ||
| /// Set whether to interpret tool calls | ||
| pub fn with_tool_interpretation(mut self, interpret: bool) -> Self { | ||
| self.interpret_chat_tool_calls = interpret; | ||
| self | ||
| } | ||
|
|
||
| /// Set the tool call interpreter model | ||
| pub fn with_tool_interpreter(mut self, model: Option<String>) -> Self { | ||
| self.tool_call_interpreter_model = model; | ||
| self | ||
| } | ||
|
|
||
| /// Get the tokenizer name | ||
| pub fn tokenizer_name(&self) -> &str { | ||
| &self.tokenizer_name | ||
| } | ||
|
|
@@ -142,4 +166,23 @@ mod tests { | |
| assert_eq!(config.max_tokens, Some(1000)); | ||
| assert_eq!(config.context_limit, Some(50_000)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_model_config_tool_interpretation() { | ||
| // Test without env vars - should be false | ||
| let config = ModelConfig::new("test-model".to_string()); | ||
| assert!(!config.interpret_chat_tool_calls); | ||
|
|
||
| // Test with tool interpretation setting | ||
| let config = ModelConfig::new("test-model".to_string()).with_tool_interpretation(true); | ||
| assert!(config.interpret_chat_tool_calls); | ||
|
|
||
| // Test tool interpreter model | ||
| let config = ModelConfig::new("test-model".to_string()) | ||
| .with_tool_interpreter(Some("mistral-nemo".to_string())); | ||
| assert_eq!( | ||
| config.tool_call_interpreter_model, | ||
| Some("mistral-nemo".to_string()) | ||
| ); | ||
| } | ||
| } | ||
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.
do we also need to add to
referenceandsummarizeagents?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.
thought those were just legacy at this point? I don't recall any way for folks to switch their agents?