[Engine Core] Gather engine notifications from workers to frontends by wseaton · Pull Request #51433 · vllm-project/vllm

wseaton · 2026-08-07T18:56:41Z

First split of #45411. Notification channel only; LoRA events, the Rust frontend consumer, and the scheduler hook will follow as separate stacked PRs.

flowchart LR
    W0["Worker rank 0<br/>notification buffer"] --> G["EngineCore<br/>gather from every rank"]
    WN["Worker rank N<br/>notification buffer"] --> G
    G --> O["EngineCoreOutputs"]
    O --> F0["API server 0"]
    O --> FN["API server N"]

vllm/v1/notifications.py: tagged msgspec union with CustomNotification(key, payload), and a process-local worker buffer.
WorkerBase.take_notifications(): drains that buffer. One default implementation covers every worker type.
EngineCore.gather_worker_notifications(): collective_rpc("take_notifications"), keeping every rank.
EngineCoreOutputs.engine_notifications: appended field, plus the matching Rust WireEngineCoreOutputs field and protocol/notifications.rs.
EngineCoreProc broadcasts to all API servers so their /metrics agree.

Why gather instead of piggybacking on ModelRunnerOutput: the executor only reads one rank's reply, so anything the other ranks published would just get dropped, and some paths (non-last PP rank via with_kv_conn_output_only) return early without a full output.

The gather itself fires once when the serving loop starts, then only if you opt in with VLLM_WORKER_NOTIFICATION_POLL_INTERVAL (off by default). This keeps the rpc off of the engine steps path.

#54830 adds a doorbell on top of this (the output rank flags a non-empty buffer on ModelRunnerOutput, and the engine core gathers after that step), so in-tree producers do not depend on polling.

cargo test -p vllm-engine-core-client --lib: 101 passed. pytest tests/v1/engine/test_notifications.py: 13 passed.

Stack

[Engine Core] Gather engine notifications from workers to frontends #51433 engine notification channel (this PR)
[Metrics][LoRA] Publish LoRA load events and expose loaded adapters in metrics #54830 LoRA load events, doorbell, and Python Prometheus metrics
[Rust][Metrics] Consume LoRA load events in the Rust frontend #54833 Rust frontend consumer for the same metrics

Each PR is based on main and includes the commits of the ones before it; review only the last commits of each.

AI assistance disclosure

Developed code and the test harness with AI assistance (Claude Code). All changes were reviewed line-by-line by myself, who ran the tests and reviewed output.

claude

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.

wseaton · 2026-08-07T19:57:28Z

Something non-obvious here is that certain consumer use-cases for this channel (Like if you wanted nixl transfer metrics from different TP ranks to fold back to the frontend, eg. when using modelexpress as a weight loader plugin) require that rank output from ranks other than rank=0 are not discarded. Marking as draft until I settle on a design that works for both cases.

Edit: The stated use case (weight transfer metrics) is moot until vllm can serve metrics before/during engine initialization.

claude

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.

mergify · 2026-08-13T20:13:35Z

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

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

mergify · 2026-08-21T01:41:42Z

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

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

yewentao256

Generally LGTM, thanks for the work!

Also want an approval from @njhill or @BugenZhao

github-actions · 2026-09-01T20:28:49Z

✅ @wseaton, CI is now available for this PR.

/ci run starts upstream CI; /amd-ci run starts AMD CI only.
/ci retry retries failed jobs in the CI build for the current PR head. If the current head has no CI build, it starts a new CI build for the current head containing only jobs that failed in the latest earlier CI build for this PR.
/amd-ci retry retries failed jobs in AMD CI for the current PR head. Use /amd-ci run when the current head has no AMD CI build.
/ci cancel cancels scheduled or running CI builds for this PR branch; /amd-ci cancel does the same for AMD CI only.

coderabbitai · 2026-09-02T21:11:01Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 82cbd4bf-82c3-4d1b-9460-b13713dc0518

📥 Commits

Reviewing files that changed from the base of the PR and between f2e2936 and 90fd7ec.

📒 Files selected for processing (12)

rust/src/engine-core-client/src/protocol/mod.rs
rust/src/engine-core-client/src/protocol/notifications.rs
rust/src/engine-core-client/src/protocol/output.rs
rust/src/engine-core-client/src/tests/client.rs
rust/src/engine-core-client/src/tests/python_compat.py
tests/v1/engine/test_notifications.py
vllm/envs.py
vllm/v1/engine/__init__.py
vllm/v1/engine/core.py
vllm/v1/engine/core_client.py
vllm/v1/notifications.py
vllm/v1/worker/worker_base.py

🚧 Files skipped from review as they are similar to previous changes (12)

vllm/v1/worker/worker_base.py
rust/src/engine-core-client/src/protocol/mod.rs
vllm/envs.py
vllm/v1/engine/init.py
rust/src/engine-core-client/src/protocol/output.rs
rust/src/engine-core-client/src/tests/client.rs
vllm/v1/engine/core_client.py
vllm/v1/engine/core.py
rust/src/engine-core-client/src/protocol/notifications.rs
tests/v1/engine/test_notifications.py
vllm/v1/notifications.py
rust/src/engine-core-client/src/tests/python_compat.py

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Summary

Summary by CodeRabbit

New Features
- Added support for custom engine notifications from workers to frontends.
- Notifications are delivered across in-process and multi-process execution paths, including cross-rank gathering.
- Added configurable notification polling, disabled by default.
- Added bounded buffering with ordered delivery to prevent excessive queue growth.
Bug Fixes
- Notification-only outputs are now preserved and transmitted correctly.
Tests
- Added coverage for buffering, polling, broadcasting, serialization compatibility, and end-to-end notification delivery.

Walkthrough

Adds tagged custom notification types to the Rust and Python protocols, a bounded worker queue, rank gathering, polling, and frontend delivery. Engine outputs carry optional notifications through in-process and multiprocessing clients. Tests cover serialization, buffering, concurrency, polling, fan-out, and end-to-end delivery.

Changes

Engine notification pipeline

Layer / File(s)	Summary
Notification protocol and wire contract `rust/src/engine-core-client/src/protocol/`, `rust/src/engine-core-client/src/tests/`, `vllm/v1/engine/__init__.py`	Defines tagged custom notifications and adds optional notification fields to Rust and Python engine output messages.
Worker notification buffering `vllm/v1/notifications.py`, `vllm/v1/worker/worker_base.py`, `tests/v1/engine/test_notifications.py`	Adds a bounded, thread-safe queue with ordered draining, overflow handling, and worker drain coverage.
Engine gathering and frontend delivery `vllm/v1/engine/core.py`, `vllm/v1/engine/core_client.py`, `vllm/envs.py`, `tests/v1/engine/test_notifications.py`	Gathers notifications across ranks, polls workers using the configured interval, flushes in-process outputs, broadcasts process outputs, and validates polling and end-to-end delivery.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 90fd7

This change adds a worker-to-frontend notification channel with optional polling disabled by default. No concrete merge-blocking risk is established for the current implementation.

Sequence Diagram(s)

sequenceDiagram
  participant WorkerBase
  participant Executor
  participant EngineCore
  participant EngineCoreProc
  participant InprocClient
  participant AsyncMPClient
  WorkerBase->>Executor: take_notifications
  Executor->>EngineCore: collective_rpc("take_notifications")
  EngineCore->>EngineCore: gather_worker_notifications
  EngineCore->>EngineCore: _flush_notifications
  EngineCore->>InprocClient: EngineCoreOutputs(engine_notifications)
  EngineCoreProc->>AsyncMPClient: enqueue EngineCoreOutputs(engine_notifications)

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 67.16% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 67 functions across 12 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: gathering engine notifications from workers and delivering them to frontends.
Description check	✅ Passed	The description directly explains the notification channel, affected components, data flow, configuration, follow-up work, and test results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai · 2026-09-03T20:59:17Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

Worker-side producers publish EngineNotification values that the model runner attaches to ModelRunnerOutput; EngineCore forwards them on EngineCoreOutputs.engine_notifications, broadcasting from EngineCoreProc so every API server sees the same events. EngineCoreOutputs is array_like and omit_defaults does not trim trailing fields, so the appended field lengthens every message. serde_tuple rejects a longer array than it knows, so the Rust wire struct has to move in the same commit. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Build a local ModelRunnerOutput instead of mutating the shared EMPTY_MODEL_RUNNER_OUTPUT singleton, and add cover for the appended array_like field surviving a msgpack round-trip and for EngineCoreProc fanning one event out to every frontend. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

The fixture mirror lacked the new field, so the cross-language test only proved the tolerated direction: Python encoding 8 elements into a Rust struct that knows 9. Encoding all 9 is the direction this PR depends on. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

…ation take_worker_notifications returns None instead of building a list on every step when no producer is installed. Drop gc=False from CustomNotification, whose payload is plugin-supplied: a cycle through an untracked struct is never collected. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Draining into ModelRunnerOutput only reached the executor's output_rank, so producers on other ranks were discarded, and paths that return early (non-last PP rank via with_kv_conn_output_only) never drained at all, accumulating in the worker for the life of the process. Gather with collective_rpc("take_notifications") instead, keeping every rank's events. The gather is an rpc, so it runs between steps rather than inside one: once before serving, on in-tree producers' own state changes, and on VLLM_WORKER_NOTIFICATION_POLL_INTERVAL when set. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

The DP override of run_busy_loop never called the startup gather or the interval poll, so notifications were never collected under data parallelism. Trim the gather docstrings and drop the docstring example's nonexistent StatLoggerBase kwarg while here. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Pin the DP busy loop's startup gather and per-iteration poll (the wiring that previously shipped missing), and the poll's off-by-default and interval gating. Drop test_empty_drain_allocates_nothing; the roundtrip test already asserts the quiet drain returns None. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

A real engine proc and worker: publish_worker_notification runs in the worker via collective_rpc, the interval poll gathers it, and the frontend client reads it off EngineCoreOutputs alongside generation output. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

The idle input-queue wait now times out at the poll interval and gathers, so an event published while the engine is idle reaches the frontend without waiting for the next request; the knob now bounds notification latency in both states. The interval poll moves to post_step, which both the busy loops and InprocClient reach, closing the in-process gap (InprocClient also gathers load-time events at construction). The gather no longer swallows executor failures: MultiprocExecutor stops dequeuing at the first failed rank, so a swallowed error leaves stale rank replies queued for the next collective. The worker buffer is capped at 1024, dropping the oldest, and the payload contract (string keys, msgpack values) is documented. The e2e now proves idle delivery with no request in flight and bounds every blocking wait. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

InprocClient.get_output now flushes after post_step; step_fn flushes before the gather runs, so a notification gathered on the final step had no later flush and sat in _pending_notifications until a call that a request-less frontend never makes. The worker buffer becomes a deque under a lock: the bounded list's len-check-then-delete raced the drainer's swap, and a publisher could del from the freshly emptied list. deque(maxlen) drops the oldest without a second mutation. The e2e wait helper re-raises drain-thread exceptions so an engine failure surfaces as itself instead of a timeout. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Co-authored-by: Wentao Ye <44945378+yewentao256@users.noreply.github.com> Signed-off-by: Will Eaton <weaton@redhat.com>

coderabbitai · 2026-09-06T03:49:42Z

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

wseaton · 2026-09-09T17:46:27Z

@njhill @BugenZhao bumping this one, trying to optimistically get it and #54830 in before the dev cut so we can ship some llm-d routing optimizations for LoRA

wseaton requested review from BugenZhao, WoosukKwon, njhill and yewentao256 as code owners August 7, 2026 18:56

claude Bot reviewed Aug 7, 2026

View reviewed changes

mergify Bot added rust mrv2 Model Runner V2 specific labels Aug 7, 2026

wseaton marked this pull request as draft August 7, 2026 19:56

wseaton changed the title ~~[Engine Core] Add engine notification channel from workers to frontends~~ [Engine Core] Gather engine notifications from workers to frontends Aug 7, 2026

wseaton force-pushed the weaton/engine-notifications-channel branch from 92b6436 to 2a9c07f Compare August 7, 2026 20:44

wseaton marked this pull request as ready for review August 10, 2026 20:33

claude Bot reviewed Aug 10, 2026

View reviewed changes

mergify Bot added the needs-rebase label Aug 13, 2026

wseaton force-pushed the weaton/engine-notifications-channel branch from 95b2c23 to b631219 Compare August 17, 2026 14:12

mergify Bot removed the needs-rebase label Aug 17, 2026

wseaton force-pushed the weaton/engine-notifications-channel branch 3 times, most recently from b4421a3 to f8b1bc0 Compare August 17, 2026 17:49

mergify Bot added the needs-rebase label Aug 21, 2026

wseaton force-pushed the weaton/engine-notifications-channel branch from a961a56 to 2f70598 Compare September 1, 2026 19:15

wseaton mentioned this pull request Sep 1, 2026

[Metrics][LoRA] Publish LoRA load events and expose loaded adapters in metrics #54830

Open

wseaton force-pushed the weaton/engine-notifications-channel branch from 2f70598 to 5a2fadd Compare September 1, 2026 19:32

This was referenced Sep 1, 2026

[Rust][Metrics] Consume LoRA load events in the Rust frontend #54833

Draft

[Feature][Rust Frontend]: Support --lora-modules for static adapter loading #54836

Closed

[Rust Frontend] Support --lora-modules for static adapter loading #54837

Merged

yewentao256 reviewed Sep 1, 2026

View reviewed changes

Comment thread tests/v1/engine/test_notifications.py Outdated

Comment thread tests/v1/engine/test_notifications.py Outdated

yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 1, 2026

mergify Bot removed the needs-rebase label Sep 1, 2026

wseaton mentioned this pull request Sep 2, 2026

Observability and tracing for LoRA adapter routing (cache hit vs miss) llm-d/llm-d-router#1500

Open

4 tasks

atantawi mentioned this pull request Sep 3, 2026

fix(lora-affinity): correct ActiveModels docs and remove wrong capacity llm-d/llm-d-router#2625

Merged

wseaton force-pushed the weaton/engine-notifications-channel branch from 1e8a3c5 to cd67406 Compare September 3, 2026 20:58

wseaton added 12 commits September 5, 2026 23:46

[Engine Core] Trim notification comments

9696250

Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>

Drop redundant notification tests

90fd7ec

Co-authored-by: Wentao Ye <44945378+yewentao256@users.noreply.github.com> Signed-off-by: Will Eaton <weaton@redhat.com>

wseaton force-pushed the weaton/engine-notifications-channel branch from cd67406 to 90fd7ec Compare September 6, 2026 03:49

wseaton mentioned this pull request Sep 9, 2026

feat(scorer): add lora-load-state-scorer reading vLLM adapter residency llm-d/llm-d-router#2792

Draft

Uh oh!

Uh oh!

Conversation

wseaton commented Aug 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack

AI assistance disclosure

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

wseaton commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

mergify Bot commented Aug 13, 2026

Uh oh!

mergify Bot commented Aug 21, 2026

Uh oh!

yewentao256 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

coderabbitai Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot commented Sep 3, 2026

Uh oh!

coderabbitai Bot commented Sep 6, 2026

Uh oh!

wseaton commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wseaton commented Aug 7, 2026 •

edited by coderabbitai Bot

Loading

wseaton commented Aug 7, 2026 •

edited

Loading

coderabbitai Bot commented Sep 2, 2026 •

edited

Loading