Conversation
|
Documentation preview: https://vllm--54830.org.readthedocs.build/en/54830/ |
931e755 to
1d1959c
Compare
1d1959c to
e6467f5
Compare
|
Downstream consumer here — @wseaton pointed me at this stack from llm-d-router's LoRA scorer is broken today in two directions, and this PR fixes both.
Confirmed by running the router's own extractor and scorer, kept as mutation-tested Go
I read the producer rather than the label names — misreading a label name is what put me
|
|
Cross-linking one downstream consumer: @dmitripikus owns |
e6467f5 to
2bf943f
Compare
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
|
This pull request has merge conflicts that must be resolved before it can be |
|
Withdrawing my third question: it was already answered, on #54833, before I asked it. Your test report there (comment) shows the startup scrape under with the Python frontend giving the same numbers. That is exactly the scrape-before-traffic case I said I could not confirm, and it is worker-side rather than a frontend registry — the event's lists come from Questions 1 and 2 stand, and 1 is the one that matters downstream: with the info-metric untouched, a consumer still cannot separate running from queued, so llm-d-router's "adapter queued to load" tier stays dead code even after this lands, and |
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>
The notification channel only gathered at startup or on an opt-in poll, so a producer that publishes during a step (LoRA activation evicting a GPU slot) was invisible by default. The output rank now sets ModelRunnerOutput.worker_notifications_pending when its buffer is non-empty and the engine core gathers from every rank after that step, so a quiet step still costs no rpc. add_lora/remove_lora/pin_lora gather after the executor call, since an idle engine has no step to ring the doorbell. LoRALoadEvent joins the tagged union on both the Python and Rust sides: a full snapshot of the worker's adapter caches by name, so consumers replace rather than merge. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
LoRAModelManager exposes the ids resident in each cache tier and the worker manager resolves them to adapter names, remembering names from _load_adapter and pruning them once an adapter is evicted. The model runner mixin snapshots that state after every activation, add, remove, pin and reset, and publishes a LoRALoadEvent only when the set changed. Dummy warmup adapters go through the manager directly, so they never publish. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
vllm:lora_requests_info only shows adapters with requests in flight, so routers cannot tell which replica already has an adapter warm. LoRALoadEvent notifications now feed three gauges: one series per resident adapter (adapter_name, level gpu|cpu, pinned) plus per-tier counts. Each event is a full snapshot, so series for evicted adapters are zeroed and removed; the zero matters under prometheus multiprocess mode, where remove() leaves the mmap-backed sample, and under Ray, which cannot delete series at all. Stat loggers receive notifications through a new non-abstract record_engine_notifications hook rather than a new record() parameter, so out-of-tree loggers keep working unchanged. lora_requests_info is marked deprecated in its help text. Assisted-by: Claude Signed-off-by: Will Eaton <weaton@redhat.com>
2bf943f to
4f90142
Compare
A router comparing occupancy against capacity needs max_loras before any adapter has served, but the only place it was exposed is the max_lora label on the deprecated vllm:lora_requests_info gauge, which appears on first use. Add vllm:max_gpu_lora_adapters and vllm:max_cpu_lora_adapters, set once per engine at startup. Signed-off-by: Will Eaton <weaton@redhat.com>
The worker LoRA manager times each disk load and each CPU-to-GPU
activation and ships them on the next LoRALoadEvent. The Prometheus
logger observes them in vllm:lora_adapter_load_seconds{transition},
so a router can price a request that lands on a server without the
adapter instead of assuming a fixed penalty.
Signed-off-by: Will Eaton <weaton@redhat.com>
Rank is a proxy for adapter size and so for what a miss costs. The worker snapshot carries each resident adapter's rank and the gauge exposes it as a label, so a router can weigh a rank-64 miss differently from a rank-8 one. Signed-off-by: Will Eaton <weaton@redhat.com>
|
Marking as ready for review (have end to end tests now in the llm-d router), but still needs it's first PR in the stack reviewed first. |
The design doc said the old gauge was replaced by "three gauges" while listing five and a histogram, and still said deprecation was a future decision. Keep the old gauge's notes with its description, state that it is still emitted under the deprecation policy, and point downstream users at the replacement metrics. Signed-off-by: Will Eaton <weaton@redhat.com>
The Rust frontend exported the old HELP text, so the deprecation notice depended on which frontend served /metrics. Use the same text as the Python logger. Also correct the design doc: the gauge is registered with multiprocess_mode="sum", not "livemostrecent". Signed-off-by: Will Eaton <weaton@redhat.com>
|
This pull request has merge conflicts that must be resolved before it can be |
Stack
Each PR is based on
mainand includes the commits of the ones before it; review only the last commits of each.End-to-end testing on the Rust frontend needs #54836 (
--lora-modulessupport), tracked separately.Note: the metrics report worker-side residency.
/v1/unload_lora_adapteron the Python server only drops the frontend registry entry (#54839), so an unloaded adapter keeps its series until LRU eviction. This bug is now actually visible!