[https://nvbugs/6284101][fix] Fix guided decoding for Harmony parser - #15741
[https://nvbugs/6284101][fix] Fix guided decoding for Harmony parser#15741dongfengy wants to merge 2 commits into
Conversation
558e70c to
ab42d37
Compare
WalkthroughAdds reasoning-parser-aware guided-decoding adaptation for Harmony and other reasoning formats. Wires the shared adaptation into LLM sampling and OpenAI serving. Adds final-content extraction and reasoning-aware JSON evaluation. ChangesReasoning-parser guided decoding and evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LLM
participant ReasoningParser
participant JsonModeEval
Client->>LLM: Submit guided-decoding parameters
LLM->>ReasoningParser: Resolve and adapt parser framing
ReasoningParser-->>LLM: Return adapted parameters
LLM-->>JsonModeEval: Return generated output
JsonModeEval->>ReasoningParser: Extract final content
ReasoningParser-->>JsonModeEval: Return parsed content
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/evaluate/interface.py`:
- Around line 90-109: The fallback parsing in interface.py is catching all
Exception in both the harmony adapter path and the ReasoningParserFactory path,
which can mask real bugs. Update the try/except blocks around
get_harmony_adapter().harmony_output_to_openai and parser.parse(text) to catch
only the expected import, parsing, or type-related failures, and leave
unexpected exceptions to surface. Keep the same fallback-to-text behavior, but
narrow the exception handling in these parser paths to the smallest specific set
possible.
In `@tensorrt_llm/evaluate/json_mode_eval.py`:
- Around line 33-48: Add a return type annotation to _load_json_from_generation
in json_mode_eval.py, and if helpful introduce a local alias for the JSON value
type to keep the signature readable. Update the function signature of
_load_json_from_generation (and any related helper typing if needed) so the new
helper is fully annotated per the codebase guidelines.
In `@tests/unittest/evaluate/test_generation_content.py`:
- Around line 1-2: The new test file currently has only an SPDX-only header, but
it needs the repository’s full NVIDIA copyright/license header block. Update the
top-of-file header in test_generation_content.py to match the required NVIDIA
format used elsewhere in the repo, including the year of the latest meaningful
modification and the standard Apache-2.0 license notice.
- Around line 23-42: Add a test in test_generation_content for
extract_final_content_from_generation that combines token_ids with
reasoning_parser="qwen3" and asserts the qwen3 path still wins. Mirror the
existing test_extract_final_content_does_not_guess_harmony_from_tokens setup by
monkeypatching tensorrt_llm.serve.harmony_adapter.get_harmony_adapter to raise
if called, then verify the function returns the expected final content. This
should cover the explicit non-Harmony parser branch even when token_ids are
present and prevent future regressions in extract_final_content_from_generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 318d2a29-586e-453d-ab59-726c7b606634
📒 Files selected for processing (7)
tensorrt_llm/evaluate/interface.pytensorrt_llm/evaluate/json_mode_eval.pytensorrt_llm/llmapi/llm.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/serve/openai_protocol.pytests/unittest/evaluate/test_generation_content.pytests/unittest/llmapi/test_sampling_params.py
|
/bot run --disable-fail-fast |
ab42d37 to
059d1c9
Compare
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #56512 [ run ] triggered by Bot. Commit: |
|
PR_Github #56512 [ run ] completed with state
|
dongfengy
changed the title
0b38f28 to
baf824c
Compare
dongfengy
changed the title
|
/bot run --disable-fail-fast |
|
PR_Github #56678 [ run ] triggered by Bot. Commit: |
|
PR_Github #56678 [ run ] completed with state
|
baf824c to
0dac1c9
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #56761 [ run ] triggered by Bot. Commit: |
|
PR_Github #56761 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #56850 [ run ] triggered by Bot. Commit: |
|
PR_Github #56850 [ run ] completed with state
|
0dac1c9 to
0cbb805
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #57856 [ run ] triggered by Bot. Commit: |
|
PR_Github #65085 [ run ] triggered by Bot. Commit: |
|
PR_Github #65085 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
PR_Github #65210 [ run ] triggered by Bot. Commit: |
|
PR_Github #65210 [ run ] completed with state
|
Apply structural guidance only after Harmony enters the final channel. Preserve unconstrained reasoning while keeping JSON and grammar evaluation scoped to final content. Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
0d9ba14 to
ff0987a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
/bot run --disable-fail-fast |
|
PR_Github #65370 [ run ] triggered by Bot. Commit: |
|
PR_Github #65370 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65431 [ run ] triggered by Bot. Commit: |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The fix looks right and the shared adapter is a real improvement over the duplicated serving logic (it also fixes the latent unbound-content NameError in the old _response_format_to_guided_decoding_params when no guide field matched). Two things should land with this PR so CI actually guards the fix:
-
waives.txtstill skips the regression test.accuracy/test_llm_api_pytorch.py::TestGPTOSS::test_eagle3_guided_decoding_4gpus[one_model]is SKIPped at tests/integration/test_lists/waives.txt:59 (under https://nvbugs/5596343). Since this PR is the fix for that failure (split into 6284101) and the description says the exact 4-GPU regression passed on head, remove the waiver here — otherwise the fixed test stays disabled and nothing in CI covers the bug. -
The new unit-test file never runs in CI — see the inline comment on
tests/unittest/evaluate/test_generation_content.py.
One scope question worth a sentence in the description or a tracked follow-up: the raw-path adaptation is deliberately gated to xgrammar (resolve_raw_guided_decoding_reasoning_parser at tensorrt_llm/llmapi/reasoning_parser.py:241), so GPT-OSS + llguidance via raw LLM.generate() still applies the guide to the whole transcript. Since structural tags are xgrammar-only that's understandable, but it leaves the original symptom reachable on the other backend — is a follow-up planned, or should llguidance + gpt_oss at least warn?
| # final output. Add that model-aware framing here so | ||
| # xgrammar applies the guide only to Harmony's final | ||
| # channel. Other formats preserve the original guide. | ||
| sampling_params.guided_decoding = ( |
There was a problem hiding this comment.
This reassigns guided_decoding on the caller's own SamplingParams object (generate_async doesn't copy it). After the first request, the user's GuidedDecodingParams(json=...) is silently replaced by an xgrammar structural-tag guide; if they reuse the same SamplingParams against another LLM (e.g. an llguidance-backed one), the structural tag — unsupported there — is what gets submitted, and their original schema is unrecoverable. The existing mutations in this method (end_id, logits processors) are additive; swapping the guide type is observably different. Consider applying the adaptation to a copy, or at least deferring the rewrite to the point where the request params are marshalled rather than the shared object.
| stag_format = ResponseFormat(type="structural_tag", format=stag_format) | ||
| return GuidedDecodingParams(structural_tag=stag_format.model_dump_json( | ||
| by_alias=True, exclude_none=True)) | ||
| return adapt_guided_decoding_params_for_reasoning_parser( |
There was a problem hiding this comment.
Minor behavior change from the refactor: the old code round-tripped the built structural tag through ResponseFormat.format, which is typed xgrammar.structural_tag.Format, so the tag (including the embedded user schema/regex content) was pydantic-validated at request time and a bad one returned a 4xx. adapt_guided_decoding_params_for_reasoning_parser now json.dumps a raw dict, so malformed structural tags are only caught later in the executor. Probably acceptable since the dict layout is constant and covered by the new unit tests, but if request-time validation was intentional, consider validating the dict against xgrammar.structural_tag.Format inside the shared helper.
Signed-off-by: Dongfeng Yu <dongfengy@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/llmapi/test_sampling_params.py (1)
216-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the added test functions.
Add
-> Noneto each test function. Add precise parameter annotations to the parametrized test functions.
tests/unittest/llmapi/test_sampling_params.py#L216-L216: add a return annotation.tests/unittest/llmapi/test_sampling_params.py#L242-L242: add a return annotation.tests/unittest/llmapi/test_sampling_params.py#L261-L261: add a return annotation.tests/unittest/llmapi/test_sampling_params.py#L283-L283: add parameter and return annotations.tests/unittest/llmapi/test_sampling_params.py#L297-L297: add parameter and return annotations.tests/unittest/llmapi/test_sampling_params.py#L314-L314: add parameter and return annotations.tests/unittest/llmapi/test_sampling_params.py#L330-L330: add a return annotation.tests/unittest/llmapi/test_sampling_params.py#L340-L340: add a return annotation.tests/unittest/llmapi/test_sampling_params.py#L363-L363: add a return annotation.As per coding guidelines, “Annotate every function.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/llmapi/test_sampling_params.py` at line 216, Annotate every added test in tests/unittest/llmapi/test_sampling_params.py: add -> None to test_harmony_guided_decoding_triggers_on_final_channel and the tests at lines 242, 261, 330, 340, and 363; add precise parameter annotations plus -> None to the parametrized tests at lines 283, 297, and 314.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Around line 242-258: Add a test alongside
test_harmony_guided_decoding_accepts_json_schema_string that passes a Pydantic
model through adapt_guided_decoding_params_for_reasoning_parser with
HARMONY_REASONING_PARSER, then parse the resulting structural_tag and assert its
JSON-schema content matches the model’s model_json_schema() output.
---
Nitpick comments:
In `@tests/unittest/llmapi/test_sampling_params.py`:
- Line 216: Annotate every added test in
tests/unittest/llmapi/test_sampling_params.py: add -> None to
test_harmony_guided_decoding_triggers_on_final_channel and the tests at lines
242, 261, 330, 340, and 363; add precise parameter annotations plus -> None to
the parametrized tests at lines 283, 297, and 314.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 640e0289-9da6-4aae-b780-a7d78937233f
📒 Files selected for processing (9)
tensorrt_llm/evaluate/interface.pytensorrt_llm/evaluate/json_mode_eval.pytensorrt_llm/llmapi/llm.pytensorrt_llm/llmapi/reasoning_parser.pytensorrt_llm/serve/openai_protocol.pytests/integration/test_lists/test-db/l0_cpu.ymltests/integration/test_lists/waives.txttests/unittest/evaluate/test_generation_content.pytests/unittest/llmapi/test_sampling_params.py
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- tensorrt_llm/llmapi/llm.py
- tensorrt_llm/evaluate/interface.py
- tensorrt_llm/evaluate/json_mode_eval.py
- tests/unittest/evaluate/test_generation_content.py
- tensorrt_llm/serve/openai_protocol.py
I have fixed waives.txt and added new tests to CI. Regarding scope, yes, this fixes all instances of Harmony parser + xgrammar and has been verified with solid tests. Harmony + llguidance requires significantly more work and may even require architectural changes. I would suggest treating this as a feature request so that we don’t bloat this bug-fix PR. |
|
PR_Github #65431 [ run ] completed with state
|
ZhanruiSunCh
left a comment
ZhanruiSunCh
left a comment
There was a problem hiding this comment.
LGTM for infra part.
crazydemo
left a comment
crazydemo
left a comment
There was a problem hiding this comment.
Review summary - CONCERNS
Verdict: The code change is correct, well-factored (shares the adapter with the serving path and fixes the latent unbound-content NameError / schema-alias mangling), and unit-tested. It cannot merge as-is: the branch is in conflict (mergeable_state = dirty) and the fix only covers the xgrammar backend, leaving the original bug reachable on llguidance.
Issues
- [MAJOR] repo state - PR is not mergeable (
mergeable_state = dirty); rebase and resolve conflicts, then get CI green. - [MAJOR]
tensorrt_llm/llmapi/reasoning_parser.py:241- GPT-OSS + llguidance still constrains the whole transcript (partial fix). - [MINOR]
tests/unittest/llmapi/test_sampling_params.py:216- added tests lack type annotations. - [NIT]
tests/unittest/evaluate/test_generation_content.py:1- SPDX-only header, missing full NVIDIA license block.
QA view
- Test coverage: partial - unit tests cover the adaptation and resolver functions and
extract_final_content_from_generationwell; the GPT-OSS+llguidance branch and thejson_mode_evalscoring path have no unit test, and the raw e2e Harmony guided-decoding path relies solely on the re-enabled 4-GPU accuracy test. - SM coverage: architecture-independent - pure Python structural-tag construction, no arch guards; integration coverage runs on B200, new units on CPU.
- Test code: missing annotations, SPDX-only header on the new file, Harmony conversion asserted only for the
jsonguide (not json_object/regex/grammar), PEP 604 syntax needs py>=3.10. - Test time: small - one fast CPU unit file added; one 4-GPU accuracy test re-enabled by waiver removal (pre-existing test, adds GPU time).
- Needs
/qa-verify: yes - waiver removed to re-enable a 4-GPU regression test and test-list infra changed; QA should confirm that test passes post-rebase and the new CPU unit test is actually collected in CI, and note the llguidance path is unfixed/untested.
Does this actually fix nvbugs/6284101?
Partial. For gpt_oss + xgrammar, llm.py:1396-1413 rewrites the guide into a triggered_tags structural tag keyed on the Harmony final-channel marker, so the constraint no longer binds the reasoning transcript - the failing path is fixed and now guarded by the unwaived 4-GPU test. But resolve_raw_guided_decoding_reasoning_parser (reasoning_parser.py:239-243) gates on guided_decoding_backend == "xgrammar", so GPT-OSS with the llguidance backend returns None, no final-channel framing is applied, and the guide still constrains the full transcript. That path remains uncovered.
Possible new issues
- Serving path now serializes via
json.dumpsrather thanResponseFormat.model_dump_json(by_alias=True, exclude_none=True). This is the intended fix (preserves a top-levelschemaproperty, tested), but any consumer relying on the old exclude_none/by_alias behavior would see a more literal JSON string. Content dicts contain no None values, so risk is low. - The Harmony branch of
extract_final_content_from_generationswallows a broad exception tuple and falls back to raw text; a malformed adapter response would silently score against raw text. Eval-only, so limited blast radius.
What I could not verify
Runtime behavior of the unwaived 4-GPU test post-rebase; whether the new unittest/evaluate/test_generation_content.py list entry is actually collected by the CPU CI job (an earlier reviewer flagged it as not running); and the exact type of guided_decoding_params.json reaching the serving path in production (I only see the diff).
Findings not attached to a line
- [MAJOR]
?:?- PR is not mergeable (mergeable_state = dirty)- The context reports mergeable=false / mergeable_state=dirty, i.e. the branch has merge conflicts against main. Regardless of code correctness this cannot be merged as-is; it must be rebased and the conflicts resolved, then CI (including the newly-unwaived 4-GPU TestGPTOSS::test_eagle3_guided_decoding_4gpus[one_model]) must be green before merge.
Automated review by NVCortex Lite, run by @crazydemo.
| ) | ||
|
|
||
|
|
||
| def test_harmony_guided_decoding_triggers_on_final_channel(): |
There was a problem hiding this comment.
[MINOR] Added test functions lack type annotations
Per the repo coding guideline ("annotate every function"), the new test functions and parametrized tests (this one and those at lines 242, 261, 283, 297, 314, 330, 340, 363) should carry -> None and precise parameter annotations. Purely a style/consistency nit; does not affect behavior.
| @@ -0,0 +1,88 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
There was a problem hiding this comment.
[NIT] Header is SPDX-only, missing full NVIDIA license block
Other test files in the repo carry the full NVIDIA copyright + Apache-2.0 header. This new file only has the two-line SPDX header. Add the standard block to match repo convention.
crazydemo
left a comment
crazydemo
left a comment
There was a problem hiding this comment.
Review summary - Approve (non-blocking)
Approving so this is not blocked on me. The points raised in my review comment above are non-blocking — please read them and address what you agree with before merging.
Worth doing before this is relied on: This is a bug fix that removes a waiver (waives.txt:59) to re-enable a 4-GPU accuracy regression test, and it changes test-list infra (l0_cpu.yml). QA should confirm the unwaived TestGPTOSS::test_eagle3_guided_decoding_4gpus[one_model] actually passes post-rebase and that the new CPU unit test is discovered/run in CI, before this is trusted. The llguidance path is unfixed and untested.
Automated review by NVCortex Lite, run by @crazydemo.
7 participants
Dev Engineer Review
LLM.generate().QA Engineer Review
unittest/evaluate/test_generation_content.pytotests/integration/test_lists/l0_cpu.yml.TestGPTOSS::test_eagle3_guided_decoding_4gpus[one_model]fromtests/integration/test_lists/waives.txt.Description
Fix guided decoding for the raw Python
LLM.generate()path when GPT-OSS uses the Harmony protocol with xgrammar. Caller-provided JSON, regex, and grammar constraints are enriched with model-aware Harmony framing so they apply only after the final-channel marker instead of constraining the reasoning transcript.The serving path keeps its existing behavior while sharing the same reasoning-parser adapter. JSON-mode evaluation now scores parser-selected final content, covering plain output, explicit Harmony output, and explicit normal reasoning-parser output without guessing a parser for unrelated models.
Raw guided-decoding behavior remains unchanged for normal reasoning parsers and llguidance. This PR does not change the public API.
Test Coverage
059d1c9a76.059d1c9a76: passed.059d1c9a76: passed with JSON accuracy 78/100 and grammar accuracy 96/100 (threshold 50.369).PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.