Skip to content

[Bugfix][MRV2] Support encoder timing stats in model runner V2 - #50020

Merged
Isotr0py merged 6 commits into
vllm-project:mainfrom
guan404ming:fix/mrv2-encoder-timing-stats
Aug 11, 2026
Merged

Isotr0py merged 6 commits into
vllm-project:mainfrom
guan404ming:fix/mrv2-encoder-timing-stats

Conversation

@guan404ming

Copy link
Copy Markdown
Contributor

Purpose

  • vllm bench mm-processor crashes on model runner V2: get_encoder_timing_stats only exists in V1
  • Port it to V2: timing lives on EncoderRunner, gated on enable_mm_processor_stats; EncoderTimingStats moves to worker/utils.py for sharing
  • Not a duplicate: no open PR touches MRV2 encoder timing

Test Plan

  • pytest tests/v1/worker/test_encoder_runner.py — adds registry accumulate/clear test
  • vllm bench mm-processor --model Qwen/Qwen2.5-VL-3B-Instruct --dataset-name random-mm --num-prompts 10 on Modal L4 (MRV2 default)

Test Result

  • 8 passed
  • Before: AttributeError: 'GPUModelRunner' object has no attribute 'get_encoder_timing_stats'
  • After: benchmark completes with encoder stats populated
Stage Mean Median P99
encoder_forward_ms 138.99 183.15 201.11
num_encoder_calls 1.00 1.00 1.00

AI assistance was used for this change (Claude); every line was reviewed by the submitter.

@mergify mergify Bot added v1 bug Something isn't working labels Jul 27, 2026
@guan404ming
guan404ming marked this pull request as ready for review July 27, 2026 17:21

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@guan404ming

Copy link
Copy Markdown
Contributor Author

cc @Isotr0py

@Isotr0py Isotr0py self-assigned this Jul 28, 2026
@mergify mergify Bot added the mrv2 Model Runner V2 specific label Jul 31, 2026
@guan404ming
guan404ming force-pushed the fix/mrv2-encoder-timing-stats branch from 312a368 to f452563 Compare August 4, 2026 16:24
@mergify

mergify Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @guan404ming.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Aug 5, 2026
self, mm_kwargs: list[tuple[str, MultiModalKwargsItem]]
self,
mm_kwargs: list[tuple[str, MultiModalKwargsItem]],
request_ids: Collection[str] | None = None,
) -> list[torch.Tensor]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to keep execute_mm_encoder only accept mm_kwargs to make sure the function is clean enough. Perhaps you can implement a context manager for timing instead of patching the function:

@contextmanager
def timed_encoder_operation(
self,
should_time: bool,
group_lora_refs: list[tuple[str, Any]],
current_item_idx: int,
num_items: int,
):
"""
Context manager to time encoder forward operations.
Args:
should_time: Whether timing is enabled
group_lora_refs: Full list of (request_id, pos_info) tuples
current_item_idx: Starting index for this group
num_items: Number of items in this group
"""
if not should_time:
yield
return
group_refs = group_lora_refs[current_item_idx : current_item_idx + num_items]
group_request_ids = {req_id for req_id, _ in group_refs}
torch.accelerator.synchronize()
start_time = time.perf_counter()
try:
yield
finally:
torch.accelerator.synchronize()
elapsed = time.perf_counter() - start_time
per_request_time = elapsed / max(len(group_request_ids), 1)
with self._encoder_timing_lock:
for req_id in group_request_ids:
if req_id not in self.encoder_timing_registry:
self.encoder_timing_registry[req_id] = EncoderTimingStats()
stats = self.encoder_timing_registry[req_id]
stats.encoder_forward_secs += per_request_time
stats.num_encoder_calls += 1

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Reverted execute_mm_encoder to only accept mm_kwargs, and moved the timing into a timed_encoder_operation context manager on EncoderRunner, mirroring the V1 pattern you linked. Callers now wrap the encoder call with it. PTAL, thanks!

@guan404ming
guan404ming force-pushed the fix/mrv2-encoder-timing-stats branch from f452563 to 4cd9ef7 Compare August 5, 2026 09:19
@mergify mergify Bot removed the needs-rebase label Aug 5, 2026
@Isotr0py
Isotr0py enabled auto-merge (squash) August 5, 2026 13:15
@github-actions github-actions Bot added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 5, 2026
@guan404ming

Copy link
Copy Markdown
Contributor Author

Hi @Isotr0py could you help trigger the buildkite ci? It seems to be inactive for a while. Thanks!

@Isotr0py

Isotr0py commented Aug 6, 2026

Copy link
Copy Markdown
Member

/ci run

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #82663 for commit 186691282a0d.

@guan404ming

Copy link
Copy Markdown
Contributor Author

Thanks!

@guan404ming

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #82774 for commit dbf9f4712526.

@mergify

mergify Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @guan404ming.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Aug 7, 2026
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
auto-merge was automatically disabled August 7, 2026 04:00

Head branch was pushed to by a user without write access

@guan404ming
guan404ming force-pushed the fix/mrv2-encoder-timing-stats branch from dbf9f47 to e9b51dc Compare August 7, 2026 04:00
@mergify mergify Bot removed the needs-rebase label Aug 7, 2026
@guan404ming

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #82781 for commit e9b51dcbf4f5.

@guan404ming

Copy link
Copy Markdown
Contributor Author

Hi @Isotr0py I just pushed a commit to resolve the conflict. Could you help enable merge again? Thanks!

@guan404ming

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

✅ Triggered Buildkite CI #82983 for commit 5be9679d7d98.

@guan404ming

Copy link
Copy Markdown
Contributor Author

/ci run

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #83261 for commit 9ef0ae4f6d71.

@guan404ming

Copy link
Copy Markdown
Contributor Author

/ci run

@guan404ming

Copy link
Copy Markdown
Contributor Author

Hi @Isotr0py just a gentle ping. I pushed a commit to resolve the conflict. Could you help enable merge again? Thanks!

@github-actions

Copy link
Copy Markdown

✅ Triggered Buildkite CI #83364 for commit ec4fa70d5c9b.

@Isotr0py
Isotr0py merged commit 457a5f3 into vllm-project:main Aug 11, 2026
94 checks passed
@guan404ming
guan404ming deleted the fix/mrv2-encoder-timing-stats branch August 11, 2026 16:20
@guan404ming

Copy link
Copy Markdown
Contributor Author

Thanks!

zyp2014 pushed a commit to zyp2014/vllm that referenced this pull request Aug 21, 2026
…project#50020)

Signed-off-by: Guan-Ming (Wesley) Chiu <105915352+guan404ming@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working mrv2 Model Runner V2 specific ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants