Send logprobs=True when top_logprobs is set on Chat Completions#3763
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8eb3f441e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
winklemad
force-pushed
the
fix/chatcompletions-logprobs
branch
from
d8eb3f4 to
dc4ce7d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc4ce7d974
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The Chat Completions request forwarded top_logprobs from ModelSettings but never
set logprobs, so any run with ModelSettings(top_logprobs=...) was rejected by the
API (400: 'top_logprobs' requires 'logprobs' to be set to true). The Responses
path already enables logprobs via include, so this only affected the Chat
Completions provider.
Add logprobs=True when top_logprobs is set, but only when the caller has not
already supplied logprobs via extra_args. That keeps extra_args={"logprobs": ...}
working and leaves the existing top_logprobs + extra_args["logprobs"] workaround
intact instead of colliding with the duplicate-arg check. Adds tests covering all
four combinations of top_logprobs and extra_args.
winklemad
force-pushed
the
fix/chatcompletions-logprobs
branch
from
dc4ce7d to
8ffce48
Compare
2 participants
What
The Chat Completions request builder forwards
top_logprobsfromModelSettingsbut never setslogprobs:The OpenAI Chat Completions API requires
logprobs=truewhenevertop_logprobsis set, and rejects the request otherwise:So any run using
ModelSettings(top_logprobs=...)with a Chat Completions model fails — even though the code clearly intends logprobs to work (it readsfirst_choice.logprobsfrom the response and the stream handler emits logprobs events).The Responses path is unaffected because it enables logprobs via
include(message.output_text.logprobs); this only hit the Chat Completions provider.Fix
Set
logprobs=Truewhenevertop_logprobsis provided, and omit it otherwise so ordinary calls are unchanged.Tests
Adds two tests to
tests/test_logprobs.pythat capture the outgoing Chat Completions request kwargs:ModelSettings(top_logprobs=2)→ request haslogprobs is Trueandtop_logprobs == 2top_logprobs→logprobsis omittedBoth fail on the current code (the request has no
logprobskey) and pass with the fix.ruff check,ruff format, andmypyare clean; the model test suite passes.