Skip to content

Add vLLM offline backend with micro-batching support - #736

Open
maryamtahhan wants to merge 13 commits into
vllm-project:mainfrom
maryamtahhan:feat/vllm-offline-batching-backend
Open

Add vLLM offline backend with micro-batching support#736
maryamtahhan wants to merge 13 commits into
vllm-project:mainfrom
maryamtahhan:feat/vllm-offline-batching-backend

Conversation

@maryamtahhan

@maryamtahhan maryamtahhan commented May 20, 2026

Copy link
Copy Markdown
Contributor

Add vLLM Offline Backend for Batch Processing

Adds VLLMOfflineBackend for batch-oriented inference using vLLM's synchronous LLM engine. Requests are queued and dispatched in configurable micro-batches via LLM.generate(), removing per-request scheduling overhead. Ideal for throughput benchmarking.

Summary

  • New VLLMOfflineBackend extending VLLMPythonBackend with batch processing
  • PID-based engine preload in validate(): workers eagerly create the engine before the timed phase; the parent process never loads model weights
  • Lock/state reset in process_startup() so asyncio primitives work after fork
  • CPU affinity reset before engine creation (works around OpenMP restriction in forked processes)
  • Metrics wiring from vLLM RequestStateStats for TTFT/ITL/TPOT reporting
  • Shared common.py module for utilities used by both vLLM backends
  • 39 unit tests covering all critical paths

Architecture

VLLMOfflineBackend inherits from VLLMPythonBackend, reusing chat template resolution, multimodal data handling, request formatting, and sampling parameter creation. Only the engine lifecycle and request processing are overridden.

Key Design Decisions

  • PID-based engine preload: _creator_pid is recorded in __init__; validate() detects forked/spawned workers (PID mismatch) and calls _ensure_engine() to preload the engine before the timed benchmark phase. The parent preflight skips engine creation entirely. resolve() still calls _ensure_engine() as an idempotent fallback.
  • Lock/state reset after fork: process_startup() recreates all asyncio locks, clears _llm, and resets batch state — locks from the parent's event loop are unusable in the child.
  • Double-checked engine init: _ensure_engine() uses an asyncio.Lock so concurrent callers create only one engine.
  • Split batch lock: _take_pending_batch() snapshots the queue under lock, _run_generate() runs lock-free so new requests can enqueue during generation.
  • Generate lock: _generate_lock serializes LLM.generate() calls (vLLM's sync LLM is not safe for overlapping generates).
  • Shutdown atomicity: _shutting_down check and enqueue happen under the same _batch_lock acquisition. Shutdown waits for in-flight generates and acquires _generate_lock before teardown.
  • Batch timeout: batch_timeout (default 0.01s) controls how long partial batches wait before flushing; full batches bypass the delay.
  • Deferred flush loop: _deferred_flush loops until _pending_batch is empty, preventing requests stuck when new ones arrive during generate.
  • CPU affinity reset: Reads cgroup v2/v1 cpuset to restore full CPU set before engine creation.

Files Changed

File Description
src/guidellm/backends/vllm_python/offline.py New offline backend implementation
src/guidellm/backends/vllm_python/common.py Shared reset_cpu_affinity() utility
src/guidellm/backends/vllm_python/__init__.py Export VLLMOfflineBackend
docs/guides/vllm-offline-backend.md User guide with examples and engine lifecycle docs
docs/guides/backends.md Updated with offline backend section
tests/unit/backends/vllm_python/test_vllm_offline.py 39 unit tests

Usage

guidellm run \
  --backend kind=vllm_offline,model=Qwen/Qwen3-0.6B,batch_size=32 \
  --data kind=synthetic_text,prompt_tokens=256,output_tokens=128 \
  --profile kind=throughput,max_concurrency=20 \
  --constraint kind=max_requests,count=100

Test Plan

Unit Tests (39 passing)

  • Engine laziness: startup defers creation, _ensure_engine creates once, concurrent callers get single engine
  • Lifecycle: shutdown resets state, calls llm.shutdown(), drains pending batch, awaits deferred flush task, acquires _generate_lock before teardown
  • Lock/state reset: locks are fresh objects after shutdown+startup cycle (verified with is not and acquire/release), _llm cleared, batch state reset
  • PID-based preload: validate() skips engine in parent, preloads in worker (simulated PID mismatch), _creator_pid set in __init__
  • Batch processing: _take_pending_batch clears queue, _run_generate distributes results, error signals all waiters, multimodal prompt format
  • Deferred flush: processes partial batches, idempotent scheduling
  • Generate lock: serializes concurrent _run_generate calls
  • Shutdown guard: resolve() rejects during shutdown, flag set under lock
  • Batch timeout: default value, custom value accepted, zero rejected
  • Metrics wiring: token count from num_generation_tokens with fallback, timing from RequestStateStats, queued_ts fallback

EC2 Integration Test (verified)

Benchmarked on EC2 with facebook/opt-125m (CPU):

  • 20/20 requests completed (was 0 before PID preload fix)
  • 2.4 req/s throughput, 77.5 gen tok/s
  • TTFT, ITL, TPOT metrics populated from RequestStateStats
  • Clean shutdown with batch drain

Vanilla vLLM Comparison (verified)

Ran 10 identical prompts through vanilla vllm.LLM.generate() and GuideLLM's offline backend:

  • 10/10 perfect match — identical token counts and generated text
  • Zero output divergence from the underlying vLLM engine

  • "I certify that all code in this PR is my own, except as noted below."

Use of AI

  • Includes code generated or substantially modified by an AI agent
  • Includes tests generated or substantially modified by an AI agent

All commits include appropriate Co-Authored-By trailers as described in DEVELOPING.md.


git log

commit 1a923cf
Author: Maryam Tahhan mtahhan@redhat.com
Date: Fri Jul 17 14:30:31 2026 +0100

Add vLLM Offline Backend for batch processing

Introduces VLLMOfflineBackend inheriting from VLLMPythonBackend,
using vLLM's synchronous LLM engine for micro-batched inference.
Wires RequestStateStats metrics into response timings. Includes
registration test, docs, and backends.md update.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit ba0eb27
Author: Maryam Tahhan mtahhan@redhat.com
Date: Fri Jul 17 15:33:26 2026 +0100

Fix tokenizer access path for vLLM V1 LLM engine

Use llm.get_tokenizer() instead of llm.llm_engine.tokenizer.tokenizer
which doesn't exist in the V1 engine.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 6d6fe7a
Author: Maryam Tahhan mtahhan@redhat.com
Date: Fri Jul 17 15:41:25 2026 +0100

Fix formatting, metrics, and mdformat issues

- Apply ruff format to offline.py
- Fix _wire_vllm_metrics: only use token counts, not monotonic
  timestamps that produce garbage when mixed with wall-clock times
- Apply mdformat to markdown docs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 7f31ab4
Author: Maryam Tahhan mtahhan@redhat.com
Date: Mon Jul 20 17:27:48 2026 +0100

Defer vLLM engine creation to first request and reset CPU affinity

The vLLM LLM engine is now created lazily on the first resolve() call
instead of during process_startup(). This avoids a double-startup that
wastes resources reloading model weights and can cause CPU-affinity
degradation when the engine is created in both the validation path and
the worker process.

Uses double-checked locking with asyncio.Lock to prevent concurrent
engine creation when multiple requests arrive simultaneously.

Adds _reset_cpu_affinity() to restore the full cgroup cpuset before
engine creation, working around OpenMP/torch restricting CPU affinity
in forked worker processes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit b763a9a
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 12:57:05 2026 +0100

Wire vLLM RequestStateStats metrics and fix batch lock contention

Map vLLM's monotonic RequestStateStats timestamps to wall-clock
values so TTFT, ITL, and TPOT are reported for offline batches.
Split _process_batch into _take_pending_batch + _run_generate to
release the batch lock during LLM.generate(), and loop the deferred
flush until all pending requests are drained.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 638a724
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 14:31:04 2026 +0100

Address review: move CPU affinity to common module, use _shutting_down, call shutdown

- Move _reset_cpu_affinity to common.py so both vllm_python backends
  can reuse it; add comment explaining cgroup v2/v1 fallback loop
- Guard resolve() with _shutting_down check to reject requests during
  shutdown
- Call llm.shutdown() before deleting the engine in process_shutdown

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 7a552bb
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 15:55:21 2026 +0100

Fix concurrency hazards and clean up batch request dataclass

- Add _generate_lock to serialize LLM.generate() calls, preventing
  overlapping generates on the non-thread-safe sync LLM engine
- Move _shutting_down check + enqueue under the same batch lock
  acquisition so shutdown cannot strand in-flight resolve() waiters
- Set _shutting_down under lock in process_shutdown for atomicity
- Remove unused _BatchedRequest fields (request, request_info,
  request_id) and the uuid import
- Drop duplicate validate_vllm_config; inherited from parent

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 459f961
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 16:11:06 2026 +0100

Add unit tests for vLLM offline backend

Covers engine laziness, lifecycle/shutdown, batch processing,
deferred flush, generate-lock serialization, shutdown-guard,
and metrics wiring with 31 test cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 7d684b2
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 16:26:23 2026 +0100

Fix shutdown race, add batch_timeout, and finish test coverage

Shutdown now waits for the deferred-flush task to complete and
acquires _generate_lock before tearing down the engine, preventing
a race where an in-flight LLM.generate() outlives _llm teardown.

Add batch_timeout field (default 0.01s) to VLLMOfflineBackendArgs
so partial batches accumulate for a configurable window before
flushing; full batches still flush immediately.

Add ## WRITTEN BY AI ## docstrings to all 34 test functions, replace
the old cancellation test with test_shutdown_waits_for_inflight_generate,
and add batch_timeout validation tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 69eceea
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 16:41:47 2026 +0100

Replace guide example with throughput profile and remove getattr from metrics

Use throughput,max_concurrency=20 instead of constant,rate=3 in the
offline backend guide example to better reflect the backend's
throughput-oriented purpose.

Replace all getattr calls in _wire_vllm_metrics with hasattr + direct
attribute access per AGENTS.md coding standards.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit dcecf87
Author: Maryam Tahhan mtahhan@redhat.com
Date: Tue Jul 21 17:33:45 2026 +0100

Reset asyncio locks in process_startup to fix forked worker hang

The benchmark framework calls process_startup/validate/shutdown in
the parent process, then forks worker subprocesses that call
process_startup again. Asyncio locks created on the parent event
loop are unusable in the child, causing the worker to hang and
produce 0 completed requests.

Recreate all asyncio primitives (_batch_lock, _generate_lock,
_engine_lock) and reset batch state in process_startup() so each
worker gets fresh locks bound to its own event loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 9f97e6b
Author: Maryam Tahhan mtahhan@redhat.com
Date: Wed Jul 22 10:31:21 2026 +0100

Preload engine in forked workers to fix 0-completion benchmark

The vLLM engine cold-start (42s on CPU) was happening during the
timed benchmark phase inside resolve(), causing SIGTERM to kill the
EngineCore before any requests could complete.

Record the creator PID in __init__ so validate() can detect forked
workers and eagerly preload the engine before the timed phase.
Clear _llm in process_startup so workers never inherit a stale
engine handle from the parent process.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

commit 681c155
Author: Maryam Tahhan mtahhan@redhat.com
Date: Wed Jul 22 10:41:26 2026 +0100

Polish engine lifecycle docs and strengthen lock-reset test

Rewrite the Engine lifecycle section to clearly describe the
three-stage flow: parent preflight (cheap check), worker preload
(PID-based eager engine creation), and resolve() idempotent
fallback.  Mention both fork and spawn workers.

Strengthen test_startup_recreates_locks: use `is not` per lock,
assert batch state is reset, and acquire/release each new lock
to prove they work on the current event loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: Maryam Tahhan mtahhan@redhat.com

@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch 4 times, most recently from fc01371 to bbe2874 Compare May 25, 2026 09:14
@maryamtahhan
maryamtahhan marked this pull request as ready for review May 25, 2026 10:25
@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch from efa1d9e to 942fa2e Compare May 25, 2026 13:43
@sjmonson
sjmonson self-requested a review May 27, 2026 15:25
@sjmonson sjmonson added the internal filed by core contributor or associate label May 27, 2026
@sjmonson sjmonson added this to the v0.8.0 milestone May 27, 2026
@sjmonson
sjmonson requested a review from jaredoconnell June 1, 2026 15:01
@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch from 942fa2e to 5d2304d Compare June 25, 2026 11:32
@mergify

mergify Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Hi @maryamtahhan, the DCO check has failed. Please click on DCO in the Checks section for instructions on how to resolve this.

@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch from 664810c to 251eb67 Compare June 25, 2026 11:37
@maryamtahhan

Copy link
Copy Markdown
Contributor Author

@sjmonson @jaredoconnell this PR has been rebased and is green again

Comment thread src/guidellm/backends/vllm_python/common.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated

@dbutenhof dbutenhof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think the code is mostly OK (except it should be updated to match the current backend init conventions). The documentation needs to be updated to 0.7 CLI conventions.

Comment thread src/guidellm/backends/vllm_python/offline.py
Comment thread docs/guides/vllm-offline-backend.md Outdated
Comment thread docs/guides/vllm-offline-backend.md Outdated
Comment thread docs/guides/vllm-offline-backend.md Outdated
Comment thread docs/guides/vllm-offline-backend.md Outdated
Comment thread docs/guides/vllm-offline-backend.md Outdated
Comment thread docs/guides/vllm-offline-backend.md Outdated
@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch 4 times, most recently from c581c3c to 5ee9091 Compare July 21, 2026 11:59

@dbutenhof dbutenhof left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some comments ... the ones around shutdown are actually from a Cursor AI review of your branch -- there's more, but I didn't copy everything.

Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
Comment thread src/guidellm/backends/vllm_python/offline.py Outdated
@maryamtahhan

Copy link
Copy Markdown
Contributor Author

Some comments ... the ones around shutdown are actually from a Cursor AI review of your branch -- there's more, but I didn't copy everything.

Thank you @dbutenhof I will have a look

@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch from e03b402 to 5c21919 Compare July 21, 2026 15:42
maryamtahhan and others added 6 commits July 21, 2026 17:13
Introduces VLLMOfflineBackend inheriting from VLLMPythonBackend,
using vLLM's synchronous LLM engine for micro-batched inference.
Wires RequestStateStats metrics into response timings. Includes
registration test, docs, and backends.md update.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Use llm.get_tokenizer() instead of llm.llm_engine.tokenizer.tokenizer
which doesn't exist in the V1 engine.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
- Apply ruff format to offline.py
- Fix _wire_vllm_metrics: only use token counts, not monotonic
  timestamps that produce garbage when mixed with wall-clock times
- Apply mdformat to markdown docs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
The vLLM LLM engine is now created lazily on the first resolve() call
instead of during process_startup(). This avoids a double-startup that
wastes resources reloading model weights and can cause CPU-affinity
degradation when the engine is created in both the validation path and
the worker process.

Uses double-checked locking with asyncio.Lock to prevent concurrent
engine creation when multiple requests arrive simultaneously.

Adds _reset_cpu_affinity() to restore the full cgroup cpuset before
engine creation, working around OpenMP/torch restricting CPU affinity
in forked worker processes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Map vLLM's monotonic RequestStateStats timestamps to wall-clock
values so TTFT, ITL, and TPOT are reported for offline batches.
Split _process_batch into _take_pending_batch + _run_generate to
release the batch lock during LLM.generate(), and loop the deferred
flush until all pending requests are drained.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
…n, call shutdown

- Move _reset_cpu_affinity to common.py so both vllm_python backends
  can reuse it; add comment explaining cgroup v2/v1 fallback loop
- Guard resolve() with _shutting_down check to reject requests during
  shutdown
- Call llm.shutdown() before deleting the engine in process_shutdown

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
maryamtahhan and others added 4 commits July 21, 2026 17:13
- Add _generate_lock to serialize LLM.generate() calls, preventing
  overlapping generates on the non-thread-safe sync LLM engine
- Move _shutting_down check + enqueue under the same batch lock
  acquisition so shutdown cannot strand in-flight resolve() waiters
- Set _shutting_down under lock in process_shutdown for atomicity
- Remove unused _BatchedRequest fields (request, request_info,
  request_id) and the uuid import
- Drop duplicate validate_vllm_config; inherited from parent

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Covers engine laziness, lifecycle/shutdown, batch processing,
deferred flush, generate-lock serialization, shutdown-guard,
and metrics wiring with 31 test cases.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Shutdown now waits for the deferred-flush task to complete and
acquires _generate_lock before tearing down the engine, preventing
a race where an in-flight LLM.generate() outlives _llm teardown.

Add batch_timeout field (default 0.01s) to VLLMOfflineBackendArgs
so partial batches accumulate for a configurable window before
flushing; full batches still flush immediately.

Add ## WRITTEN BY AI ## docstrings to all 34 test functions, replace
the old cancellation test with test_shutdown_waits_for_inflight_generate,
and add batch_timeout validation tests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
… metrics

Use throughput,max_concurrency=20 instead of constant,rate=3 in the
offline backend guide example to better reflect the backend's
throughput-oriented purpose.

Replace all getattr calls in _wire_vllm_metrics with hasattr + direct
attribute access per AGENTS.md coding standards.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan
maryamtahhan force-pushed the feat/vllm-offline-batching-backend branch from 5c21919 to 69eceea Compare July 21, 2026 16:14
@maryamtahhan

Copy link
Copy Markdown
Contributor Author

not ready for re-review... will add a comment when it is. Still doing some testing

maryamtahhan and others added 3 commits July 21, 2026 17:33
The benchmark framework calls process_startup/validate/shutdown in
the parent process, then forks worker subprocesses that call
process_startup again. Asyncio locks created on the parent event
loop are unusable in the child, causing the worker to hang and
produce 0 completed requests.

Recreate all asyncio primitives (_batch_lock, _generate_lock,
_engine_lock) and reset batch state in process_startup() so each
worker gets fresh locks bound to its own event loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
The vLLM engine cold-start (42s on CPU) was happening during the
timed benchmark phase inside resolve(), causing SIGTERM to kill the
EngineCore before any requests could complete.

Record the creator PID in __init__ so validate() can detect forked
workers and eagerly preload the engine before the timed phase.
Clear _llm in process_startup so workers never inherit a stale
engine handle from the parent process.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
Rewrite the Engine lifecycle section to clearly describe the
three-stage flow: parent preflight (cheap check), worker preload
(PID-based eager engine creation), and resolve() idempotent
fallback.  Mention both fork and spawn workers.

Strengthen test_startup_recreates_locks: use `is not` per lock,
assert batch state is reset, and acquire/release each new lock
to prove they work on the current event loop.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Maryam Tahhan <mtahhan@redhat.com>
@maryamtahhan

Copy link
Copy Markdown
Contributor Author

Ready for Re-review

Comment on lines +74 to +79
# Hide the inherited ``stream`` field -- offline is never streaming.
stream: Literal[False] = Field( # type: ignore[assignment]
default=False,
exclude=True,
description="Offline backend does not support streaming.",
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would be cleaner to build a common base class for the in-process vLLM backends (possibly in common.py) and then add stream only to the asynchronous subclass rather than trying to hide it here. I guess I won't quite go to the extent of demanding a change here, since it is more work & touch surface and this will probably suffice ... but it would be cleaner. 😁

@jaredoconnell jaredoconnell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks pretty good. I have one comment.

Comment on lines +570 to +607
def _wire_vllm_metrics(
request_info: RequestInfo,
request_output: Any,
) -> None:
"""Populate iteration counts and timing from vLLM metrics.

Extracts token counts and, when available, maps vLLM's
monotonic-clock ``RequestStateStats`` timestamps to
wall-clock values anchored on the wall-clock
``arrival_time`` that vLLM also provides.
"""
metrics = request_output.metrics if hasattr(request_output, "metrics") else None
num_gen = (
metrics.num_generation_tokens
if metrics and hasattr(metrics, "num_generation_tokens")
else 0
)

if num_gen > 0:
request_info.timings.token_iterations = num_gen
elif request_output.outputs and request_output.outputs[0].token_ids is not None:
num_gen = len(request_output.outputs[0].token_ids)
request_info.timings.token_iterations = num_gen

if num_gen > 0:
request_info.timings.request_iterations = 1

if metrics is None:
return

arrival = metrics.arrival_time if hasattr(metrics, "arrival_time") else 0.0
scheduled = metrics.scheduled_ts if hasattr(metrics, "scheduled_ts") else 0.0
queued = metrics.queued_ts if hasattr(metrics, "queued_ts") else 0.0
mono_base = scheduled or queued
first_tok = (
metrics.first_token_ts if hasattr(metrics, "first_token_ts") else 0.0
)
last_tok = metrics.last_token_ts if hasattr(metrics, "last_token_ts") else 0.0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't like the repeated use of hasattr. Can you import from vllm import RequestOutput as RequestOutput, and use that to change the type from Any to vllm.RequestOutput? And then you can access the field directly, with the type RequestStateStats | None.

The only reason I could see this being necessary is if some versions of vLLM that you want to support have different fields here. But this kind of thing makes static type analysis fail, resulting in successful runs with blank metrics if vLLM's types change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal filed by core contributor or associate priority-low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants