feat(gpt-oss): combine xgrammar structural_tag grammar with Harmony parser - #4907
windreamer wants to merge 1 commit into
Conversation
9d1c914 to
1a0cb5e
Compare
There was a problem hiding this comment.
Pull request overview
This PR integrates xgrammar structural_tag guided generation with the existing GPT-OSS Harmony response parser so that tool-calling and structured outputs can be enforced via hard grammar constraints (instead of Harmony-native prompt injection), while keeping a legacy prompt-injection fallback when grammar construction is unavailable.
Changes:
- Extend the OpenAI protocol
ResponseFormatto supporttype='structural_tag'and carry astructural_tagpayload. - Update the GPT-OSS Harmony parser to build and attach Harmony-compatible
structural_taggrammars for (a) tool calling and (b) non-textresponse_formatmodes, with a prompt-injection fallback. - Revise and expand unit tests to validate structural_tag conversion/injection and the fallback behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lmdeploy/serve/openai/protocol.py |
Adds structural_tag to ResponseFormat so requests can carry structural-tag grammars end-to-end. |
lmdeploy/serve/parsers/_openai_harmony.py |
Implements structural_tag grammar generation for tool calling and response_format, plus legacy fallback injection. |
tests/test_lmdeploy/serve/parsers/test_gpt_oss_parser.py |
Updates tests to assert structural_tag behavior and legacy fallback when grammar building fails. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
1a0cb5e to
fe1f1dd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
lmdeploy/serve/parsers/_openai_harmony.py:109
ResponseFormat.typenow supports'structural_tag', but_maybe_inject_tool_grammar()treats any non-textresponse_format as needing conversion and will fall back to prompt injection (which clearsresponse_format) whentype=='structural_tag'. That makes client-supplied structural tags effectively unusable.
if fmt is not None and getattr(fmt, 'type', 'text') != 'text':
self._convert_response_format_to_harmony()
return
if fmt is not None and getattr(fmt, 'type', 'text') != 'text':
grammar = self._build_response_format_grammar(fmt)
219bee1 to
389ec5b
Compare
70f66e6 to
8aac090
Compare
8aac090 to
d5d8a5f
Compare
|
Thanks for the review! @lvhan028 Fully addressed in d5d8a5f (force-pushed): the What changedThe structural_tag is now a purely internal mechanism: the Harmony parser builds the grammar server-side (via I also verified the vLLM convention mentioned in the review: vLLM handles Harmony constraints server-side in Other review items (from Copilot, fixed in prior commits, kept in this rework)
All 29 tests in |
d5d8a5f to
a539a8d
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect allowed-tool handling and structural-tag request validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
lmdeploy/serve/parsers/_openai_harmony.py:203
- The documented fallback is not honored for exceptions raised during grammar construction or serialization: only the xgrammar imports are inside
try, so a validation/API error fromJSONSchemaFormat(and likewise the regex/structural-tag constructors below) escapesGptOssResponseParser.__init__instead of calling_convert_response_format_to_harmony(). Keep the conversion andmodel_dump_json()work under the exception handler, as_build_tool_grammar()does.
if fmt_type == 'json_schema':
# normalize dumps JsonSchema via by_alias: inner schema lives at
# key 'schema' (OpenAI wire format).
raw = (fmt.get('json_schema') or {}).get('schema')
lmdeploy/serve/parsers/_openai_harmony.py:207
- An empty regex is a valid value for the
regex_schema: strfield, butor '.*'converts it into an unconstrained wildcard. That weakens the promised hard constraint for this edge case; distinguishNonefrom''when choosing the default.
elif fmt_type == 'json_object':
lmdeploy/serve/parsers/_openai_harmony.py:211
- The new
structural_tagresponse-format type is treated as unsupported here, so_maybe_inject_tool_grammarfalls through to legacy prompt injection, serializes the grammar as prose, and clears it. A caller that supplies an already-built structural tag therefore loses hard grammar enforcement; pass this type through unchanged before handling the legacy formats.
return None
lmdeploy/serve/parsers/_openai_harmony.py:50
- When
tool_choiceisAllowedToolChoiceand top-levelrequest.toolsis omitted,allowed_tools.toolsis the source of the allowed definitions (the base parser populatestoolsfrom it), but this branch discards them asNone. The subsequenthas_toolscheck skips_build_tool_grammar, so GPT-OSS cannot emit any requested allowed tool; preserve/render those definitions here.
rendered_tools = None
elif getattr(request.tool_choice, 'type', None) == 'function':
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| else: | ||
| # auto/required/allowed_tools: keep all tools. | ||
| rendered_tools = [item.model_dump() for item in request.tools] |
…r structural_tag
Combine xgrammar structural_tag as a grammar constraint layer with the
existing GPT-OSS Harmony parser so that tool calling and structured
output are enforced at generation time instead of via prompt injection.
The structural_tag is an internal mechanism only: it is built server-side
by the Harmony parser and reaches the engine through the regular
response_format dict. The OpenAI protocol is not extended (per review,
guided decoding extras are kept out of response_format).
Parser (lmdeploy/serve/parsers/_openai_harmony.py):
- Inject the grammar after normalize_chat_request so response_format is
already the normalized engine-level dict; dict access replaces the
Pydantic attribute access throughout.
- With tools + tool_choice != 'none': build a Harmony-compatible
structural_tag via xgrammar.get_model_structural_tag('harmony', ...)
and attach it to response_format; tool grammar takes priority over any
response_format (consistent with vLLM).
- Without tools + non-text response_format (json_schema / regex_schema /
json_object): wrap the schema in a Harmony final-channel structural_tag
instead of injecting it into the prompt.
- Grammar construction failure falls back to the original prompt
injection path and clears response_format.
Bug fixes (from review):
- AllowedToolChoice crash: __init__ tool filtering accessed
.function.name on any non-string tool_choice; now checks
getattr(tool_choice, 'type') == 'function'.
- Dead BaseModel.schema branch: _build_response_format_grammar had an
elif hasattr(schema, 'schema') branch that picked up the deprecated
BaseModel.schema method when json_schema had no inner schema; removed.
- Tool grammar failure left response_format intact: now falls back to
prompt injection and clears it so json/regex modes cannot conflict
with Harmony tool-call constraints downstream.
Tests (tests/test_lmdeploy/serve/parsers/test_gpt_oss_parser.py):
- Regression tests for the three bug fixes.
- TestGptOssResponseFormatGrammarConversion: json_schema /
regex_schema / json_object -> structural_tag conversion,
grammar-failure fallback, and BaseModel.schema leak prevention.
- TestGptOssToolGrammarInjection: required / auto / specific function /
none / allowed_tools / priority / failure-fallback.
- Consolidated the existing suite: merged redundant fixtures, removed
dead helpers, parametrized overlapping cases.
a539a8d to
60a81eb
Compare
Motivation
GPT-OSS Harmony response parsing relies on
openai_harmonyfor token stream parsing, but lacks grammar-level constraints —tool_choice=requiredcannot enforce tool calling, andresponse_formatcan only be injected into the prompt (soft constraint). This PR combines xgrammar structural_tag as a grammar constraint layer with the existing Harmony parser to achieve hard guarantees.Modification
Protocol (
lmdeploy/serve/openai/protocol.py)Add
structural_tagtoResponseFormat.typeand astructural_tag: dict[str, Any] | Nonefield.Parser (
lmdeploy/serve/parsers/_openai_harmony.py)_convert_response_format_to_harmony()with_maybe_inject_tool_grammar()in__init__.tool_choice != 'none': build a Harmony-compatible structural_tag viaxgrammar.get_model_structural_tag("harmony", ...), set it onresponse_formatwithout clearing (grammar reaches the engine).response_format: wrapjson_schema/regex_schema/json_objectinto a Harmony final-channel structural_tag via_build_response_format_grammar(), no longer injecting into the prompt._convert_response_format_to_harmony(inject system prompt + clear).response_formatpresent: tool grammar takes priority (consistent with vLLM).Bug fixes (found during review)
__init__tool filtering usednot isinstance(tool_choice, str)then accessed.function.name, crashing onAllowedToolChoice(type='allowed_tools'). Fixed withgetattr(tool_choice, 'type') == 'function'.BaseModel.schemabranch:_build_response_format_grammarhad anelif hasattr(schema, 'schema')branch that always picked up the deprecatedBaseModel.schemamethod (a bound method, notNone) whenjson_schemahad no inner schema, producing invalid grammar. Removed._build_tool_grammarreturnedNone, a non-textresponse_formatwas left on the request, whichnormalize_chat_requestwould pass through to the engine — re-enabling json/regex modes that conflict with Harmony tool-call constraints. Now falls back to prompt injection and clears.Tests (
tests/test_lmdeploy/serve/parsers/test_gpt_oss_parser.py)TestGptOssResponseFormatGrammarConversion: verifyjson_schema/regex_schema/json_object→ structural_tag conversion, grammar-failure fallback, andBaseModel.schemaleak prevention.TestGptOssToolGrammarInjection: verifyrequired/auto/specific function/none/allowed_tools/priority/failure-fallback.BC-breaking
None. Adding
structural_tagtoResponseFormatis purely additive. The legacy_convert_response_format_to_harmonyis retained as fallback.Checklist
test_gpt_oss_parser.py.get_model_structural_tag.