Skip to content

Validate logprobs range and pin top-k logprobs round-trip - #455

Draft
Ranoobaba wants to merge 1 commit into
togethercomputer:mainfrom
Ranoobaba:fix/logprobs-topk-251
Draft

Validate logprobs range and pin top-k logprobs round-trip#455
Ranoobaba wants to merge 1 commit into
togethercomputer:mainfrom
Ranoobaba:fix/logprobs-topk-251

Conversation

@Ranoobaba

Copy link
Copy Markdown

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: logprobs accepts 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:

  1. The docstrings said only "Number of top-k logprobs to return", with no range, in all four create() methods (chat and completions, sync and async).
  2. An out of range value such as logprobs=50 was sent straight to the API and surfaced only as an opaque HTTP error after the request.
  3. Nothing in the test suite pinned that the per token top_logprobs alternatives returned by the API actually survive response parsing and model_dump().

Fix decisions.

  1. ChatCompletionRequest and CompletionRequest now reject logprobs outside [0, 20] inside the existing verify_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 raises ValueError elsewhere 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 --logprobs flag.
  2. The logprobs docstring in all four create() methods now documents the semantics and the range, using the same "Must be in the range [a, b]." phrasing the file already uses for presence_penalty, frequency_penalty, min_p, and logit_bias.
  3. A new regression test pins that response.choices[0].logprobs.top_logprobs, which is a list with one {token: logprob} dict per generated token as captured from the live API in LogProbs.top_logprobs typed as Dict but API returns List[Dict] #443 and Get Log Probs #251, round trips through parsing and model_dump() with no Pydantic serialization warnings. Today this works because response models use extra="allow". The test guards the accessor path against the mistyping failure mode reported in LogProbs.top_logprobs typed as Dict but API returns List[Dict] #443, where top_logprobs declared as Dict[str, float] instead of a per token list made model_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]] | None field to LogprobsPart, plus context_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

  1. This is the first hard range validation in these request models; the existing validator only warned before. If the API intentionally accepts logprobs > 20 for 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 a warnings.warn if you prefer.
  2. Code that previously built request models with out of range values and relied on the server rejecting them will now fail on the client side with pydantic.ValidationError, which subclasses ValueError.
  3. Streaming chunk typing (ChatCompletionChoicesChunk.logprobs: float | None) is intentionally untouched: I have no captured streaming fixture with logprobs > 1 to justify retyping it.
  4. The response fixture is reconstructed from real API output pasted in LogProbs.top_logprobs typed as Dict but API returns List[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.py and src/together/types/completions.py restored to main, the six rejection tests fail as expected while the regression net tests still pass, which confirms each test measures what it claims.

On un-fixed main:
  FAILED test_chat_request_rejects_out_of_range_logprobs[-1|21|100]        DID NOT RAISE ValidationError
  FAILED test_completion_request_rejects_out_of_range_logprobs[-1|21|100]  DID NOT RAISE ValidationError
  PASSED test_*_accepts_in_range_logprobs[0|1|20]                          (regression net)
  PASSED test_request_logprobs_defaults_to_omitted                        (regression net)
  PASSED test_top_logprobs_survive_parsing_and_model_dump                 (regression net; 0 must survive model_dump(exclude_none=True))

With the fix: 20 passed.

Lint and format: black clean, mypy --strict clean on both touched type modules, and ruff check clean on all touched files. The one I001 in resources/completions.py pre-exists on main and is left alone, per CONTRIBUTING's rule against reformatting unrelated code.

@broly-code-security-scanner

Copy link
Copy Markdown

Broly Security Scan

Note

Clean scan
No vulnerabilities detected in this PR.

Note

Re-scan this PR anytime with /broly scan — useful after /broly undismiss, or to refresh findings without a new push.

Broly — SAST (zai-org/GLM-5.2) · Secrets · SCA · GH Actions (zizmor) · Containers · SBOM · Powered by Together AI

@Ranoobaba Ranoobaba mentioned this pull request Jul 21, 2026
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
Ranoobaba force-pushed the fix/logprobs-topk-251 branch from e3e138c to 3bdd9c1 Compare July 21, 2026 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant