[None][fix] Do not synthesize mrope_config for text-only prompts - #16874
Draft
lingjiew wants to merge 2 commits into
Draft
[None][fix] Do not synthesize mrope_config for text-only prompts#16874lingjiew wants to merge 2 commits into
lingjiew wants to merge 2 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe Qwen2VL text-only prompt path now returns tokenized input IDs with no processed multimodal metadata. New tests cover absent, null, and empty multimodal inputs across Qwen VL processors and verify multimodal prompts retain their existing processing. ChangesQwen2VL text-only prompt handling
Estimated code review effort: 2 (Simple) | ~10 minutes 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tensorrt_llm/_torch/models/modeling_qwen2vl.py (1)
869-879: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for the text-only fast path.
Please verify that
call_with_text_prompt()returns(input_ids, None)and does not callget_mrope_config()whenmulti_modal_datais empty. The supplied downstream test only verifies thatNoneis tolerated, not that this memory-retention fix remains in place.🤖 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 `@tensorrt_llm/_torch/models/modeling_qwen2vl.py` around lines 869 - 879, Add a regression test covering call_with_text_prompt() when multi_modal_data is empty: assert it returns the tokenized input_ids with None for multimodal configuration, and mock or spy on get_mrope_config() to assert it is not called. Keep the test focused on preserving the text-only fast path and its memory-retention behavior.
🤖 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.
Nitpick comments:
In `@tensorrt_llm/_torch/models/modeling_qwen2vl.py`:
- Around line 869-879: Add a regression test covering call_with_text_prompt()
when multi_modal_data is empty: assert it returns the tokenized input_ids with
None for multimodal configuration, and mock or spy on get_mrope_config() to
assert it is not called. Keep the test focused on preserving the text-only fast
path and its memory-retention behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1dec3717-ab8b-4ab7-8135-8c52717af6a9
📒 Files selected for processing (1)
tensorrt_llm/_torch/models/modeling_qwen2vl.py
lingjiew
force-pushed
the
user/lingjiew/fix-textonly-mrope-ipc-leak
branch
from
July 27, 2026 04:00
7ad0136 to
d0fdde8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unittest/_torch/multimodal/test_qwen2vl_text_only_prompt.py (1)
20-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the required function annotations.
_FakeTokenizermethods,_make_processor, and both tests are unannotated. Add precise parameter and return annotations, including-> Nonefor the tests. As per coding guidelines, “Annotate every function.”Also applies to: 28-35, 55-55, 82-82
🤖 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/_torch/multimodal/test_qwen2vl_text_only_prompt.py` around lines 20 - 25, Annotate every function in this test module, including the _FakeTokenizer constructor and __call__, _make_processor, and both test functions. Add precise parameter and return types based on their existing usage, and explicitly use -> None for the test methods.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/_torch/multimodal/test_qwen2vl_text_only_prompt.py`:
- Around line 38-114: Add the relevant test-list entry for
test_text_only_prompt_returns_no_extra_processed_inputs and
test_multimodal_prompt_still_takes_the_full_path in the appropriate
tests/integration/test_lists/test-db or QA list, ensuring these Qwen VL
text-only and multimodal coverage tests are included in CI or manual QA
execution.
---
Nitpick comments:
In `@tests/unittest/_torch/multimodal/test_qwen2vl_text_only_prompt.py`:
- Around line 20-25: Annotate every function in this test module, including the
_FakeTokenizer constructor and __call__, _make_processor, and both test
functions. Add precise parameter and return types based on their existing usage,
and explicitly use -> None for the test methods.
🪄 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: fae816ae-0c42-4b2a-9905-ab6a94bf0365
📒 Files selected for processing (1)
tests/unittest/_torch/multimodal/test_qwen2vl_text_only_prompt.py
lingjiew
force-pushed
the
user/lingjiew/fix-textonly-mrope-ipc-leak
branch
from
July 27, 2026 04:04
d0fdde8 to
3b23dcd
Compare
Qwen2VLInputProcessorBase.call_with_text_prompt has a text-only fast path that skips the multimodal HF processor but still calls get_mrope_config(), on the reasoning that the LM is M-RoPE. For a prompt with no images or video that work is redundant: PyTorchModelEngine._prepare_tp_inputs already establishes the text-only answer itself by broadcasting position_ids to [3, 1, N], and the per-axis positions only ever overwrite multimodal spans. get_rope_index() on text-only input returns exactly that broadcast, with a delta of 0. It is also unbounded under disaggregated serving. Populating multimodal_data makes MultimodalParams.has_content() true, which opens the EPD mRoPE re-registration for context-only requests. That path exports both tensors via SharedTensorContainer.from_tensor() -> reduce_tensor() -> _share_cuda_(), after which the block sits in CudaIPCSentDataLimbo until a consumer opens and drops the handle. Over trtllm-serve disaggregation no consumer exists -- the serve-layer DisaggregatedParams has no mRoPE handle fields, see the existing TODO(TRTLLM-12407) -- so every context request permanently strands one (3, 1, prompt_len) int64 tensor. Measured on Qwen3.5-397B-A17B over long-context agentic traces: 24 bytes per prompt token per scheduled context request, 16.6 MiB/s of unrecoverable growth, context worker OOM in 2,300-3,500 s. CUDA allocator snapshots 900 s apart show the two clone sites going from 828 -> 4,247 live blocks each (100% of the process's live-block growth) to 0 -> 0 with this change, device growth over the window falling from +10,037 MiB to +10 MiB, with throughput, prefix-cache hit rate and startup KV-pool sizing unchanged. This does not close the defect for genuine multimodal traffic over trtllm-serve, which still needs the protocol fields tracked by TRTLLM-12407; it removes the text-only case, which fires on every request of a text workload. Signed-off-by: lingjiew <lingjiew@nvidia.com>
Assert that call_with_text_prompt emits no extra processed inputs and never synthesizes an mrope_config when a prompt carries no multimodal data, across the three falsy shapes of multi_modal_data and all three processor classes that inherit the fast path. Regressing this is silent: every functional test still passes, and the only symptom is the CUDA-IPC mRoPE export for context-only requests pinning device memory until the context worker runs out under disaggregated serving. Signed-off-by: lingjiew <lingjiew@nvidia.com>
lingjiew
force-pushed
the
user/lingjiew/fix-textonly-mrope-ipc-leak
branch
from
July 27, 2026 04:10
3b23dcd to
2945612
Compare
lingjiew
marked this pull request as draft
July 27, 2026 05:50
| @@ -866,18 +866,17 @@ def call_with_text_prompt( | |||
|
|
|||
Collaborator
There was a problem hiding this comment.
Talked offline. Need to consider mixed modality requests(e.g. text request following with multimodal requests)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[None][fix] Do not synthesize mrope_config for text-only prompts
Summary
Qwen2VLInputProcessorBase.call_with_text_prompt()has a text-only fast path thatskips the multimodal HF processor but still calls
get_mrope_config(), reasoningthat "the LM is M-RoPE". For a prompt with no images or video that work is
redundant, and it has an unbounded side effect under disaggregated serving: the
context worker permanently strands one
(3, 1, prompt_len)int64 tensor perscheduled context request.
On Qwen3.5-397B-A17B over long-context agentic traces this is 24 bytes per prompt
token per scheduling event — 16.6 MiB/s of unrecoverable device memory, and the
context worker exhausts a GB300 in 2,300–3,500 s depending on KV-pool headroom.
Why it is redundant
PyTorchModelEngine._prepare_tp_inputsalready establishes the text-only answeritself:
The per-axis positions only ever overwrite multimodal spans. With no such spans
there is nothing to overwrite, so the synthesized
mrope_configreproduces avalue the engine has already written.
Verified numerically rather than argued —
get_rope_index()on text-only input,across four prompt lengths:
arange(L)broadcast to[3,1,L]mrope_position_deltas == 0int64(24 B/token)deltas == 0also covers the generation phase: with nomrope_position_deltathe engine falls back to
past_seen_token_num, which is exactlypast_seen_token_num + 0.Why it leaks
Populating
multimodal_datamakesMultimodalParams.has_content()true, whichopens the EPD mRoPE re-registration for context-only requests in
model_engine.py:set_mrope_positionwraps both clones inSharedTensorContainer.from_tensor(),i.e.
reduce_tensor()→storage._share_cuda_(), which replaces the storage'sDataPtrwith aCudaIPCSentDataowning the original. The block does not returnto the caching allocator when the Python tensor dies; it enters
CudaIPCSentDataLimboand is freed only once a consumer opens and drops thehandle.
shared_tensor.pystates the contract directly:Under
trtllm-servedisaggregation no consumer can exist: the serve-layerDisaggregatedParamscarries no mRoPE handle fields — see the existingTODO(TRTLLM-12407)inserve/openai_protocol.py. The handle is dropped at theorchestrator, so every export is a permanent pin.
Evidence
CUDA allocator snapshots (
torch.cuda.memory._record_memory_history) taken 900 sapart on the context worker, before and after this change. Same image, same
workload, same config.
mrope_position_idsclonemrope_position_deltascloneThe two clone sites accounted for 100% of the process's live-block growth
(+6,838 of +6,839) in exact 1:1 pairing, with block sizes spread over 1,474
distinct values from 0.007 to 6.740 MiB — i.e. 24 bytes × prompt_len, mean
2.44 MiB.
Throughput, prefix-cache hit rate and startup KV-pool sizing are unchanged
(8.33 req/s, 95.6%, 168.20 GiB); the benchmark completed with zero errors.
With the fix the affected configuration also reproduces its pre-regression
baseline. A full 3600 s disaggregated run at the same operating point as a known
good build from before this path started firing — same parallelism, batch sizes,
sequence lengths, KV-pool fraction and client concurrency:
Every metric is within run-to-run noise, and the context worker no longer runs
out of memory. Without the fix the same configuration cannot finish the window.
Scope
This does not close the underlying defect for genuine multimodal traffic: a
request that really carries images over
trtllm-servedisaggregation would stillexport mRoPE handles that nothing can rebuild. That needs the protocol fields
tracked by TRTLLM-12407. This change removes the text-only case, which is the one
that fires on 100% of requests for a text workload.
Test coverage
tests/unittest/_torch/multimodal/test_qwen2vl_text_only_prompt.py(new).test_text_only_prompt_returns_no_extra_processed_inputsasserts thatcall_with_text_promptreturnsextra_processed_inputs is Noneand thatget_mrope_configand_preprocessare both never called. It is parametrizedover the three falsy shapes of
multi_modal_data(key absent /None/{})and over the three processor classes that inherit this fast path
(
Qwen2VLInputProcessorBase,Qwen2_5VLInputProcessorBase,Qwen3VLInputProcessorBase), so a subclass reintroducing the export is caughttoo.
test_multimodal_prompt_still_takes_the_full_pathis the complement: withnon-empty
multi_modal_datathe full path still runs and still emitsmrope_config, so the fast path cannot be widened into the multimodal case.It passes both before and after this change and is not the regression guard.
Both are CPU-only and need no model weights or network; the processor is built
with
object.__new__plus a fake tokenizer, the same technique the neighbouringtest_qwen3vl_disagg_prompt.pyalready uses. The file needs no test-list entrybecause
tests/integration/test_lists/test-db/l0_h100.ymlcollectsunittest/_torch/multimodalas a directory.Against a revert of this PR's diff, both guard assertions in the first test fail,
in all nine parametrizations.
Accuracy has not been re-measured on non-disaggregated text-only inference for
other Qwen-VL models sharing this processor; happy to run whatever the reviewers
consider sufficient.
Dev Engineer Review
tensorrt_llm/_torch/models/modeling_qwen2vl.py(Qwen2VLInputProcessorBase.call_with_text_prompt()): for the text-only fast path (multi_modal_dataempty/falsy), the implementation now avoids computing/synthesizingmrope_configand does not return it viaextra["multimodal_data"]. It directly tokenizes the prompt and returns onlyinput_idswith no processed multimodal inputs produced.PyTorchModelEngine._prepare_tp_inputs, which already supplies default 3-axis M-RoPE position IDs for text-only inputs.extra["multimodal_data"]["mrope_config"]is still produced.QA Engineer Review
tests/unittest/_torch/multimodal/test_qwen2vl_text_only_prompt.py:_FakeTokenizer,_make_processor(processor_cls)test_text_only_prompt_returns_no_extra_processed_inputs(processor_cls, inputs)covering text-only handling acrossQwen2VLInputProcessorBase,Qwen2_5VLInputProcessorBase,Qwen3VLInputProcessorBasewith falsymulti_modal_data(missing,None,{}), asserting:extra is Nonetoken_idsmatch expected prompt token IDsget_mrope_configand_preprocessare not calledtest_multimodal_prompt_still_takes_the_full_path()verifying that non-empty multimodal inputs still run the full preprocessing path and produceextra["multimodal_data"]["mrope_config"](including expectedmrope_position_ids/mrope_position_deltastensor shapes).tests/integration/test_listsfortest_qwen2vl_text_only_prompt/qwen2vl_text_only_prompt; no matching entries found (no test-list updates).