-
-
Notifications
You must be signed in to change notification settings - Fork 180
86: Add default limit for tools completions #87
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
lib/ruby_llm/chat.rb
Outdated
@@ -105,6 +107,10 @@ def handle_tool_calls(response, &) | |||
end | |||
|
|||
def execute_tool(tool_call) | |||
raise ToolCallsLimitReachedError, "Tool calls limit reached: #{@max_tool_calls}" if max_tool_calls_reached? |
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.
It might be worth discussing if this should be handled via the chat instead of raising an unhandled error.
- Otherwise the chat object could not have 'ask' executed on again due to malformed messages
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.
changing the whole interface to chat
is a bit heavy handed. this should be a simple config change.
lib/ruby_llm.rb
Outdated
def chat(model: nil, provider: nil) | ||
Chat.new(model: model, provider: provider) | ||
def chat(model: nil, provider: nil, max_tool_completions: config.max_tool_completions) | ||
Chat.new(model: model, provider: provider, max_tool_completions: max_tool_completions) |
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.
changing the whole interface to chat
is a bit heavy handed. this should be a simple config change.
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.
Thanks for the feedback @crmne. I've adjusted things so the chat interface isn't modified, and instead, a single instance can use an override from the config using with_max_tool_completions
.
Please let me know what you think?
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.
@crmne would love an update here ... I'm getting looping tool calls as well, and would like to avoid implementing my own solution.
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.
Thank you for your work!
Left you some comments + I'm not a big fan of the naming. max_tool_calls
is less wordy. Same thing for all the instances of the name, like the error name and the documentation.
spec/ruby_llm/chat_tools_spec.rb
Outdated
{ title: Faker::Name.name, score: rand(1000) }, | ||
{ title: Faker::Name.name, score: rand(1000) }, | ||
{ title: Faker::Name.name, score: rand(1000) } |
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.
I think we can come up with a fake title instead of adding a dependency for these three lines of code.
def with_max_tool_completions(...) | ||
to_llm.with_max_tool_completions(...) | ||
self | ||
end | ||
|
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.
We now have configuration Contexts so we don't need the per-chat max tool completions method.
lib/ruby_llm/chat.rb
Outdated
@@ -11,7 +11,7 @@ module RubyLLM | |||
class Chat # rubocop:disable Metrics/ClassLength | |||
include Enumerable | |||
|
|||
attr_reader :model, :messages, :tools | |||
attr_reader :model, :messages, :tools, :number_of_tool_completions |
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.
No need to have it as attr_reader.
lib/ruby_llm/chat.rb
Outdated
def with_max_tool_completions(max_tool_completions) | ||
@max_tool_completions = max_tool_completions | ||
self | ||
end | ||
|
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.
We now have configuration Contexts so we don't need the per-chat max tool completions method.
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.
👌 Much nicer for overridding
lib/ruby_llm/chat.rb
Outdated
if max_tool_completions_reached? | ||
raise ToolCallCompletionsLimitReachedError, "Tool completions limit reached: #{@max_tool_completions}" | ||
end | ||
|
||
@number_of_tool_completions += 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.
Shouldn't this be at the top of the method? Say max_tool_completions
is 0, that would mean that we should process 0 tool completions, right?
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.
Thanks for the review @crmne, you're right, this should have been before the max_tool_completions_reached? check. I've amended this now along with your other feedback.
I haven't manually tested the new changes yet, but the test cases are passing for the providers I have keys for.
Please let me know what you think.
if context.config | ||
@config = context.config | ||
@max_tool_llm_calls = @config.max_tool_llm_calls | ||
end |
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.
Wrapped this in a conditional to ensure that the config was present on the context. This more closely aligns with the assignment in the initialiser (@config = context&.config || RubyLLM.config
)|
If I've made a mistake here, please let me know.
Resolves #86
Purpose
Introduces a configurable limit on tool completions to prevent infinite loops and excessive API usage. This feature adds protection against scenarios where AI responses might trigger continuous tool calls.
Implementation Details
max_tool_llm_calls
configuration option (default: 25 calls)ToolCallLimitReachedError
when limit is exceedednumber_of_tool_completions
counternil
Usage Example
Testing
Documentation
TODO