fix: GoogleGemini generation_config param #665
Conversation
|
@mazenkhalil Do you think you could add a quick spec? |
I have synced the whole implementation with GeminiAPI & and I have added the tests |
|
@andreibondarev Any update here? |
|
@andreibondarev Is there anything I can do to speed up the review process here? |
| parameters[:generation_config] = {temperature: parameters.delete(:temperature)} if parameters[:temperature] | ||
| parameters[:generation_config] ||= {} | ||
| parameters[:generation_config][:temperature] ||= parameters[:temperature] if parameters[:temperature] | ||
| parameters.delete(:temperature) |
There was a problem hiding this comment.
Why not change:
parameters[:generation_config][:temperature] ||= parameters[:temperature] if parameters[:temperature]
parameters.delete(:temperature)to
parameters[:generation_config][:temperature] ||= parameters.delete(:temperature) if parameters[:temperature]everywhere?
There was a problem hiding this comment.
Because we always need to delete the parameter even if it is already defined in the generation_config. Otherwise actual payload will have additional unneeded parameters. This case already covered in the specs.
| @@ -0,0 +1,25 @@ | |||
| module Langchain | |||
| module Utils | |||
| class HashTransformer | |||
There was a problem hiding this comment.
I'm wondering if we should've just yanked out the code out of the activesupport lib? This is fine, but I was wondering if it could be more elegant.
There was a problem hiding this comment.
The code here is 100% genuine, not copied from ActiveSupport in any way. In fact It is GPT generated.
A few comments, otherwise -- let's ship it! |
|
@mazenkhalil Thank you! Merged. |
Fixes #648