[None][perf] Improve Qwen3-VL Preprocessing Perf#15598
Conversation
📝 WalkthroughWalkthroughAdds dedicated ChangesMedia I/O & Async Threading
VLM Processor Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tensorrt_llm/inputs/media_io.py (1)
50-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
_media_load_runto preserve caller result types.Every async media load now goes through this helper, but the untyped signature erases
_MediaTtoAny. UseParamSpec/TypeVarso type checking followsload_bytes/load_base64/load_file.Suggested typing shape
+from collections.abc import Callable +from typing import ParamSpec + +_P = ParamSpec("_P") +_R = TypeVar("_R") + -async def _media_load_run(fn, *args, **kwargs): +async def _media_load_run( + fn: Callable[_P, _R], *args: _P.args, **kwargs: _P.kwargs +) -> _R:As per coding guidelines, “Always annotate functions. Make the return type
Noneif the function does not return anything.”🤖 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/inputs/media_io.py` around lines 50 - 55, The helper `_media_load_run` is currently untyped, which causes the caller-specific media result type to degrade to Any across `load_bytes`, `load_base64`, and `load_file`. Update `_media_load_run` to use `ParamSpec` for the callable arguments and a `TypeVar` for the return value so the async wrappers preserve their concrete `_MediaT` result types. Keep the typing consistent with the existing media-loading helpers in `media_io.py` and ensure the annotation reflects that the function returns the awaited result from `run_in_executor`.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 `@tensorrt_llm/_torch/models/modeling_qwen2vl.py`:
- Around line 100-103: The cache-key fallback in the key-building logic is
catching too broadly in the function that computes
`repr(sorted(kwargs.items()))`, which can mask unrelated bugs and violates the
repo’s exception-handling rule. Narrow the `except Exception` in that cache-key
construction path to only the specific failure(s) expected from sorting or repr
generation, and keep the fallback to `orig(*args, **kwargs)` only for those
concrete cases so other processor regressions still fail visibly.
In `@tensorrt_llm/_torch/models/modeling_qwen3vl.py`:
- Around line 262-265: The Qwen3VL multimodal preprocessing in the `proc_kwargs`
merge still allows `mm_processor_kwargs` to override `do_sample_frames`, which
can re-enable HF resampling. Update the `modeling_qwen3vl` merge logic so
`do_sample_frames` remains fixed to False and is never copied from
`mm_processor_kwargs`, while still preserving the other allowed keys like
`num_frames` and `fps`.
In `@tensorrt_llm/inputs/media_io.py`:
- Around line 450-459: The frame return contract for the default format path is
inconsistent: `VideoData.frames` and `get_num_tokens_per_video` still assume PIL
images or torch tensors, but `media_io.py` now returns HWC uint8 ndarrays from
the `format="pt"` flow. Update the shared `VideoData` type/doc and any consumers
like `get_num_tokens_per_video` to recognize numpy frames (using shape-based
dimensions instead of `.height`/`.width`), or change the
`load_video`/frame-return path so `format="pt"` continues to yield tensors and
add a separate explicit numpy format.
- Around line 389-397: The fallback video-loading path that uses
tempfile.NamedTemporaryFile(delete=False) leaves behind temporary .mp4 files;
update the media I/O flow so the temp path is unlinked after all processing is
finished, not immediately after opening the VideoCapture. Make the cleanup
happen after any optional audio extraction that still depends on the video
filename, and apply the same fix in both fallback sites in media_io.py so the
byte-loaded video tempfiles are always removed.
- Around line 383-384: Narrow the fallback in the backend-probing block of
media_io.py: the broad Exception handler around the OpenCV API/registry probe is
masking unrelated bugs. Update the exception handling in that probe to only
catch the expected availability failures, specifically AttributeError and
cv2.error when backend queries fail, while keeping the tempfile fallback logic
unchanged.
In `@tensorrt_llm/serve/openai_server.py`:
- Around line 115-118: Validate TRTLLM_INPUT_PROC_WORKERS before constructing
_INPUT_PROC_EXECUTOR in openai_server.py so startup fails with a clear
operator-friendly message instead of crashing on zero, negative, or non-integer
values. Update the _INPUT_PROC_WORKERS initialization to reject invalid values
and ensure ThreadPoolExecutor only receives a positive worker count, keeping the
fix near the existing module-level executor setup.
---
Nitpick comments:
In `@tensorrt_llm/inputs/media_io.py`:
- Around line 50-55: The helper `_media_load_run` is currently untyped, which
causes the caller-specific media result type to degrade to Any across
`load_bytes`, `load_base64`, and `load_file`. Update `_media_load_run` to use
`ParamSpec` for the callable arguments and a `TypeVar` for the return value so
the async wrappers preserve their concrete `_MediaT` result types. Keep the
typing consistent with the existing media-loading helpers in `media_io.py` and
ensure the annotation reflects that the function returns the awaited result from
`run_in_executor`.
🪄 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: 446e6a41-d8ed-4df8-9012-a296acd412b6
📒 Files selected for processing (5)
tensorrt_llm/_torch/models/modeling_qwen2vl.pytensorrt_llm/_torch/models/modeling_qwen3vl.pytensorrt_llm/commands/serve.pytensorrt_llm/inputs/media_io.pytensorrt_llm/serve/openai_server.py
aswinvisva
force-pushed
the
avisva/cosmos3-vlm-preproc-perf
branch
from
08b42ad to
2b7db68
Compare
|
/bot run |
|
PR_Github #55605 [ run ] triggered by Bot. Commit: |
| PAD_INDEX = -100 # NOTE: refer to https://github.com/huggingface/transformers/blob/main/src/transformers/models/qwen2_5_vl/modular_qwen2_5_vl.py#L269 | ||
|
|
||
|
|
||
| def _install_merge_kwargs_cache(processor) -> None: |
There was a problem hiding this comment.
After re-evaluating the perf benchmarks with everything included, this may be removable - it doesn't seem to make a huge difference when removing it. Will test in Jet and report back
|
/bot run |
|
PR_Github #55618 [ run ] triggered by Bot. Commit: |
|
PR_Github #55605 [ run ] completed with state |
|
PR_Github #55618 [ run ] completed with state
|
| # already sampled to the target frame count, so letting a caller flip | ||
| # it back to True would re-enter the HF sample_frames branch on | ||
| # already-sampled input and break the contract this patch establishes. | ||
| proc_kwargs = {"do_sample_frames": False} |
There was a problem hiding this comment.
Hm...
- are we sure the sampling is the same in media IO vs in the processor?
- is it possible to gate this on whether the
num_frames/fpsare different than what we already sampled?
There was a problem hiding this comment.
Updated to the logic we discussed in person - let me know what you think
| if _tmp_path is not None: | ||
| try: | ||
| os.unlink(_tmp_path) | ||
| except OSError: |
There was a problem hiding this comment.
Hm, I don't know how I feel about this. Could we move some of the logic in lines 426-439 to the caller VideoMediaIO.load_bytes? We could use a contextlib.ExitStack to conditionally push a tempfile.NamedTemporaryFile(..., delete=True) into it.
If not, I would still vote to use it here to rely on NamedTemporaryFile to do the cleanup, it's just a bit less readable in that we add one more indent level for the context manager.
There was a problem hiding this comment.
Updated to move this upstream and use context managers to handle the cleanup rather than doing it here
| ) | ||
|
|
||
|
|
||
| async def _media_load_run(fn, *args, **kwargs): |
There was a problem hiding this comment.
Nit: I think _load_media might be better?
There was a problem hiding this comment.
Updated to run_in_executor since it's mostly just wrapping a blocking function as an async by offloading to the threadpoolexecutor - let me know what you think
|
PR_Github #57213 [ run ] triggered by Bot. Commit: |
|
PR_Github #57212 [ run ] completed with state |
aswinvisva
added
the
api-compatible
|
PR_Github #57213 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57230 [ run ] triggered by Bot. Commit: |
|
PR_Github #57230 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57253 [ run ] triggered by Bot. Commit: |
|
PR_Github #57253 [ run ] completed with state
|
|
/bot run |
3 similar comments
|
/bot run |
|
/bot run |
|
/bot run |
|
PR_Github #57266 [ run ] triggered by Bot. Commit: |
|
PR_Github #57266 [ run ] completed with state
|
|
/bot run |
5 similar comments
|
/bot run |
|
/bot run |
|
/bot run |
|
/bot run |
|
/bot run |
|
PR_Github #57274 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #57274 [ run ] completed with state
|
|
/bot run |
|
PR_Github #57387 [ run ] triggered by Bot. Commit: |
|
PR_Github #57388 [ run ] triggered by Bot. Commit: |
|
PR_Github #57387 [ run ] completed with state |
|
PR_Github #57388 [ run ] completed with state
|
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Description
A few improvements to the preprocessing speed of Qwen3-VL (and other models should benefit too)
[uvloop](https://uvloop.readthedocs.io/)asyncio event loop backend - already default in vllm and is 2-4 faster which benefits us when event loop is under a huge queue of pending events - this is just a simple drop-in replacement that makes the largest diff_load_video_by_cv2via python list comprehension (callstorch.from_numpyfor every request) - instead return the uint8 numpy frames and let the HF processor's do_rescale=True path do the rescale + permute as a single vectorized op on the stacked (N, H, W, 3) arraydo_sample_frames=Falsevia mm_processing_kwargs so we don't duplicate the work (Media IO already does this work upstream)merge_kwargsin preprocessing_qwen3vl. This function is a pure function (i.e same args produce same output), and is called for every input processor invocation - and was taking 76ms under load. Simply memoize the output for the args passedOverall perf improvements:
benchmark: 30s 374×374
libx264mp4 video (moving_shapessynth) + 50 text tokens → ISL ≈ 2.3k–13.1k depending on fps, OSL ∈ {1, 100}, fps ∈ {1, 2, 4, 8}, concurrency ∈ {1, 8, 16, 32, 64, 128, 256}Cosmos3-Super-Reasoner
Cosmos3-Nano-Reasoner
Test Coverage
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.