-
-
Notifications
You must be signed in to change notification settings - Fork 190
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?
Changes from 14 commits
e7f7a0f
47c8742
190e9de
fe837de
0f7ce26
6b807cf
df12db9
3af500d
39894a8
4718e2d
7885c4b
585af65
df26988
19c0cd1
3ad25f4
d94d6dd
71dba53
6455f08
5a44bff
17b55cc
d09ce92
69af539
018cc3b
4cb6765
6cb8ec3
4cdb924
f8fc8a5
731fcc6
092ff2c
a0a1fa8
397438e
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 |
---|---|---|
|
@@ -11,9 +11,9 @@ module RubyLLM | |
class Chat | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. No need to have it as attr_reader. |
||
|
||
def initialize(model: nil, provider: nil) | ||
def initialize(model: nil, provider: nil, max_tool_completions: nil) | ||
model_id = model || RubyLLM.config.default_model | ||
with_model(model_id, provider: provider) | ||
@temperature = 0.7 | ||
|
@@ -23,9 +23,13 @@ def initialize(model: nil, provider: nil) | |
new_message: nil, | ||
end_message: nil | ||
} | ||
@max_tool_completions = max_tool_completions | ||
@number_of_tool_completions = 0 | ||
end | ||
|
||
def ask(message = nil, with: {}, &block) | ||
@number_of_tool_completions = 0 | ||
|
||
add_message role: :user, content: Content.new(message, with) | ||
complete(&block) | ||
end | ||
|
@@ -108,6 +112,11 @@ def handle_tool_calls(response, &) | |
@on[:end_message]&.call(message) | ||
end | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be at the top of the method? Say 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. 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. |
||
complete(&) | ||
end | ||
|
||
|
@@ -124,5 +133,11 @@ def add_tool_result(tool_use_id, result) | |
tool_call_id: tool_use_id | ||
) | ||
end | ||
|
||
def max_tool_completions_reached? | ||
return false unless @max_tool_completions | ||
|
||
@number_of_tool_completions >= @max_tool_completions | ||
end | ||
end | ||
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.
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.