Repository navigation
Enrich chat_parsing streaming events - #40478
yonigozlan wants to merge 16 commits into
Conversation
43c3c83 to
a4904b5
Compare
Rocketknight1
left a comment
There was a problem hiding this comment.
A lot of these changes seem useful! A couple of them change the spec, though, like strict for xml-inline, and I'm not sure if that's necessary or not. Also, in some cases there are probably more changes than necessary (I flagged a couple of examples, there may be more!)
Overall I like the integration, though, and if there are necessary changes then I'm willing to upstream them to transformers! But it'd be cool if we could make sure that the changes are 100% minimal and focused, rather than a big sprawling update with unnecessary changes!
a4904b5 to
ada8f35
Compare
|
Thanks for the reviews @Rocketknight1 ! I simplified the additions a bit following your comments |
|
Got it! If you're using agents for this, Opus 5.5 seems like it's a big improvement for simplifying/reviewing code. Might be worth handing the whole thing to it and asking if it can find conceptual simplifications or ways to reduce the diff without breaking funcitonality |
|
Thanks @Rocketknight1 ! Made some more simplifications, the diff is now +52/-18 to response_parser.py, and is mostly adding support for returning malformed region instead of raising, +exact positions of closing delimiters. I don't think we can do less without compromising on correctness now. Indeed Opus 5.5 helped a lot there! |
Rocketknight1
left a comment
There was a problem hiding this comment.
Yes, that sounds great now! Thanks for the cleanup!
resubmitting with commenti nstead of changes requested
Jiminator
left a comment
There was a problem hiding this comment.
Just left one comment, thank you for the contribution!
| stream = ResponseParser(response_template, prefix=prefix, tools=tools) | ||
| events = stream.feed(text) | ||
| message, final_events = stream.finalize() | ||
| for event in events + final_events: |
There was a problem hiding this comment.
Could you include stream.initial_events in this malformed-event check. A malformed JSON region closed in the assistant prefill currently returns {} instead of raising, while the implementation in #40477 raises for the same input.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
A `repeats` field appends into the default list object owned by the compiled template (and, through the shallow copy, into the caller's spec dict), so one parse's tool calls leak into every later parse sharing the template. Deep-copy defaults at template load and at parser init, and add a regression test.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Delimiter text comes from the input offsets, and open events keep named captures. Unmatched text around XML tags no longer fails the parse.
Keep the event contract to what adapters need: open, close and malformed events carry the span of input_text they consumed, explicit opens carry their captures, and malformed events carry the original error. - Drop per-chunk offsets, prefix_end, close_start and closed; the prefix boundary is len(input_text) before the first feed(). - parse_response re-raises the original exception, like Transformers. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
`parse_response` only checked events from the generated text, so a region the prefix left malformed was silently ignored. Include `initial_events` in the check. Co-authored-by: Cursor <cursoragent@cursor.com>
4fce556 to
106b751
Compare
JustinTong0323
left a comment
There was a problem hiding this comment.
The prefix fix and the simplifications look good. A few small things before this lands — the first one should be settled before the serving adapter builds on it.
| # Prefix bytes held back as a possible delimiter belong to the prompt, not the chunk. | ||
| text = text[max(0, self._prefix_len - self._pos) :] | ||
| if text: | ||
| dirty = field.content not in STREAMABLE_PARSERS |
There was a problem hiding this comment.
Clean chunks still aren't final-value fragments: <n>1/2a/</n> emits two dirty=False chunks and then goes malformed at close, and a text field with a transform emits clean chunks whose close value is a dict. Settle this before the adapter relies on it — either restrict clean to text without transform, or redefine dirty and make code, docs and tests agree.
| stream.feed(text) | ||
| message, _ = stream.finalize() | ||
| events = stream.feed(text) | ||
| message, final_events = stream.finalize() |
There was a problem hiding this comment.
finalize() runs before any malformed event is inspected, so a missing required field masks the region's real error: template with required x and y, input <x>{</x> raises "fields missing: ['y']" instead of the JSON parse error. Check the recorded malformed events before finalizing.
| f"Field '{field.name}': 'join' requires each match to parse to a string, " | ||
| f"got {type(value).__name__}." | ||
| ) | ||
| except (KeyError, TypeError, ValueError) as error: |
There was a problem hiding this comment.
RecursionError escapes this handler — a region body with deep nesting ('[' * 20000) aborts the whole stream instead of recovering (same through _coerce via the JSON content parser). Convert it to ValueError at the decode boundary so recover-and-continue holds.
| self._opened = True | ||
| events.append({"type": "region_open", "field": field.name}) | ||
| events.append( | ||
| {"type": "region_open", "field": field.name, "start": m.start(), "end": m.end(), "captures": self._captures} |
There was a problem hiding this comment.
This now aliases self._captures — a consumer mutating the event's captures dict changes the parsed value at close. Use a dict(...) copy to keep the snapshot semantics.
| self.assertEqual(result, {"role": "assistant"}) | ||
| self.assertEqual(final_events, []) | ||
|
|
||
| def test_region_events_expose_delimiter_offsets(self): |
There was a problem hiding this comment.
Non-blocking: offsets are only exercised on single feeds. A mutation that mis-reports implicit-open offsets passes the whole suite — worth adding a chunking-invariance check on non-chunk events (compare full event dicts across chunkings, offsets included).
|
/tag-and-rerun-ci |
Extend
chat_parsingstreaming events with just enough context for serving adapters, while keeping parsing results unchanged.Context
Transformers response templates describe how generated text is split into structured message fields. The core parser can already parse those fields, but its original events are intentionally minimal.
A serving adapter needs a little more: where each delimiter sits in the parser input, the tool name captured by an opener before the region body exists, and a way to recover when one region fails to parse. Without that, every serving library has to reconstruct parser state on its own.
Event contract
region_openstart,end,capturesinput_text(empty for implicit opens). Explicit opens also carry the opener's named groups.region_chunktextis raw region text.dirty=Falseonly for untransformedtextfields; whitespace trimming still happens at close.region_closestart,endvalue.region_malformedregion_closewhen a region's value fails to parse or transform. Carriesstart,end, and the originalerror.ResponseParser.input_textexposes the raw input after start-anchor truncation. Before the firstfeed()it is exactly the truncated prefix, so its length marks where generated text begins.region_chunktext.Malformed output
region_malformedand parsing continues, so later content and valid calls still parse.parse_response()re-raises the original exception from the first malformed region, even when required fields are also missing.Scope
The grammar, transforms, and tool-argument coercion are unchanged. This PR only enriches the event and recovery contract consumed by the adapters in the next layer.
Stacked on Port chat_parsing core.
CI States
Latest PR Test (Base): ❌ Run #37720990537
Latest PR Test (Extra): ❌ Run #37720990107
Latest PR Test (AMD ROCm 10): ❌ Run #37720990395