Repository navigation
[Parser] Migrate Hermes tool parser to the new streaming Parser Engine - #51937
muhammadfawaz1 wants to merge 5 commits into
Conversation
Replace the regex/diff-based Hermes2ProToolParser (~275 lines) with a
declarative ParserEngineConfig in vllm/parser/hermes.py plus a thin
ToolParser bridge, following the Qwen3/MiniMax M2/Inkling pattern. The
registry entry and the Hermes2ProToolParser class name are unchanged.
Arguments are carved out of the {"name":...,"arguments":{...}} wrapper by
_hermes_arg_converter as a verbatim, prefix-stable substring. A converter
is required here: ParserEngine._compute_arg_delta returns the raw delta and
skips _fix_arg_types when arg_converter is None, while non-streaming
extraction always coerces, so omitting it makes streaming and non-streaming
disagree on argument types. Re-serialising through json.dumps is not
prefix-stable across ticks, which the engine's argument diffing requires.
token_id_terminals is left empty so the delimiters are always matched by
the text lexer; the engine's strict token-id mode otherwise rejects a
text-matched terminal once real token IDs have been seen.
extract_tool_calls_from_content preserves the legacy single-shot
json.loads strictness (no tool calls reported when a body is malformed),
which the engine's generic best-effort EOS path does not reproduce.
LongcatFlashToolParser subclassed Hermes2ProToolParser and customised the
delimiters via tool_call_start_token/tool_call_end_token, which the engine
ignores. It now has its own HermesParser subclass with the delimiters as
class attributes, keeping its inherited structural_tag_model.
tests/parser/test_streaming.py used Hermes as its reference *non-engine*
tool parser, a premise this migration invalidates. Those tests now use
Granite4ToolParser, which is still non-engine, uses the same <tool_call>
delimiters, and has supports_required_and_named=True natively, so the
autouse monkeypatch fixture that forced it on Hermes is no longer needed.
Test intent is unchanged; only the reference model is.
Co-authored-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com>
Signed-off-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com>
Signed-off-by: muhammadfawaz1 <135441198+muhammadfawaz1@users.noreply.github.com>
Docstrings and inline comments were re-explaining behavior already stated at the implementing function, or explaining engine internals a vLLM-familiar reader doesn't need. Keep only the two comments where the "obvious" alternative produces silent wrong output with tests still green: re-serializing the arg converter, and populating token_id_terminals. Co-authored-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com> Signed-off-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com> Signed-off-by: muhammadfawaz1 <135441198+muhammadfawaz1@users.noreply.github.com>
|
/ci run |
|
✅ @muhammadfawaz1, CI is now available for this PR.
|
|
✅ Triggered Buildkite CI #83699 for commit |
The replay tests enumerate every ParserEngine in registered_adapters and fail collection when one has no trace_builder entry. Both parsers opt out of token-id terminal matching, so they cannot use this TOOL_END-token-id harness; skip them alongside Mistral and Inkling, which opt out for the same class of reason. Coverage lives in tests/tool_parsers/. LongcatParser also inherited hermes_config()'s hardcoded name and reported config.name='hermes', which would mislabel it wherever the config name is used. Add CONFIG_NAME so it reports 'longcat'. Co-authored-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com> Signed-off-by: Mahad Rehman <114791389+mahadrehmann@users.noreply.github.com> Signed-off-by: muhammadfawaz1 <135441198+muhammadfawaz1@users.noreply.github.com>
…om/muhammadfawaz1/vllm into fix/hermes-parser-engine-migration
This comment was marked as outdated.
This comment was marked as outdated.
|
✅ Triggered Buildkite CI #83721 for commit |
|
@chaunceyjiang all the CI tests have passed, would appreciate a review on this when you get a chance so we can get it merged and keep moving. One thing we ran into while verifying this: tool_choice="required" crashes for engine-based parsers (documented in the PR's "Out of scope" section) — reproduces on the already-merged Kimi K2 parser, so it's unrelated to Hermes. Should we file a PR for that now, or hold off and keep going on migrations first? |
|
/cc @bbrowning @sfeng33 PTAL. |
|
@bbrowning @sfeng33 following up here, CI's been green since the last push and this is tagged ready. Would appreciate a review when you get a chance so I can keep moving on the rest of the parser-engine migration series. Happy to address anything that comes up. |
|
This pull request has merge conflicts that must be resolved before it can be |
[Parser] Migrate Hermes tool parser to the streaming Parser Engine
Summary
Migrates the Hermes tool parser from the legacy regex/diff implementation (~275 lines) to the declarative streaming Parser Engine, following the pattern already used by Qwen3, MiniMax M2, Gemma4, Kimi K2 and Inkling.
vllm/parser/hermes.py:HermesParser(ParserEngine)+ a declarativeParserEngineConfig.vllm/tool_parsers/hermes_tool_parser.pybecomes a thin bridge. The registry entry and theHermes2ProToolParserclass name are unchanged, so--tool-call-parser hermesis unaffected.structural_tag_model = "hermes"is preserved, keeping guided decoding (and thesupports_required_and_named=Falseit implies) identical.This follows the suggestion on #49535 to migrate tool parsers to the Parser Engine rather than patch the legacy implementations further.
Why Hermes first, and not
hunyuan_a13b/xlamThose two parsers emit a single undelimited JSON array (
[{...},{...}]), where the boundary between one tool call and the next is a bare comma at the array's top nesting level — there is no terminal token. The engine's state machine only transitions on explicit terminal tokens; brace/bracket depth tracking in the engine exists only to decide which characters are safe to stream, not to drive a transition. Migrating those two needs new engine-level capability, not just a per-model config, so it's a separate design question rather than something to fold into this PR.Hermes uses the same per-call delimiter shape as the already-migrated models (
<tool_call>…</tool_call>repeated per call), which is proven engine capability, so it's independently actionable now.Root cause / design notes
Why an
arg_converteris mandatory. Hermes's tool-call body is a{"name":..., "arguments":{...}}wrapper, so the whole wrapper lands inTOOL_ARGS.ParserEngine._compute_arg_deltareturns the raw delta and never calls_fix_arg_typeswhenarg_converter is None, while non-streaming extraction always coerces via_build_extracted_result. Without a converter, streaming and non-streaming silently disagree on argument types.Why a verbatim scanner rather than re-serialising. The engine diffs successive converter outputs and requires each to extend the previous one (
startswith); a violation silently drops argument deltas. Re-serialising throughjson.dumpschanges whitespace and closes unterminated structures differently across ticks, so the only prefix-stable output is a verbatim substring of the input._hermes_arg_convertertherefore carves theargumentsvalue out with a string/escape-aware scanner, mirroring_inkling_arg_converter, which has the same wrapper shape. The scanner is also what makes an argument literally named"name"safe — the bug class that started this work (#49535).Why
token_id_terminalsis empty.StreamingParserEngine._process_lex_tokensenters a strict token-id mode once any real token IDs have been seen, and refuses to accept a text-matched terminal that is listed intoken_id_terminalsunless it was also pre-lexed from an actual matching token ID. Hermes delimiters reliably arrive as literal text (ParserEngine.adjust_requestforcesskip_special_tokens=False), so token-ID pre-lexing buys nothing and would starve text matching for tokenizers where the delimiter is not a single true token. Inkling opts out the same way.Non-streaming strictness is preserved explicitly. The legacy parser required the whole tool-call body to parse as valid JSON and reported no tool calls at all otherwise. The engine's generic EOS path is best-effort and would happily emit a tool call from a truncated wrapper, so
HermesParser.extract_tool_calls_from_contentkeeps the single-shotjson.loadscheck. Streaming keeps the engine's normal best-effort behavior.Longcat.
LongcatFlashToolParsersubclassedHermes2ProToolParserand customised the delimiters by assigningtool_call_start_token/tool_call_end_token/tool_call_regex, none of which the engine consults. Left as-is, the migration would have silently broken it (13 of its 17 tests). It now has its ownHermesParsersubclass overriding the delimiters as class attributes, which is whyTOOL_CALL_START/TOOL_CALL_ENDare class attributes onHermesParserat all.Test-suite change, and why
tests/parser/test_streaming.pyused Hermes as its reference non-engine tool parser (a section titled "Engine-based reasoning + non-engine tool parser", assertingengine_based_streaming is False). This migration invalidates that premise by design.Those tests now use
Granite4ToolParser, which is still non-engine, uses the identical<tool_call>/</tool_call>delimiters soMODEL_OUTPUTis unchanged, and hassupports_required_and_named=Truenatively — which also removes the need for theautousefixture that was force-setting that flag on Hermes.test_parse_delta_finished_appends_remaining_argsstill uses Hermes explicitly, since its subject is that parser's unstreamed-args behavior. No test was weakened or deleted; only the reference model changed.Out of scope (pre-existing, not introduced here)
While validating the above I found that
tool_choice="required"crashes for any engine-based parser whosesupports_required_and_namedisTrue:StreamState.advance()passes only the latest delta for engine-based parsers, butextract_required_tool_call_streaming()expects the full accumulated text, so the final chunk reachespartial_json_parseras a bare]and raisesIndexError: pop from empty list.This reproduces on the already-merged
KimiK2ToolParserwith no Hermes involvement, so it is not caused by this PR. It is normally unreachable because every engine-based parser setssupports_required_and_named=Falseviastructural_tag_model+VLLM_ENFORCE_STRICT_TOOL_CALLING(default on), which Hermes continues to do here. Happy to file it separately.Not a duplicate
gh pr list --repo vllm-project/vllm --search "hermes parser engine" --state alland--search "migrate parser engine in:title is:open"— no PR migrates Hermes. In-flight migrations cover other models (Kimi K3 [Parser] Migrate Kimi K3 to Parser Engine #50229, llama3_json/llama4_json [Frontend] [Parser] Migrate llama3_json/llama4_json to the streaming parser engine #51577, Granite [Bugfix][Tool Parser] Migrate Granite to the streaming Parser Engine #49648, Olmo3 [Bugfix] Migrate Olmo3 reasoning parser to the streaming parser engine #48160, poolside_v1 [Tool Parser] poolside_v1: migrate to the declarative parser engine #47167).vllm/parser/hermes.pydoes not exist onmain; the registry still points at the legacyhermes_tool_parser/Hermes2ProToolParser.vllm/tool_parsers/hermes_tool_parser.py(fix: reject duplicate JSON keys in Hermes tool parser for streaming consistency #48864, [Bugfix][ToolParser] Make Hermes streaming tool call boundaries JSON string aware #45310, [Bugfix] Hermes tool parser: parse tool call JSON by object boundary, not literal </tool_call> #48353). This PR replaces that file wholesale, so it will conflict with them textually. Flagging explicitly so maintainers can sequence them — [Bugfix] Hermes tool parser: parse tool call JSON by object boundary, not literal </tool_call> #48353 in particular changes tool-call boundary semantics, and if it is preferred first I am happy to rebase on top of it.gh pr list --search "longcat"— no open PR touchesvllm/tool_parsers/longcat_tool_parser.py([Rust Frontend] Add LongCat tool parser #45773 is the Rust frontend).Test plan
Environment note: run offline against a torch-stubbed environment, so the CUDA/engine-dependent top-level
tests/conftest.pyis skipped via--confcutdir=tests/tool_parsers.Result: 105 passed, 1 skipped, 1 xfailed (the skip and xfail are pre-existing Granite4-specific cases, unchanged by this PR).
Per-suite, before vs after this change:
tests/tool_parsers/test_hermes_tool_parser.pytests/tool_parsers/test_longcat_tool_parser.pytests/parser/test_parse.py,test_streaming.py,test_include_reasoning.pyThe 4 added Hermes tests are one new case (
test_hermes_streaming_tool_call_with_name_key_argument) across the existingstream_intervalparametrisation (2/3/5/8), covering an argument literally named"name"— the bug class from #49535 — at multiple chunk granularities.Lint:
pre-commithooks ran on commit —ruff check,ruff format, andmypyfor Python 3.10 all passed.End-to-end, against a real server and model
Run separately on a T4 GPU, since the suite above is offline-only:
Result: 4 passed, 4 deselected (the deselected cases are the
granite4config). This boots a realvllm servewith--tool-call-parser hermesagainstmeta-llama/Llama-3.2-1B-Instructplus theminpeter/LoRA-Llama-3.2-1B-tool-vllm-ciadapter, and drives it through theopenaiclient — covering non-streaming, streaming, and parallel tool calls.The streaming case confirms argument type coercion survives the migration end to end, reconstructing
{'inserted': True, 'product_id': 7355608}— a real boolean and a real integer rather than strings, which is the divergence thearg_converterexists to prevent.Longcat has no e2e suite upstream, so its coverage here is the 17 unit tests only — worth noting since it was changed as a consequence of the Hermes migration rather than being its target.