Validate logprobs range and pin top-k logprobs round-trip - #455
Draft
Ranoobaba wants to merge 1 commit into
Draft
Conversation
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
Open
logprobs must be 0 to 20, but out of range values were only caught by the API. Reject them in the request validator, document the range in the create() docstrings, and add a test that top_logprobs survives model_dump() (togethercomputer#251, togethercomputer#443). Addresses togethercomputer#251.
Ranoobaba
force-pushed
the
fix/logprobs-topk-251
branch
from
July 21, 2026 02:01
e3e138c to
3bdd9c1
Compare
This file contains hidden or 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
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.
Have you read the Contributing Guidelines? Yes.
Issue #251
Describe your changes
What and why. Issue #251 asks how to get log probabilities for several top-k alternative tokens, not just the sampled one. The API supports this today:
logprobsaccepts an integer between 0 and 20, described in the API reference as "the number of top tokens to return log probabilities for at each generation step, instead of only the sampled token." The SDK made it hard to discover and easy to misuse:create()methods (chat and completions, sync and async).logprobs=50was sent straight to the API and surfaced only as an opaque HTTP error after the request.top_logprobsalternatives returned by the API actually survive response parsing andmodel_dump().Fix decisions.
ChatCompletionRequestandCompletionRequestnow rejectlogprobsoutside[0, 20]inside the existingverify_parameters@model_validator, with an error that explains what the parameter does. This follows the repo's own precedent: parameter checks live in that validator, and invalid user input raisesValueErrorelsewhere in the SDK (resources/evaluation.py,resources/code_interpreter.py). Because the validator runs on the request model, one change covers the sync client, the async client, and the CLI's--logprobsflag.logprobsdocstring in all fourcreate()methods now documents the semantics and the range, using the same "Must be in the range [a, b]." phrasing the file already uses forpresence_penalty,frequency_penalty,min_p, andlogit_bias.response.choices[0].logprobs.top_logprobs, which is a list with one{token: logprob}dict per generated token as captured from the live API inLogProbs.top_logprobstyped asDictbut API returnsList[Dict]#443 and Get Log Probs #251, round trips through parsing andmodel_dump()with no Pydantic serialization warnings. Today this works because response models useextra="allow". The test guards the accessor path against the mistyping failure mode reported inLogProbs.top_logprobstyped asDictbut API returnsList[Dict]#443, wheretop_logprobsdeclared asDict[str, float]instead of a per token list mademodel_dump()warn, and it keeps guarding it after any future typed field lands.Complements #452. Open PR #452 adds the typed
top_logprobs: List[Dict[str, float]] | Nonefield toLogprobsPart, pluscontext_length_exceeded_behavior. This PR deliberately does not duplicate that. It covers the request side (validation and docs) and the round trip regression net, and it touches none of #452's hunks (different validator and docstring lines, a different test file). Both PRs merge cleanly in either order, and the new test passes both with and without #452's typed field.Known limitations
logprobs > 20for some models, this is a behavior change for those calls. I could not verify live behavior for out of range values because no API key was available, so the bound comes from the documented contract. I am happy to downgrade it to awarnings.warnif you prefer.pydantic.ValidationError, which subclassesValueError.ChatCompletionChoicesChunk.logprobs: float | None) is intentionally untouched: I have no captured streaming fixture withlogprobs > 1to justify retyping it.LogProbs.top_logprobstyped asDictbut API returnsList[Dict]#443 and Get Log Probs #251, not from a fresh live call.Testing
tests/unit/test_logprobs.py: 14 new tests (parametrized), all passing. The full unit suite goes from 206 to 220 passed, with no new warnings.Fail then pass proof: with
src/together/types/chat_completions.pyandsrc/together/types/completions.pyrestored tomain, the six rejection tests fail as expected while the regression net tests still pass, which confirms each test measures what it claims.Lint and format:
blackclean,mypy --strictclean on both touched type modules, andruff checkclean on all touched files. The oneI001inresources/completions.pypre-exists onmainand is left alone, per CONTRIBUTING's rule against reformatting unrelated code.