feat(vllm): gate chat-shaped Prometheus collectors on embedding worker - #9886
Conversation
Mirrors the SGLang publisher gating change for the vLLM backend. The vLLM ``StatLoggerFactory`` previously constructed a ``DynamoStatLoggerPublisher`` (with its ``WorkerMetricsPublisher`` / NATS endpoint + chat-shaped ``LLMBackendMetrics`` gauges: ``total_blocks``, ``gpu_cache_usage``, ``model_load_time``) on every worker, including embedding workers running pooling engines that emit no ``SchedulerStats``. The result was a /metrics scrape full of gauges stuck at zero, hiding actual embedding signal. Changes ------- ``StatLoggerFactory`` gains an ``embedding_worker: bool`` flag. When set, ``create_stat_logger`` returns a new ``_NoopStatLogger`` (record + log_engine_initialized are pass-through) and the chat-shaped ``DynamoStatLoggerPublisher`` is never constructed. ``setup_vllm_engine`` inspects the same flag on the factory and skips the ``LLMBackendMetrics(registry=DYNAMO_COMPONENT_REGISTRY, ...)`` registration entirely, so no chat-shaped collectors land on the embedding worker's registry. ``EngineSetupResult`` and ``setup_vllm_engine``'s return type widen the trailing element to ``Optional[LLMBackendMetrics]``. ``_create_embedding_worker`` in ``worker_factory.py`` flips the new flag to ``True``; the chat/decode and prefill paths pass through unchanged. vLLM still calls the factory unconditionally during ``AsyncLLM`` init, so the no-op logger is necessary to keep that seam happy without leaking chat-shaped state. Embedding-shaped metrics (e.g. ``dynamo_embedding_*`` histograms) are registered on the Rust frontend, not this worker -- so this change removes the irrelevant gauges without taking any embedding observability with it. Tests ----- New ``tests/test_vllm_publisher.py`` covers: - factory returns ``_NoopStatLogger`` on the embedding path and does NOT construct ``DynamoStatLoggerPublisher`` (asserted by monkey-patching the constructor to raise). - ``_NoopStatLogger.record`` accepts vLLM's positional + keyword call shapes without raising, including ``scheduler_stats=None``. - the embedding factory does not trip the ``component_gauges is not None`` assert that guards the chat path. - the default (chat) factory still wires up ``DynamoStatLoggerPublisher`` with the supplied ``component_gauges``. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
WalkthroughThis PR adds embedding-worker support to the vLLM stat logger by introducing a no-op logger implementation and gating the ChangesEmbedding Worker Stat Logger Gating
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@components/src/dynamo/vllm/main.py`:
- Line 415: The code dereferences component_gauges even when it's None
(embedding path), causing a crash; wrap any calls like
component_gauges.set_model_load_time(load_time) (and any other
component_gauges.* calls in the same function/section around lines 440-458) with
a None check (e.g., if component_gauges is not None:
component_gauges.set_model_load_time(...)) so writes are skipped for embedding
workers; update the same guard pattern wherever component_gauges is used in this
function to avoid startup errors.
In `@components/src/dynamo/vllm/tests/test_vllm_publisher.py`:
- Around line 26-31: The test module's pytestmark list includes a framework
marker pytest.mark.vllm but lacks the required single component marker; update
the pytestmark list (in test_vllm_publisher.py where the pytestmark variable is
defined) to include exactly one of the component markers — e.g., add
pytest.mark.core alongside pytest.mark.vllm (keeping the other existing markers
pytest.mark.unit, pytest.mark.gpu_0, pytest.mark.pre_merge) so the module
satisfies the (backend × component) marker contract.
🪄 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
Run ID: 21a54296-dfd1-4b59-9727-292faa002414
📒 Files selected for processing (4)
components/src/dynamo/vllm/main.pycomponents/src/dynamo/vllm/publisher.pycomponents/src/dynamo/vllm/tests/test_vllm_publisher.pycomponents/src/dynamo/vllm/worker_factory.py
…t __init__ Two CI failures from the previous commit on this branch: 1. mypy in ``components/src/dynamo/vllm/main.py:587`` complained that ``Item "None" of "LLMBackendMetrics | None" has no attribute "set_model_load_time"``. ``component_gauges`` was widened to ``Optional[LLMBackendMetrics]`` so the chat-shaped registration could be skipped on the embedding-worker path, but the unconditional ``component_gauges.set_model_load_time(load_time)`` call after engine construction was not guarded. Skipping is the correct behavior on the embedding path -- there is no collector to publish to. 2. Three new unit tests in ``test_vllm_publisher.py`` failed with ``TypeError: Can't instantiate abstract class _NoopStatLogger without an implementation for abstract method '__init__'``. vLLM's ``StatLoggerBase`` marks ``__init__`` as abstract, so subclasses must declare one even when they hold no state. Adding an empty ``__init__`` (mirroring how ``DynamoStatLoggerPublisher`` satisfies the same constraint via its own signature) unblocks construction. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Two follow-ups from review on the embedding-worker metric gating PR: 1. ``components/src/dynamo/vllm/tests/test_vllm_publisher.py`` -- ``pytestmark`` had ``vllm`` + ``unit`` + ``gpu_0`` + ``pre_merge`` but no component marker. Per the project's marker contract (framework markers ``vllm``/``trtllm``/``sglang`` must be paired with exactly one of ``multimodal``/``router``/``kvbm``/``core`` so CI fans out per (backend × component)), add ``pytest.mark.core``. The test exercises the chat/embedding worker factory split, which is "core" backend behavior, not multimodal/router/kvbm. 2. ``components/src/dynamo/vllm/publisher.py::_NoopStatLogger`` -- tighten the ``__init__`` signature to ``(vllm_config=None, engine_index=0)`` instead of ``()``. Either works today because ``StatLoggerFactory.create_stat_logger`` invokes the constructor with no args, but vLLM's concrete ``StatLoggerBase`` subclasses conventionally accept those two parameters, so matching the shape keeps the no-op a drop-in if vLLM ever changes its factory call pattern to invoke subclasses directly with the stat-logger config. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Per repo policy: no internal ticket IDs in source code. The rationale the comment carries (chat-shaped gauges aren't registered on the embedding worker path) is self-contained and doesn't need the ticket reference. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Per @jh-nv's review feedback on PR #9886: dropping the leading underscore makes the no-op logger available for any future worker shape that wants to satisfy vLLM's ``StatLoggerBase`` factory contract without registering Prometheus collectors -- not just the embedding worker that drove this commit. Also rewrite the class docstring so the general-purpose framing (rather than embedding-worker-specific framing) actually invites reuse. ``NoopStatLogger`` is now a callable name, not a private embedding-only helper. Updates the test file's import + ``isinstance`` checks to track the rename. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
CI pre-commit failed on the previous commit (renaming ``_NoopStatLogger`` to ``NoopStatLogger``) because the import block ended up alphabetically out of order. isort wants ``DynamoStatLoggerPublisher, NoopStatLogger, StatLoggerFactory``. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
ai-dynamo#9886) Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
#9886) Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Overview:
Mirrors the SGLang publisher gating (PR #9830) for the vLLM backend.
vLLM's
StatLoggerFactorypreviously constructed aDynamoStatLoggerPublisher(with itsWorkerMetricsPublisher/ NATS endpoint + chat-shapedLLMBackendMetricsgauges --total_blocks,gpu_cache_usage,model_load_time) on every worker, including embedding workers running pooling engines that emit noSchedulerStats. The result on an embedding deployment was a/metricsscrape full of gauges stuck at zero, drowning out the embedding-shaped collectors that operators actually want to alert on.Details:
components/src/dynamo/vllm/publisher.py--StatLoggerFactorygains anembedding_worker: boolflag. When set,create_stat_loggerreturns a new_NoopStatLogger(record+log_engine_initializedare pass-through) and the chat-shapedDynamoStatLoggerPublisheris never constructed. Thecomponent_gauges is not Noneassert that guards the chat path is also skipped on the embedding branch, since the factory no longer needs gauges to wire up.components/src/dynamo/vllm/main.py--setup_vllm_engineinspectsstat_logger.embedding_workerand skips theLLMBackendMetrics(registry=DYNAMO_COMPONENT_REGISTRY, ...)registration entirely on the embedding path, so no chat-shaped collectors land on the embedding worker's registry. Return-type element widens toOptional[LLMBackendMetrics].components/src/dynamo/vllm/worker_factory.py--_create_embedding_workerflipsembedding_worker=Trueon the factory it constructs.EngineSetupResultwidens the trailing element to match. The chat/decode and prefill paths pass through unchanged.vLLM still calls the factory unconditionally during
AsyncLLMinit, so the no-op logger is necessary to keep that seam happy without leaking chat-shaped state into the embedding worker.Embedding-shaped metrics (e.g.
dynamo_embedding_batch_size,dynamo_embedding_input_tokens,dynamo_embedding_latency_seconds) are registered on the Rust frontend, not this worker -- so this change removes the irrelevant gauges without taking any embedding observability with it.Where should the reviewer start?
components/src/dynamo/vllm/publisher.py-- theStatLoggerFactory.create_stat_loggershort-circuit and the new_NoopStatLogger. This is the meat of the change.components/src/dynamo/vllm/main.py-- thesetup_vllm_enginebranch that decides whether to registerLLMBackendMetrics.components/src/dynamo/vllm/worker_factory.py-- single call-site flip in_create_embedding_worker.components/src/dynamo/vllm/tests/test_vllm_publisher.py-- four unit tests. The embedding-worker test patchesDynamoStatLoggerPublishertoraiseto prove the constructor doesn't run on that path; the chat-worker sibling test verifies the default path still wires up the publisher.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Refactor