Document what logprobs returns and bound it to the documented range - #455
Document what logprobs returns and bound it to the documented range#455Ranoobaba wants to merge 1 commit into
Conversation
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
e3e138c to
3bdd9c1
Compare
3bdd9c1 to
4e94c92
Compare
Issue togethercomputer#251 asks how to get log probabilities for more than the sampled token. The answer is logprobs, but the four create() docstrings only said "number of top-k logprobs to return". Say what it does, and bound the field to the documented 0 to 20 so a bad value fails before the request. The bound sits on the field, not in the model validator, so the error names logprobs and carries only that value. A model validator reports the whole request as the bad input, putting the prompt into the exception. Addresses togethercomputer#251.
4e94c92 to
53d12b6
Compare
|
@blainekasten ready for review when you have time. This answers #251 by documenting what logprobs actually does in the four create() docstrings, and bounds the field to the documented 0 to 20. It merges cleanly with #452 and I confirmed the tests pass with that PR's field applied. One judgement call is flagged in the body: this is the first hard client side rejection of a pass through parameter, so if you would rather it warn than raise, say the word and I will switch it. |
Have you read the Contributing Guidelines? Yes.
Issue #251
Describe your changes
What and why. Issue #251 asks how to get log probabilities for more than just the sampled token. The answer is the
logprobsparameter, which takes the number of top tokens to score at each step, but nothing in the SDK says so. All fourcreate()docstrings read:which reads as "how many logprobs come back" rather than "how many alternatives per token", and gives no range. The documented range is 0 to 20 for both chat completions and text completions: "An integer between 0 and 20 of the top k tokens to return log probabilities for at each generation step." Out of range values were only rejected by the server, so the round trip was spent before finding out.
The fix. Two parts.
create()docstrings (sync and async, chat and text), including the range.on
ChatCompletionRequestandCompletionRequest, so a bad value fails locally instead of after a request.Why on the field and not in the validator.
verify_parametersalready exists on both models and was the obvious place, but amode="after"model validator reports the whole request as the offending input. That puts the message list or the prompt inside the exception:str(e)truncates it, so it looks harmless, but anything that serializeserrors(), such as a crash reporter or structured logging, captures the prompt verbatim. Before this change the request went to the server and came back as an API error that did not repeat the prompt, so moving validation earlier should not hand user text to every logger. A field constraint keeps the error scoped:It also puts the range in the generated JSON schema, and it is less code than the validator branch. Two tests pin the prompt staying out of the exception, one per request model.
Known limitations
logit_bias, documents its range of [-100, 100] and does not enforce it, and the same is true oftemperature,top_p,min_pandn. The existingverify_parametersvalidator only ever warns. I enforced here because the range is documented for both endpoints and an out of range value can never succeed, so failing early is strictly faster. If you would rather keep the SDK consistent and permissive, say so and I will switch it towarnings.warn, matchingrepetition_penalty.ValidationError, not aTogetherException. Code shaped likeexcept together.error.InvalidRequestError:aroundcreate()used to catch an out of rangelogprobs, because the rejection came back from the server. It will not catch it now.--logprobsis a plainclicktype=inton bothchat.pyandcompletions.py, so an out of range value reaches the model and surfaces as a pydantic traceback rather than a clean click message. Addingclick.IntRange(0, 20)there would fix it, but it is a separate change in two more files and I kept this diff small.ChatCompletionChoicesChunk.logprobsandCompletionChoicesChunk.logprobsare typedfloat | None, while the non streaming models use theLogprobsPartobject. The samecreate()methods whose docstrings this PR rewrites also acceptstream=True, so anyone following the new documentation with streaming on will hit a parse failure if the server sends the object shape on chunks. I had no API key to confirm the streaming wire shape, so I have not widened the type on a guess. Worth a follow up either way.logprobs=Trueis silently accepted as 1.boolsubclassesint, and pydantic accepts it in lax mode, so a caller coming from the OpenAI SDK, where chatlogprobsis a boolean and the count lives in a separatetop_logprobsfield, gets top 1 rather than an error. Pre existing, unchanged here, noted because the new docstring says "integer".validate_assignment, so settingrequest.logprobs = 999afterwards survives. Everycreate()builds the model in one shot, so this is not reachable through the public API today.together-py). The docstring half is a plain documentation fix; the validation half is a behaviour change, so it is reasonable to want it in 2.0 instead. Happy to split them.Testing
New file
tests/unit/test_logprobs.py, 16 tests: 6 rejection cases across both request models, 6 acceptance cases including 0 and 20 as boundaries, one thatlogprobsstays out of the serialized payload when unset, two that the rejection does not carry the prompt, and one response side round trip.Fail then pass: run against unfixed
main, the 6 rejection tests fail withDID NOT RAISE ValidationError. On this branch the full offline suite is222 passed, against206 passedonmain, and the difference is exactly the new file. No existing test changed.Relation to open PR #452. #452 ("Fix chat completion request and logprob contracts") touches two of the same files and proposes declaring
top_logprobs: List[Dict[str, float]]onLogprobsPart. The two are compatible and independent: #452 is response side typing, this PR is request side validation plus docstrings. I checked rather than assumed. They merge cleanly withgit merge-tree, and all 16 tests here still pass with #452's typed field applied locally. If #452 lands first, nothing here needs changing. Whichever order suits you is fine by me.The response side test deserves a caveat rather than a claim.
top_logprobsis not a declared field onLogprobsPart, which declares onlytokensandtoken_logprobs. It survives because the base model setsextra="allow", so the value is carried untyped and dumped back unchanged. That test pins the pass through and fails if the field is later declared with the wrong shape. It does not constrain what the server sends, and no source change in this PR affects that path. The test docstring says exactly that.ruffreports oneI001onsrc/together/resources/completions.py, identical onmain, in an import block this PR does not touch, so it is left alone per the contribution guide.blackis clean andmypy --strictreports the same 14 pre existing errors in unrelated files before and after.Everything runs offline. All 16 tests pass with socket creation and DNS raising and no API key set.
Addresses #251.