[GPT-OSS] Strict tool call and constrained decoding for Harmony - #45560
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| return StructuralTag(format=suffix_tag) | ||
|
|
||
|
|
||
| def _patch_harmony(fmt: object) -> None: |
There was a problem hiding this comment.
We can define the tags directly in HarmonyParser
There was a problem hiding this comment.
Implemented complete structural tags in vLLM (inside HarmonyParser). Although still importing helpers from structural_tag_registry.py.
|
This pull request has merge conflicts that must be resolved before it can be |
| ) | ||
|
|
||
| if model == "harmony": | ||
| _patch_harmony(tag.format) |
There was a problem hiding this comment.
I would recommend implementing the complete structural tag directly in vLLM rather than relying on this kind of patch-based solution.
There was a problem hiding this comment.
Implemented complete structural tags in vLLM. Bypasses the bugs and better performance.
6c3a6f8 to
d8fe628
Compare
d8fe628 to
c441e9a
Compare
|
The structural tags help |
|
The change looks good overall. The only concern I have left, that we discussed via chat, is if a client passes |
Good catch. If a client passes We can turn a user-passed I didn't do this to mostly to avoid complexity. My reasoning was that Will update |
|
LGTM |
35cee12 to
eba1b58
Compare
|
Rebased and added commit that "converts a user-passed request.structured_outputs into a reasoning-aware structural_tag". Unit tests pass locally; BFCL guided looks good. While testing, found an edge case where the final tags leak into the constrained output because the model generates them char by char instead of as a single special token e.g. This isn't a Harmony specific bug however. Will create and issue for this later. |
bbrowning
left a comment
There was a problem hiding this comment.
We talked about the constrained edge case and the tag leak over Slack, and I'm ok deferring solving that for now as it looks like a bit deeper wiring into Structural Tags here to have it wire those special tokens in as ids instead of text so that we don't end up generating special tokens via multiple subword regular tokens. Not something to forget, but a deeper fix into how we wire these structural tags into xgrammar.
The only other thing that stands out to me is this removes the only real implementation of prepare_structured_tag, leaving the only callsite calling into the no-op implementation. That can also be deferred for the sake of fixing this, but effectively becomes dead code we can cleanup unless there is future intent to use that for something. I'm ok deferring that because it is technically part of our reasoning parser interface today since it's defined on ReasoningParser, so that may need some wider consideration before removal.
Thanks for all the work cleaning this up for this family of models!
|
|
|
|
7170d3d to
4e40720
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
Thanks for driving this PR @yzong-rh. I'm interested in seeing this land. Some of our customers are interested in using this feature. |
Signed-off-by: Yifan Zong <yzong@redhat.com>
Signed-off-by: Yifan Zong <yzong@redhat.com>
4e40720 to
c4181af
Compare
|
I reviewed the latest changes and still good to merge this - thanks! One thing I'll note for a potential future follow-up - I believe if tool_choice="auto", user supplies a response_format/structured_outputs in their request (like json schema), and at least one tool has strict set, we end up not constraining the final output if the model decides not to call a tool. I believe we'd need to take that user-supplied json schema into account when building our structural tag in that case so that it gets applied when guiding the final channel output. That's a few combinations of things to hit this, and I could be reading things wrong here so worth double-checking before trying to implement that. Either way, this change as-is gets things cleaned up nicely for the general cases where we need to guide outputs! |
…-project#45560) Signed-off-by: Yifan Zong <yzong@redhat.com>
…-project#45560) Signed-off-by: Yifan Zong <yzong@redhat.com> Signed-off-by: Tej Kiran <kiran.tej@amd.com>
…-project#45560) Signed-off-by: Yifan Zong <yzong@redhat.com> Signed-off-by: root <root@smci355-ccs-aus-m02-09.cs-aus.dcgpu>
Purpose
Following #45003 and #45396, attempt to implement constrained decoding for tool calls for Harmony models.
Related: #35904. Also "embed content constraints in structural tags".
Problem
vLLM's constrained decoding for the Harmony delays enforcing
response_formatconstraints (json_object,json_schema,structural_tag) until<|channel|>final...<|message|>is detected. This works correctly for JSON formats but creates a mismatch for structural-tag tool calling, since those constraints need to apply during the commentary/tool-call phase rather than only in the final-message channel.Solution
Rewrite
json_object/json_schemaresponse_formats into a Harmony-aware structural_tag (encoding the analysis and final-message boundaries directly in the grammar) so that constrained decoding is governed entirely by the structural tag throughout the entire generation.HarmonyParser'sadjust_requestso that JSONresponse_formatgets wrapped into structural tags that apply over the entire generation.adjust_requestfor Harmony modelsGptReasoningParser's reasoning_end methods and tests (they are no longer required).Test Plan
BFCL Multiturn
Manual with:
Benchmarking
Test Results:
Chat Completions
Responses
cc @bbrowning @chaunceyjiang
Used AI assistance
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.