Set default penalty params to null when unspecified #37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
estohlmann
reviewed
Jun 20, 2024
| // LangChain's JS OpenAI client sets default values that we do not want if we didn't set them. | ||
| // The following parameters are preferred to be null instead of having a numeric default value | ||
| // as they degrade the quality of the model response if set incorrectly by default. | ||
| if (!modelConfig?.frequency_penalty && modelConfig?.frequency_penalty !== 0) { |
There was a problem hiding this comment.
I sent you a patch file but what I would suggest doing is pulling the properties that we want to null out by defualt out into the modelKwargs property of the modelConfig. These properties will be defaulted to null when passed into modelKwargs and you will get the behavior you are replicating here in a much cleaner way.
There was a problem hiding this comment.
Awesome suggestion- doing that for the next set of changes. I left the number of tokens alone because those are usually required at the client level, but this is a much nicer approach and we don't need any other custom logic.
estohlmann
approved these changes
Jun 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The LangChain JS library sets some default values for presence and frequency penalties if we don't set them as part of the OpenAI client. As a result, we end up with a non-required numeric value (0) that we did not intend to put there, which heavily degrades the model responses on tgi containers. These changes override the defaults set by the client so that the values are either the numeric value we set, including 0, otherwise they will be null. I validated that this happens by setting and unsetting the values in the model kwargs of the Chat UI and checked the request data from the network tab of my browser. Now, by default, these values won't be set unless we want them to be.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.