Streamable chat parsing - #45847
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
9ec6ddc to
fd4f211
Compare
|
This should now be ready for review! It's a lot, but thankfully I think you can skim a lot of it. It's also a little ugly right now, because we need to maintain legacy support for the older The main things to review are:
Most other files are agent-written boilerplate changes to support the new API (the pipeline + Gemma conversion script changes), or agent-written internal mechanics that are mostly dense and should be well-covered by tests ( |
pcuenca
left a comment
There was a problem hiding this comment.
Quick look at the docs + gemma 4 testing.
| The `json` parser also accepts dialect arguments (`unquoted_keys`, `string_delims`) for models that emit JSON with cute | ||
| quirks that completely break the standard parser. The model authors who are responsible for this being necessary | ||
| know who they are and should feel an appropriate amount of shame. |
There was a problem hiding this comment.
This comment should never be resolved. LOVE IT. 😂
Each schema becomes a JSON file under response_parser/schemas/, loaded once at import. schemas.py shrinks to a thin loader plus a comment explaining why this code exists at all: llama.cpp's chat-auto-parser introspects the GGUF chat template at runtime so per-family schemas disappear, but that module lives in llama.cpp's common/ tree and is not reachable from llama-cpp-python today. When a binding appears the whole response_parser/ package gets deleted in one commit. transformers pinned to >=5.8.1,<5.9 because HF is mid-refactor of the schema DSL (PR huggingface/transformers#45847 renames response_schema to response_template with a new shape). A casual minor bump would break extraction.
3b0b423 to
3bd988b
Compare
| }, | ||
| }, | ||
| }, | ||
| "open_pattern": r"<\|tool_call>call:(?P<name>\w+)", |
There was a problem hiding this comment.
Note this is Python-only syntax for named groups, so we'd need a translation layer to ICU somewhere for downstream implementations, cc @xenova. Perhaps we should accept both in transformers and document ICU as the standard?
There was a problem hiding this comment.
Ah, I wasn't aware! Is there a universal way to do it, or is there no universal way to make named groups work? I might even prefer unnamed groups and putting the names in a separate field rather than causing cross-language problems
There was a problem hiding this comment.
I didn't find a universal way to do it, unfortunately. This is Python and this is ICU (screenshots with the relevant fragments below).
Yeah, perhaps unnamed groups is the best option if it doesn't make things too confusing.
There was a problem hiding this comment.
Hmmn, it might be possible to handle this universally by just having other language implementations strip the <> angle brackets if their regex engine treats them as part of the name!
There was a problem hiding this comment.
The main problem is the P, the regex has to be rewritten without.
There was a problem hiding this comment.
Ah, I'm sorry, I misread that bit! I'll update the docs
| "close": "<|END_ACTION|>", | ||
| "content": "json", | ||
| "transform_each": True, | ||
| "transform": {"type": "function", "function": {"name": "{tool_name}", "arguments": "{parameters}"}}, |
There was a problem hiding this comment.
Perhaps we could show a fragment of a rendered Cohere tool call so readers understand it contains tool_name and parameters.
There was a problem hiding this comment.
Added a little example to clarify!
189e804 to
1f8619e
Compare
dbae352 to
5210577
Compare
01b9c65 to
462064d
Compare
|
I think we've gotten all the reviews we're going to get! Pinging core maintainers @ArthurZucker @Cyrilvallez @vasqu if any of you wants a veto before I merge this 😅 |
b49a3a6 to
fdfc1d4
Compare
ArthurZucker
left a comment
There was a problem hiding this comment.
TY!
IDK how much we can trust AI for the core part, I don't have the time to deep dive into what it's doing. The format is the most important, we can improve what's inside later on!
Examples are really important when defining the standards imo! Let's make sure we add more when explaining.
"content": "text", appears a lot, could be default in lots of cases no?
But that's a nit.
Format wise I like json, I think it should be alright to express stuff. Cross pateform is super important so it helps. !
| A field should have **either** `open` or `open_pattern`, but not both, and the same is true for `close` and `close_pattern`. | ||
|
|
||
| A field with **neither** `open` nor `open_pattern` is the **implicit** field: it's active whenever no explicit | ||
| region is open, so it captures leftover text. At most one field can be implicit. This is most often used when `content` | ||
| does not have special token tags, it's just written as plaintext after the other fields. | ||
|
|
||
| In addition to opening and closing delimiters, you can also specify `repeats`, which indicates that the field is a list | ||
| and the delimiters can match multiple times. This is most common for parallel tool calling, when a model emits | ||
| multiple tool calls simultaneously. | ||
|
|
||
| Finally, you can specify `optional: false` for fields that must be present. If parsing finishes and a non optional field | ||
| was never opened, we raise an error instead of silently omitting the field. | ||
|
|
||
| The end of generation will close and finalize any open regions, even if their closing delimiter was not seen. |
There was a problem hiding this comment.
small examples here again can help otherwise its hard to visualize
There was a problem hiding this comment.
Scattered a few small examples here!
| type conversion if required. They do not have any `content_args`, except for `text` which supports the arg `strip`, | ||
| which strips whitespace from the start and end of the captured text, and defaults to `true`. | ||
|
|
||
| #### json |
There was a problem hiding this comment.
i would put small example for each type here
| "content": "json", | ||
| "transform": {"type": "function", "function": "{content}"}, | ||
| }, | ||
| ``` |
There was a problem hiding this comment.
i would show again how that would render
| "content": "json", | ||
| "transform": {"type": "function", "function": {"name": "{name}", "arguments": "{content}"}}, | ||
| }, | ||
| ``` |
There was a problem hiding this comment.
again would like to see example!!!
There was a problem hiding this comment.
like out -> becomes what with this
| _LAX_OPEN, _LAX_CLOSE = "\x01", "\x02" | ||
|
|
||
|
|
||
| def _json(text: str, args: dict) -> Any: |
There was a problem hiding this comment.
if anything is vendored let's credit !
There was a problem hiding this comment.
None of this was copied from elsewhere, I think! A lot of it was agent-written because it was deep internals
| return out | ||
|
|
||
|
|
||
| CONTENT_PARSERS = { |
There was a problem hiding this comment.
can people easily register custom parsers?
There was a problem hiding this comment.
There isn't a facility for that right now! Custom parsers introduce remote code execution issues, and the range of parsers we have right now covers every case I know of. I think if a new model needs a new parser, we should just accept a PR to add a new parser to transformers, or to expand the args for an existing parser to support the new model. But hopefully that should be quite rare.
| reloaded = AutoTokenizer.from_pretrained(tmpdir) | ||
| self.assertEqual(reloaded.response_template, ernie_template) | ||
|
|
||
| def test_tokenizer_parse_response(self): |
There was a problem hiding this comment.
these are very nice!
vasqu
left a comment
There was a problem hiding this comment.
Sorry for the delays, also some comments from my side. My only real concern is the batching capability being a limitation now - I don't expect the stream based parsing to be fully async but it would be nice if we had some simple version for batches 🤔
There was a problem hiding this comment.
So we are starting with gemma4 as the first one?
There was a problem hiding this comment.
Yeah, Gemma4 is the only model that has the old-style response_schema. I'm hoping for a very, very short deprecation period where we merge this PR, then move Gemma4 to a new response_template, then totally deprecate/remove the old code.
| task="text-generation", | ||
| model="hf-internal-testing/tiny-gpt2-with-chatml-template", | ||
| ) | ||
| text_generator.tokenizer.chat_template = ( |
There was a problem hiding this comment.
Just fmi, are overriding the templates to simplify or why is it necessary?
There was a problem hiding this comment.
In this case I'm overriding the template to prefill part of the assistant message, to check that parsing still works!
|
|
||
| def test_batched_inputs(self): | ||
| def test_batched_inputs_raise(self): | ||
| # Batched parsing is no longer supported: `parse_response` accepts a single sequence only |
There was a problem hiding this comment.
That is a huge limitation imo. We really should aim to enable batching 🤔 maybe not with streaming at the same time (async) but at least in a simpler variation where we can safely process the complete text
There was a problem hiding this comment.
This is very temporary! I was basically trying to avoid code clutter while we deprecate the old response_schema. After it's gone, clean batching is much simpler. However, if you prefer I can try to implement batching for both, do the deprecation, then simplify after we deprecate
There was a problem hiding this comment.
Yea I think, this is the biggest point to me. The rest are rather small issues or nits. But it really would be nice to support batching from the get-go even if it comes with some additional overhead at the beginning
There was a problem hiding this comment.
They are currently not integrated into CI ig? Should we add them to slow ci? Just unsure how long these would take 👀
In general no complaints here, solid integration tests
There was a problem hiding this comment.
I think the CI is running these tests already! I think anything in tests/utils/* ends up in tests_non_model
vasqu
left a comment
There was a problem hiding this comment.
Ok, I think this looks pretty ready. We can push for efficiency in follow up PRs imo (meaning for batching) but I think this is probably still gonna see a lot of changes regardless. Maybe we should note in the docs that this is experimental / in development so things are subjected to change?
My points are smaller in nature 🤗 nice job!
| The model authors responsible for the existence of `unquoted_keys` and `string_delims` know who | ||
| they are and should feel an appropriate amount of shame. |
There was a problem hiding this comment.
😂 but we should refrain from formulating it as this lol
| parsed JSON dict slots in directly as the value of `function`. A placeholder must be the entire string: mixing | ||
| text and placeholders (`"abc {name} def"`) is not permitted. They're not f-strings! | ||
|
|
||
| You can abuse `transform` quite a lot, which becomes necessary when the model output has a wildly different format |
There was a problem hiding this comment.
wording :D abusing -> versatile
| model authors, this shouldn't be a problem, but if you are a developer writing an implementation of response parsing | ||
| in another language, you should be aware of our implementation details. This section is dedicated to everyone | ||
| who had to implement an entire Jinja parser to get non-Python chat templating to work - we hope that if you | ||
| follow the simple guidelines below, then response templates should be much less painful: |
There was a problem hiding this comment.
nit: do we want to wrap this in a details / summary section
There was a problem hiding this comment.
I think it's okay down at the bottom under the 'for developers' block, but we can change later if it's annoying!
| # Decode inputs and outputs the same way to remove input text from generated text if present | ||
| skip_special_tokens = skip_special_tokens if skip_special_tokens is not None else True | ||
| if getattr(self.tokenizer, "response_schema", False): | ||
| if getattr(self.tokenizer, "response_template", None) or getattr(self.tokenizer, "response_schema", None): |
There was a problem hiding this comment.
not really related to this PR but we should probably refactor these common parts out at some point 🤔 because we copy paste this through anything generation related
| ) | ||
| else: | ||
| # Explicit schema argument: detect new-style by the presence of a top-level `fields` key. | ||
| use_new_template = isinstance(schema, dict) and "fields" in schema |
There was a problem hiding this comment.
hmm, seems a bit brittle as check no?
| if isinstance(prefix, str): | ||
| prefix_texts, prefix_batched = [prefix], False | ||
| elif isinstance(prefix, (list, tuple)) and (not prefix or isinstance(prefix[0], str)): | ||
| prefix_texts, prefix_batched = list(prefix), True | ||
| else: | ||
| decoded = self.decode(prefix) | ||
| prefix_texts, prefix_batched = ([decoded], False) if isinstance(decoded, str) else (decoded, True) | ||
| if not prefix_batched: | ||
| prefixes = prefix_texts * len(responses) # broadcast the single prefix to every response |
There was a problem hiding this comment.
its the same as for responses nit maybe to refactor a bit? but tbh very big nit
| spec's `start_anchor`). Generated chunks fed via `stream.feed()` are then classified | ||
| correctly even when the chat template emitted assistant-turn content (e.g., `<think>\\n`) | ||
| that the model continues from.""" | ||
| tmpl = response_template if response_template is not None else getattr(self, "response_template", None) |
| parsed = [_template_parse_response(text, schema, prefix=pfx) for text, pfx in zip(responses, prefixes)] | ||
| else: | ||
| parsed = [recursive_parse(text, schema) for text in responses] | ||
| return parsed if batched else parsed[0] |
There was a problem hiding this comment.
ok yea it's a foreach as we heavily rely on the stream parser. idk how chat templates are handled currently but let's aim to make this efficient in follow up PRs, e.g. some multi threading pool, async streamer etc
There was a problem hiding this comment.
I think chat templates are also just a foreach because there is no parallel Jinja! I'm hopeful that the region matching is pretty fast though, especially when run on a whole message at once.
There was a problem hiding this comment.
But yeah if performance is annoying later we can definitely refactor to speed it up a lot without breaking BC
| "content": "text", | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
you prolly already thought of this but let's make these be loaded with auto tokenizer when we upload the templates one by one
Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
|
[For maintainers] Suggested jobs to run (before merge) run-slow: gemma4 |
|
I think most of the review comments have now been addressed, going to merge! There are a few nits in docs or performance that we can move to follow-up work but nothing that will break BC, so I think merging is safe at this point! |
* scaffolding * Let's do an initial commit * Let's do an initial commit * More cleanup * More cleanup * Add the prefill * One more stash commit * One more stash commit * Renames/docstring cleanup * More renames/docstring cleanup * More renames/docstring cleanup * Docstring cleanup * make fix-repo * Doc cleanup * Doc cleanup * make fix-repo * No more coerce, merge content types * Cleanup: Support tokens/arrays/lists correctly for input and prefix * Add prefix to docs too * Better initial_events handling, better region handling * Allow multiple literal matches * Allow emitting dirty regions * Doc updates * Drop `assemble` to just use `transform` * Lots more doc updates * Lots more doc updates * Remove batched handling, lots of cleanup * Use the new method in test_serve * Small doc update * Update docs/source/en/chat_response_parsing.md Co-authored-by: Pedro Cuenca <pedro@huggingface.co> * Update docs/source/en/chat_response_parsing.md Co-authored-by: Pedro Cuenca <pedro@huggingface.co> * Update docs/source/en/chat_response_parsing.md Co-authored-by: Pedro Cuenca <pedro@huggingface.co> * Update src/transformers/models/gemma4/convert_gemma4_weights.py Co-authored-by: Pedro Cuenca <pedro@huggingface.co> * Mention the extra * Fix the example stream so it doesn't refer to the previous example * Remove the `strip` arg for numerical/bool types because it was meaningless * Fix convert script bracket bug * Add a regex compatibility section * Update regex compatibility section * jmespath: defeated * jmespath: defeated * Flesh out the Cohere example * Update the docs with proper content_args * Make start_anchor mandatory * Update gemma4 template! * Proper buffering * Corrected prefix handling * Type fixes * make fix-repo * Pipeline updates, batching changes * Post-rebase fixes * Tiny reorder on the version check * Fix licences + no variable abbreviations * Cleanups, remove assertion * Refactor load_response_template * Lots of little doc examples * Fix docs to explain prefix a bit better * Add batching * Add batching * Docs cleanup * Docs cleanup, big block of vasqu comments * Apply suggestions from code review Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com> * More comment fixes --------- Co-authored-by: Pedro Cuenca <pedro@huggingface.co> Co-authored-by: Anton Vlasjuk <73884904+vasqu@users.noreply.github.com>
Work in progress PR for streamable chat parsing. This introduces a new improved kind of response schema, which I'm calling
response_template. Response templates are a lot easier to write, since they avoid most of the messy regexes of the older system. More importantly, though, using a region-based system means that model authors can write templates that enable streaming parsing, emitting fields as they're ready, rather than needing the whole message to be ready at once.TODO:
coerceinto content types?