Repository navigation
[https://nvbugs/6858496][fix] Fail disagg gen requests whose grammar matcher cannot be created - #20903
[https://nvbugs/6858496][fix] Fail disagg gen requests whose grammar matcher cannot be created#20903trtllm-agent wants to merge 1 commit into
Conversation
…matcher cannot be created GuidedDecoder._init_disagg_gen_requests created the grammar matchers of generation-only requests without per-request error handling, so an error from the matcher factory propagated into the executor loop. Context requests already handle the same error in _build by failing only the affected request. Catch the error per request in _init_disagg_gen_requests: the slot keeps no matcher, its rollback counters are cleared, and the request is reported by the next _build through the existing failed-requests path, which the executor turns into an error response for that request. Other requests in the batch are unaffected. Add unit tests for the failing request, the other request of the batch, and the rollback passes after a failed creation. Signed-off-by: trtllm-agent <296075020+trtllm-agent@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesGuided decoder initialization failure handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description explains the failure mode, the per-request error-handling solution, affected behavior, and relevant unit and deployment tests. It covers the required Description and Test Coverage sect… Title check The title follows the required ticket, type, and summary format. It clearly identifies the fix for disaggregated generation requests whose grammar matcher cannot be created. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the failure mode, the per-request error-handling solution, affected behavior, and relevant unit and deployment tests. It covers the required Description and Test Coverage sections. The repository checklist is not reproduced, but the critical content is complete.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The fix contains the failure at the right point for GuidedDecoder: a None matcher is already skipped by _build's advance branch, and the executor's _handle_guided_decoder_errors handles the reporting. Both tests target that class.
Two follow-ups:
- One-model speculative decoding is only partly covered.
CapturableGuidedDecoderreuses_init_disagg_gen_requests, so the crash goes away there too. But the failure list never reaches the executor (see inline), so the request silently runs unguided. Either report the failure on that path or state the limitation in the PR description, which currently claims the request is always failed. - Consider rejecting invalid grammars when the request arrives. The context path and this new path both catch the error only after the request is scheduled. A check at request entry (e.g. compile the
response_formatgrammar once in the serving layer) would return a 400 before any KV transfer happens, and the disagg client would not have to wait through a gen-side retry and timeout. This can be a separate PR.
| req.seq_slot] = self.grammar_matcher_factory.create( | ||
| slot] = self.grammar_matcher_factory.create( | ||
| req.guided_decoding_params) | ||
| except Exception as e: |
There was a problem hiding this comment.
In CapturableGuidedDecoder, both init_disagg_gen_requests and build are @hostfunc callbacks. build() returns the launch handle, not _build's list, and execute() returns that value to _execute_guided_decoder_if_present, which discards it. PyExecutor.guided_decoder is None in that mode. So for one-model spec decoding plus disagg, this branch clears the matcher, init_failed_requests is never delivered, and the request keeps generating with no grammar constraint and no error. The worker no longer dies, but the response silently ignores response_format.
Possible fixes: have the host side read init_failed_requests after _drain_host_functions() and route it to the executor's error handling, or at least log that the request will run unguided. A test with CapturableGuidedDecoder would cover this path.
| reported as failed by the next build, like a context request whose | ||
| matcher creation fails in build. | ||
| """ | ||
| self.init_failed_requests = [] |
There was a problem hiding this comment.
Clearing the list here assumes every _init_disagg_gen_requests call is followed by a _build on the same batch before the next init. That holds for the current PyExecutor call sites. Please add a short note to the init_failed_requests comment in __init__ stating this ordering requirement, so a future caller that runs init twice (e.g. a retry or a second microbatch) does not drop failures silently.
Description
In disaggregated serving, a generation worker creates the grammar matcher of each generation-only request in
GuidedDecoder._init_disagg_gen_requests, before the model forward. That method calledgrammar_matcher_factory.create()with no per-request error handling. An error from the factory therefore escaped into the executor loop and terminated it, and every later request to that worker failed. Context requests create their matchers in_build, which already catches this error and fails only the affected request._init_disagg_gen_requestsnow catches the error per request, in the same way as_build:(request_id, error_msg)failed-requests list returned by the next_build. The executor already turns that list into an error response for the request, via_handle_guided_decoder_errors;Test Coverage
tests/unittest/_torch/misc/test_guided_decoder_bitmask.py:test_disagg_init_failure_fails_only_that_request: the failing request is reported once by the next build, a matcher left in its slot is not reused, and the other request stays guided.test_disagg_init_failure_is_skipped_by_rollback: the rollback passes run without error after a failed creation.On H100, both fail without the change (the factory error is raised out of
_init_disagg_gen_requests) and pass with it. All 24 tests in the file pass with the change.tests/unittest/_torch/speculative/hw_agnostic/(test_sa_guided_rollback.py,test_guided_hostfunc_drain.py,test_dflash_guided_rollback.py,test_pard_guided_rollback.py) pass before and after the change (30 passed).guided_decoding_backend: llguidanceand an NIXL cache transceiver, plus the disaggregated server), built from themainbase of this PR. A request whose matcher cannot be created on the generation worker:With guided decoding also enabled on the context worker, the results are the same before and after: valid guided requests and ordinary requests return 200.
Fixes https://nvbugs/6858496
🤖 Generated with Claude Code
Dev Engineer Review
_build.QA Engineer Review
tests/unittest/_torch/misc/test_guided_decoder_bitmask.py. It addstest_disagg_init_failure_fails_only_that_requestandtest_disagg_init_failure_is_skipped_by_rollback. Both cover matcher creation failure; the first also checks that another request in the batch remains guided. The reported test results are 24 passing tests in this file and 30 passing tests across four related rollback and host-function files.tests/integration/test_lists/. The supplied context does not identify it as an integration test requiring CI or manual-QA listing.Per-File QA Perspective
tensorrt_llm/_torch/pyexecutor/guided_decoder.py: Verify that a factory error is isolated to the affected disaggregated generation request, clears its matcher and rollback counters, and reaches the existing failed-request response path on the next build.tests/unittest/_torch/misc/test_guided_decoder_bitmask.py: The added tests cover per-request failure isolation, retention of guided decoding for another request, one-time error reporting, and rollback behavior. No matching CI or manual-QA list entry was found.