-
Notifications
You must be signed in to change notification settings - Fork 149
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
feat: add repetition_penalty and top_k to openai #288
feat: add repetition_penalty and top_k to openai #288
Conversation
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 this PR! Just a couple suggestions to better align with OpenAI's format.
@@ -455,6 +455,8 @@ struct ChatCompletionRequest { | |||
// Additional parameters | |||
// TODO(travis): add other LoRAX params here | |||
response_format: Option<ResponseFormat>, | |||
repetition_penalty: Option<f32>, |
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.
Looks like the OpenAI spec defines this as presence_penalty
: https://platform.openai.com/docs/api-reference/chat/create
Since there is no top_k
equivalent in OpenAI, adding it as a new param is fine.
@@ -587,8 +591,8 @@ impl From<CompletionRequest> for CompatGenerateRequest { | |||
api_token: None, | |||
best_of: req.best_of.map(|x| x as usize), | |||
temperature: req.temperature, | |||
repetition_penalty: None, | |||
top_k: None, | |||
repetition_penalty: req.repetition_penalty, |
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.
If we map repetition_penalty
to the OpenAI presence_penalty
, we need to shift the range from (-2, 2) (OpenAI) to (0, 4) ours, so something like:
repetition_penalty: req.presence_penalty.map(|x| x + 2.0)
Same for lin 629.
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.
Hi @tgaddair As I known, presence_penalty and repetition_penalty both have the same effect. But
- presence_penalty - Between -2.0 and 2. value 0 means no penalty. Default to 0.0
- repetition_penalty – Between 1.0 and infinity. 1.0 means no penalty. Default to 1.0.
So it not have same range, and presence_penalty=0 (no penalty) not have same effect with repetition_penalty=2 (much penalty)
Do you think we need keep it seprately or any better method to shift it ?
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 recommend keep both params presence_penalty and repetition_penalty.
Or if you still want to reuse presence_penalty
, I think repetition_penalty: req.presence_penalty.map(|x| x + 1.0)
is better.
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.
Hmm, maybe it's fine to keep both for now. We can think about how best to map presence_penalty to repition_penalty in a follow-up. Thanks for the PR!
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.
@tgaddair thank u
Fixes #287.