fix: normalize null and missing fields in streaming chat deltas - #454
fix: normalize null and missing fields in streaming chat deltas#454Ranoobaba wants to merge 1 commit into
Conversation
Broly Security ScanNote ✅ Clean scan Note Re-scan this PR anytime with
|
7b94526 to
36751d6
Compare
The API sends null where OpenAI leaves a field out, so an explicit null and a missing field behaved differently on the streaming delta, and tool call fragments stayed raw dicts (togethercomputer#160). Declare role and tool_calls on a chat delta subclass so both parse to None and fragments become typed models, and carry the fragment index on a streaming only subclass so non streaming message dumps are untouched. Related to togethercomputer#160
36751d6 to
9aa3867
Compare
|
@blainekasten ready for review when you get a chance. Small V1 fix for #160: the streaming delta now declares role and tool_calls, so an explicit null and a missing field stop behaving differently. 20 lines of source, 8 tests, all offline. The body flags one thing worth your judgement, that this makes the SDK stricter about malformed tool call fragments. |
Have you read the Contributing Guidelines? Yes.
Issue #160
Describe your changes
What and why. Issue #160 reports that the API returns an explicit
nullwhere the OpenAI streaming format leaves the field out, in three places:choices[n].delta.tool_calls = nullon text only chunks.choices[n].delta.tool_calls[n].function.arguments = nullon the first tool call chunk, where the name is given.choices[n].delta.tool_calls[n].function.name = nullon argument continuation chunks.The root cause is on the API side. This PR does not change the wire, it stops the SDK from passing the problem on. The streaming delta model (
DeltaContent) declares onlycontent, so everything else rides onextra="allow". On currentmainthat produces four separate problems, listed by how much they actually bite:ToolCallsandFunctionCallmodels the non streamingChatCompletionMessageuses. So the same tool call has two different shapes depending on whether you streamed it.model_dump(exclude_none=True)does not strip the nulls nested inside those raw dicts, so{"function": {"arguments": None}}and{"function": {"name": None}}reach OpenAI compatible consumers even when the caller asked for nulls to be dropped.delta.roleraisesAttributeErroron continuation chunks. The API sendsroleon the first chunk of a response and leaves it out afterwards, so the same attribute works or raises depending on which chunk you are holding.delta.tool_callsraisesAttributeErrorwhen the field is absent. Worth being precise here: an explicit"tool_calls": nullalready parses toNoneonmain, so this one does not fire on Together's own responses today. It fires on the OpenAI shape, where the key is left out. It is the same defect as 3, one field over, and it is the one the issue title points at, so it is fixed rather than left as a trap.The fix. Two small classes in
src/together/types/chat_completions.py, andChatCompletionChoicesChunk.deltanow uses the first:That gives:
nulland missing both parse toNoneforroleandtool_calls, so the two are no longer distinguishable.FunctionCall.nameand.argumentswere alreadystr | None, so the nested nulls normalize the same way once the items are validated.openai-pythontypes its deltas (ChoiceDeltaToolCallFunction.nameand.argumentsareOptional[str]).model_dump(exclude_none=True)now recursively drops the API provided nulls.Deliberate decisions:
indexgoes on a streaming only subclass. Streaming splits one tool call across chunks, so callers needindexto reassemble them, but the non streamingToolCallshas no such field. Subclassing keepsindexout ofChatCompletionMessage.tool_callsdumps. A test asserts it in both directions.roleis typedstr, notMessageRole. Chunks are parsed one at a time inside the streaming generator, so an unrecognised role value from a future model would otherwise raise part way through and end the stream.Noneinto an empty string forarguments. openai-python also leaves absent fields asNone; coercing would diverge from it and silently changeis Nonechecks.DeltaContentis shared withCompletionChunk, so the new fields are added on a chat specific subclass rather than the shared class.Sync and async clients share these models (
ChatCompletionChunk(**line.data)at both call sites inresources/chat/completions.py), so both are covered.Known limitations
ValidationError, and because chunks are parsed one at a time inside the generator, that error ends the stream and the caller loses every remaining chunk, including plain text that had nothing to do with tool calls. Payloads that used to pass and now raise includefunction.argumentssent as a parsed object instead of a JSON string,tool_callssent as a dict instead of a list, andidsent as an integer. This is the same strictness the non streamingChatCompletionMessage.tool_callshas had all along, so it makes the two paths consistent rather than inventing a new rule, but it is a real behaviour change and worth a maintainer's judgement.delta.tool_calls[0]["function"]worked; it now raisesTypeError: 'ToolCalls' object is not subscriptableand needs attribute access. That is exactly the population that hit this issue. Also, a plainmodel_dump()withoutexclude_none=Trueof a text only delta now includestool_calls: Noneandrole, which is standard Pydantic optional field behaviour and matchesChatCompletionMessageand openai-python.together.types.__all__.ChatCompletionDeltaContentis now the runtime type of every chat stream delta, so there is a case for exporting it, but that touches another file and I kept the diff to the fix. Happy to add it.together-py), so this is scoped as a small maintenance bug fix. The underlying API cleanup this issue asks for is still worth doing server side.Testing
New file
tests/unit/test_chat_completion_stream_types.py, 8 tests using chunk fixtures shaped exactly like the issue's three instances, plus a missing field control, an undeclared field passthrough check, anexclude_nonerecursive dump assertion, and a non streaming regression control.Fail then pass, with the new tests run against unfixed
main:The 2 that pass on
mainare the deliberate controls: the non streaming path is untouched, and undeclared fields still flow through.On this branch the full offline suite is
214 passed, against206 passedonmain, and the difference is exactly the new file. No existing test changed.ruffandblackare clean on both changed files, andmypy --strictreports the same 5 pre existing errors in unrelated files before and after.Everything runs offline. The tests build models from literal dicts, so no client is constructed, no key is read, and no host is named. I confirmed that by re-running them with
socket.socket.connect,socket.create_connection,socket.getaddrinfoandssl.SSLContext.wrap_socketall replaced with a raiser, after first checking the block itself fires.