feat: add support for 'presence_penalty' param to Responses API#4830
feat: add support for 'presence_penalty' param to Responses API#4830nathan-weinberg wants to merge 2 commits intollamastack:mainfrom
Conversation
Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
✱ Stainless preview buildsThis PR will update the Edit this comment to update it. It will appear in the SDK's changelogs. ✅ llama-stack-client-node studio · code · diff
⏳ These are partial results; builds are still running. This comment is auto-generated by GitHub Actions and is automatically kept up to date as you push. |
nathan-weinberg
requested review from
ashwinb,
bbrowning,
cdoern,
ehhuang,
franciscojavierarceo,
leseb,
mattf and
raghotham
as code owners
| max_output_tokens=max_output_tokens, | ||
| metadata=metadata, | ||
| include=include, | ||
| presence_penalty=presence_penalty, |
There was a problem hiding this comment.
This seems also need to be propagated to chat completion api.
There was a problem hiding this comment.
meta-cla
bot
added
the
CLA Signed
skamenan7
left a comment
skamenan7
left a comment
There was a problem hiding this comment.
Thanks Nathan. Looks good.
| request.max_output_tokens, | ||
| request.reasoning, | ||
| request.metadata, | ||
| request.presence_penalty, |
There was a problem hiding this comment.
This call is using a long positional-arg list. With the recent addition of presence_penalty, it looks like reasoning and max_output_tokens are now swapped due to parameter order. Can we switch this call to keyword arguments (at least for the tail params)
There was a problem hiding this comment.
FYI: I was trying to fix the order issue at #4825 (review)
| @@ -171,6 +172,7 @@ def __init__( | |||
| self.store = store | |||
| self.include = include | |||
| self.store = bool(store) if store is not None else True | |||
There was a problem hiding this comment.
Tiny nit: self.store gets assigned twice in __init__ (first self.store = store, then immediately overwritten by the bool(store) normalization).
Not blocking, but worth cleaning up while we’re in here.
|
This pull request has merge conflicts that must be resolved before it can be merged. @nathan-weinberg please rebase it. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork |
3 participants
What does this PR do?
Closes #4704
Test Plan
4 unit tests have been added for testing this. Happy to provide additional tests/scripts if desired.