[Issue-13318][fix] Trim context chunks in _prepare_tp_inputs to prevent over-admission crash - #13351
[Issue-13318][fix] Trim context chunks in _prepare_tp_inputs to prevent over-admission crash#13351yifjiang wants to merge 1 commit into
Conversation
…nt over-admission crash The MicroBatchScheduler admits a batch using a reuse-discounted token budget: a last-chunk context request with `reusable > 0` is charged only `max(0, context_remaining - reusable)` tokens in the admission decision (see `_reuse_adjusted_compute` in `scheduler/scheduler.py` and the matching C++ logic). `_prepare_tp_inputs`, however, extends `position_ids` by the full `context_chunk_size` per context request regardless of reuse: the reusable prefix still needs its position embeddings computed in the forward pass. When `max_num_tokens` is tight, the admission-vs-forward mismatch lets `len(position_ids)` overshoot `self.max_num_tokens` by up to `min(chunk_size, reusable)` per offending request, tripping the `assert total_num_tokens <= self.max_num_tokens` check a few lines below. The assertion fires inside the event-loop thread, kills it, and leaves the server in a state where `/v1/chat/completions` hangs forever while `/v1/models` keeps returning 200 (see issue NVIDIA#13318 for a full reproduction on both `1.3.0rc12` and main + PR NVIDIA#12976 + NVIDIA#13029). Fix: at the top of `_prepare_tp_inputs`, pre-compute the predicted `position_ids` growth using the same accounting the loops below use, and shrink the trailing context chunk(s) to bring the total back within `max_num_tokens`. `isLastContextChunk()` and `isFirstContextChunk()` are live-derived from `context_current_position`, `context_chunk_size`, and `prompt_len` — mutating `context_chunk_size` automatically de-promotes a "last" chunk to a "middle" chunk for this iteration, and the next scheduler tick picks up the remaining tokens through the normal chunked-prefill progression. KV cache reserved by `prepare_resources` for the original chunk size stays in place and is reclaimed when the request eventually finishes. This is a mitigation, not a root-cause fix: the scheduler's admission math is still wrong (`_reuse_adjusted_compute` subtracts reusable tokens that the forward pass still has to process), and the proper fix should align the admission budget with the actual forward-pass cost. Until that lands, this guard keeps the event loop alive and lets the server degrade gracefully instead of crashing. Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds a pre-forward admission guard in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/pyexecutor/model_engine.py`:
- Around line 2270-2279: The current predicted_gen_tokens sum undercounts cases
where a request will append more position_ids than 1 +
max(get_draft_token_length(req), self.runtime_draft_len) (specifically: plain
generation with beam_width > 1 and py_is_first_draft requests that use
original_max_draft_len). Update the aggregation that computes
predicted_gen_tokens (over scheduled_requests.generation_requests) to use the
actual generation-side accounting per request: for each req compute
tokens_to_account = 1 + max(get_draft_token_length(req), self.runtime_draft_len,
(self.original_max_draft_len if req.py_is_first_draft else 0), (req.beam_width
if getattr(req, "beam_width", 1) > 1 else 0)) and sum those values; reference
get_draft_token_length, self.runtime_draft_len, req.py_is_first_draft,
self.original_max_draft_len, and req.beam_width when making the change so
trimming matches the generation-side logic.
🪄 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: Pro Plus
Run ID: 34c01a53-7e34-42b4-aef1-f32ba0562539
📒 Files selected for processing (1)
tensorrt_llm/_torch/pyexecutor/model_engine.py
| # Each generation request extends `position_ids` by | ||
| # `1 + num_draft_tokens`. The gen loop below uses either | ||
| # `get_draft_token_length(req)` (extend path) or | ||
| # `self.runtime_draft_len` (plain gen / first-draft path), so | ||
| # take the larger of the two to stay conservative — a slight | ||
| # overestimate only causes us to trim a little earlier, never | ||
| # fails to catch a real overshoot. | ||
| predicted_gen_tokens = sum( | ||
| 1 + max(get_draft_token_length(req), self.runtime_draft_len) | ||
| for req in scheduled_requests.generation_requests) |
There was a problem hiding this comment.
Mirror the actual generation-side token accounting here.
The formula on Line 2278 still undercounts batches that later append more position_ids than 1 + max(...): plain generation with beam_width > 1 adds one token per beam at Lines 2641-2694, and py_is_first_draft requests add 1 + self.original_max_draft_len at Lines 2574-2616. In those cases this guard can skip trimming and the assertion at Lines 2746-2748 still remains reachable.
Suggested direction
- predicted_gen_tokens = sum(
- 1 + max(get_draft_token_length(req), self.runtime_draft_len)
- for req in scheduled_requests.generation_requests)
+ predicted_gen_tokens = 0
+ for req in scheduled_requests.generation_requests:
+ if get_draft_token_length(req) > 0 or next_draft_tokens_device is not None:
+ predicted_gen_tokens += 1 + max(
+ get_draft_token_length(req), self.runtime_draft_len)
+ elif req.py_is_first_draft:
+ predicted_gen_tokens += 1 + self.original_max_draft_len
+ else:
+ predicted_gen_tokens += req.sampling_config.beam_width🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tensorrt_llm/_torch/pyexecutor/model_engine.py` around lines 2270 - 2279, The
current predicted_gen_tokens sum undercounts cases where a request will append
more position_ids than 1 + max(get_draft_token_length(req),
self.runtime_draft_len) (specifically: plain generation with beam_width > 1 and
py_is_first_draft requests that use original_max_draft_len). Update the
aggregation that computes predicted_gen_tokens (over
scheduled_requests.generation_requests) to use the actual generation-side
accounting per request: for each req compute tokens_to_account = 1 +
max(get_draft_token_length(req), self.runtime_draft_len,
(self.original_max_draft_len if req.py_is_first_draft else 0), (req.beam_width
if getattr(req, "beam_width", 1) > 1 else 0)) and sum those values; reference
get_draft_token_length, self.runtime_draft_len, req.py_is_first_draft,
self.original_max_draft_len, and req.beam_width when making the change so
trimming matches the generation-side logic.
|
Closing in favor of porting the |
Summary
Fixes #13318. Adds a Python-side post-scheduler guard in
_prepare_tp_inputsthat pre-computes the predictedposition_idsgrowth and trims the trailing context chunk(s) if the batch would overshootmax_num_tokens. Mitigates the event-loop-killingassert total_num_tokens <= self.max_num_tokenscrash observed in production withenable_chunked_prefill: true+enable_block_reuse: true.Root cause (quoting the issue)
MicroBatchScheduler(V1, C++) admits batches using a reuse-discounted token budget. For a last-chunk context request withreusable > 0it charges onlymax(0, context_remaining - reusable)tokens — see_reuse_adjusted_computeintensorrt_llm/_torch/pyexecutor/scheduler/scheduler.py:321and its C++ counterpart._prepare_tp_inputs(this file, around line 2276) extendsposition_idsby the fullcontext_chunk_sizeper context request regardless of reuse: the reusable prefix's position embeddings still need to be computed in the forward pass.max_num_tokensis tight, the admission-vs-forward mismatch letslen(position_ids)overshootmax_num_tokensby up tomin(chunk_size, reusable)per offending request. The assertion atmodel_engine.py:2678fires, the event-loop thread dies, and the server hangs untildocker kill+ restart (issue [Bug]: Scheduler deadlock on main + #12976 + #13029: AssertionError total_num_tokens > max_num_tokens in _prepare_tp_inputs under KV offload + chunked prefill permanently hangs the event loop #13318 has the full reproduction).The admission math in
_reuse_adjusted_compute(and its C++ twin) is still wrong after this fix — it subtracts reusable tokens that the forward pass has to process anyway. A proper root-cause fix should align the admission budget with the actual forward-pass cost. Until that lands, this guard keeps the event loop alive and lets the server degrade gracefully instead of crashing.What the fix does
At the top of
_prepare_tp_inputs(beforeinput_ids = []), after the_apply_incremental_updateearly-return:predicted_ctx_tokens = sum(req.context_chunk_size for req in context_requests)— exact match for what the context loop will extend ontoposition_ids.predicted_gen_tokens = sum(1 + max(get_draft_token_length(req), self.runtime_draft_len) for req in generation_requests)— conservative upper bound covering both extend-request and plain-gen / first-draft paths.predicted_total > self.max_num_tokens, iterate the context requests in reverse and shrinkreq.context_chunk_sizeuntil the overshoot is cleared (leaving each chunk at least 1 token).Why mutating
context_chunk_sizemid-pipeline is safe:isLastContextChunk()(cpp/include/tensorrt_llm/batch_manager/llmRequest.h:1636) is live-derived fromgetContextCurrentPosition() + getContextChunkSize() == mPromptLen, so shrinking the chunk automatically de-promotes a "last" chunk to a "middle" chunk for this iteration. The next scheduler tick picks up the remaining tokens through the normal chunked-prefill progression.context_chunk_sizeis a read/write property in the nanobind binding (cpp/tensorrt_llm/nanobind/batch_manager/bindings.cpp:128), mapped tosetContextChunkSizewhich clamps togetContextRemainingLength()and rejects negatives.prepare_resourcesfor the original chunk size stays in place and is reclaimed when the request finishes.Relationship to PR #12806
PR #12806 fixes the analogous bug on
feat/bench_y, which carries a Python-sideremaining_budgetre-check inprepare_resourcesthat doesn't exist onmain. The fix here applies the same intent ("pre-account committed forward-pass cost; shrink admissions to fit") but at the layer that exists onmain— the pre-loop guard in_prepare_tp_inputs. Once the C++ admission math is aligned with forward-pass cost (or a Python-side pre-scheduler budget lands onmain), this guard becomes a redundant backstop and can be removed.Test plan
max_num_tokens=4096,enable_chunked_prefill=true,enable_partial_reuse=true,enable_block_reuse=true): verify noAssertionError, no event-loop hang,[issue-13318] trimming context chunkwarnings appear in the server log, and traffic continues to be served.max_num_tokens=8192baseline (clean across 10k+ requests per issue's matrix) is unchanged.ScheduledRequestsbatch whose summedcontext_chunk_size + gen_tokensexceeds a mockedmax_num_tokens; assert the trailingcontext_chunk_sizeis reduced to fit and a warning is logged.predicted_total > max_num_tokens, which should not happen under a correct scheduler, so steady-state behavior should be unchanged.Summary by CodeRabbit