-
-
Notifications
You must be signed in to change notification settings - Fork 225
Improve RubyLLM::Chat#with_params
(#265) by allowing to override default params
#303
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
…de default params
@MichaelHoste I agree that it would be great to have some way to "dangerously" mutate the params after the payload! FYI the merge order (merge payload after params) I implemented in #265 came directly due to @crmne 's comment #130 (review) on an earlier PR that had it like yours (merge params after payload):
So I think any proposed solution here has to address that comment if it's going to get merged. My hope with proposing the block format: chat = RubyLLM
.chat(model: "gpt-4o-search-preview", provider: :openai)
.with_params(web_search_options: {search_context_size: "medium"}) do |payload|
payload.delete(:temperature)
end
chat.ask(...) is that the block style would be considered to be a "power user feature" enough that it's not an easy footgun, compared to just passing in keyword arguments. Another alternative might be that But I'd love to see other variations. There may be a much simpler way that makes everybody happy here. 😄 |
The hacky, monkeypatch way to set context = RubyLLM.context # Important: must use a new context to ensure a separate RubyLLM::Connection is used for this Chat!
chat = RubyLLM
.chat(model: "claude-3-5-haiku-20241022", provider: :anthropic, context: context)
### BEGIN INSTANCE MONKEYPATCH
class << chat
attr_reader :connection
end
class << chat.connection
attr_reader :responses
def post(url, payload, &)
puts "payload before: #{payload.inspect}"
payload[:max_tokens] = 4
puts "payload after: #{payload.inspect}"
super(url, payload, &)
end
end
### END INSTANCE MONKEYPATCH
chat.ask("What is the capital of France?")
payload before: {model: "claude-3-5-haiku-20241022", messages: [{role: "user", content: [{type: "text", text: "What is the capital of France?"}]}], temperature: 0.7, stream: false, max_tokens: 8192}
payload after: {model: "claude-3-5-haiku-20241022", messages: [{role: "user", content: [{type: "text", text: "What is the capital of France?"}]}], temperature: 0.7, stream: false, max_tokens: 4}
=> #<RubyLLM::Message:0x000074ea7f5682c0
@content=#<RubyLLM::Content:0x000074ea8005ce38 @attachments=[], @text="The capital of France">,
@input_tokens=14,
@model_id="claude-3-5-haiku-20241022",
@output_tokens=4,
@role=:assistant,
@tool_call_id=nil,
@tool_calls=nil> |
Thanks for this precision @compumike, i completely missed it when reading your PR, sorry about that! I have mixed feelings about the "block" approach. My main issue is that for some models, this would work: RubyLLM
.chat(model: "gpt-4o-mini-2024-07-18", provider: :openai)
.with_params(max_tokens: 40) # or even max_completion_tokens But with others, where RubyLLM
.chat(model: "claude-3-5-haiku-20241022", provider: :anthropic)
.with_params do |payload|
payload.merge!(max_tokens: 40)
end Since the user doesn't know exactly what is in the final payload, the behavior could be unexpected for other similar scenarios. Since there are very few parameters that should be protected at any cost. Wouldn't a more straightforward solution like this be preferable to avoid any footguns? PROTECTED_PARAMS = %w(model tools messages stream) # maybe more?
protected_params = params.keys.map(&:to_s) & PROTECTED_PARAMS
raise ArgumentError, "Protected parameters: #{protected_params.join(', ')}" unless present_params.empty? |
The (I do wonder if there are any use cases where it should be applied in a "deep" way to protect |
Perhaps this is for a different topic, but locally I made the following change:
The changed part is the support for merging array values. This was because you cannot currently use |
What this does
This PR slightly improves what #265 does by allowing the override of default parameters.
I just inverted the order of the
deep_merge
between payload and params, refactored theUtils
method to make it more generic and added an Anthropic test.This small PR solves many issues that we encountered:
max_tokens
is mandatory in the Anthropic API, and this gem sets it as the maximum tokens allowed by the model, but sometimes we want to force a lower value.We use this gem for Mistral by setting
openai_api_base
ashttps://api.mistral.ai/v1
because it's almost compatible. The only issue we encountered was that Mistral doesn't support thestream_options: { include_usage: true }
parameter that is always added to the request for OpenAI. Now we can just add.with_params(stream_options: nil)
to make it compatible.payload.compact!
also clean the extra param when we want to use the default model temperature withwith_temperature(nil)
My intuition is that it could help other people fix similar situations.
@compumike suggested this, but I feel that this PR is even simpler:
Type of change
Scope check
Quality check
overcommit --install
and all hooks passmodels.json
,aliases.json
)API changes
Related issues
Related to #265