Conversation
timzsu
marked this pull request as ready for review
September 15, 2026 15:54
kaiitunnz
requested changes
Sep 15, 2026
kaiitunnz
left a comment
Collaborator
There was a problem hiding this comment.
A couple of comments. PTAL.
timzsu
force-pushed
the
zsu/placement-fix
branch
from
September 15, 2026 18:30
0fdca95 to
8cc5de3
Compare
When a vLLM load fails because free GPU memory is insufficient (a resident warm engine still holds the card), the failure is transient: the dispatcher requeues and the idle checker evicts the stale engine before the retry. Such failures now raise a retryable ExecutionError so the existing requeue path fires instead of failing the task permanently. The discriminator is the executor's own memory-constrained signal: whether any init attempt had to be sized below the requested gpu_memory_utilization because free memory was insufficient. vLLM 0.28 collapses both memory-pressure and child-side deterministic failures (model not found, bad config) into a generic RuntimeError in the parent, so exception type/chain cannot separate them; the free-memory signal is the reliable one. Deterministic failures with plenty of free memory stay retryable=False and fail fast. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
timzsu
force-pushed
the
zsu/placement-fix
branch
3 times, most recently
from
September 16, 2026 03:24
0bd73f3 to
e037fd2
Compare
kaiitunnz
reviewed
Sep 16, 2026
kaiitunnz
requested changes
Sep 16, 2026
timzsu
force-pushed
the
zsu/placement-fix
branch
3 times, most recently
from
September 16, 2026 06:20
738a524 to
1540582
Compare
Homogeneous workers on a multi-GPU node score byte-identically in _collect_worker_metrics (same vram/sys_ram/cores/cost), and the stable sort then sends every job to the same card while the other sits idle. Track a per-worker last-dispatch timestamp in the dispatcher and use it as a pure tie-breaker sort key, so identical workers round-robin instead of piling onto one card. The recency term is kept out of the score and used only as a secondary sort key (unbounded), so it breaks ties between equal scores and never overrides a real capacity difference. It lives in dispatcher memory (not Redis) because it is not mission-critical and need not be persistent. A dispatcher hook (_on_worker_unregistered) drops the stale entry when a worker unregisters, wired into the monitoring and watchdog unregister paths. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
timzsu
force-pushed
the
zsu/placement-fix
branch
from
September 16, 2026 06:27
1540582 to
0ca1ee0
Compare
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
kaiitunnz
requested changes
Sep 16, 2026
…ook public Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
Simplify both recency keys to last_dispatch.get(worker.id, 0.0), which drops the explicit None branch. best_fit negates it because that sort is descending; _select_min_capacity must not, because its sort is ascending and the key sits at position 2 of the tuple. Negating there reversed the policy: the pool's most-recently-dispatched worker sorted first, and a never-dispatched worker sorted last instead of first. The existing suite did not cover recency under min_satisfying, so the inversion passed 34 green tests. Add the two cases that fail without this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
The tuple reshape that added the recency key dropped the [:5] slice, so the debug payload grew one dict per candidate on every dispatch. Nothing consumes top_scores, and _select_min_capacity still caps its own list at five. Signed-off-by: Noppanat Wadlom <noppanat.wad@gmail.com>
_stable_jitter already perturbs the best-fit score from (task_id, worker_id) and is on by default (SCHEDULER_SELECTION_JITTER=1e-3), so homogeneous workers never tie on score and the recency key never fired. Measured over 200 tasks on two identical workers: 103/97 with jitter on, 200/0 with it off. The tie-break was inert wherever the dispatcher runs, since it always passes task_id. The diagnosis reasoned from the measured throughput tie plus the stable sort and did not account for the jitter term added to the score in between, so the pile-up it predicted does not occur under the default configuration. Its own placement recommendation was a free-VRAM utilisation term, which would move throughput rather than compete with a 1e-3 perturbation; that needs telemetry the server does not collect yet. Removes the per-worker last-dispatch map, the unregister hook and its two call sites, and both recency sort keys. Keeps the vLLM requeue fix, which addresses the failure that was actually observed, and the worker-metrics typing tightened to dict[str, float] with worker_id read from the Worker rather than carried in the metrics. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Zhengyuan Su <su.zhengyuan@u.nus.edu>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
A vLLM engine-init failure caused by transient GPU memory pressure is classified as a deterministic error, so the dispatcher never requeues it and the task fails permanently. Any workload that alternates models on one worker fails intermittently as a result.
Changes
src/worker/executors/vllm_executor.py— track whether any init attempt had to be sized below the requestedgpu_memory_utilizationfor lack of free memory, and pass that asretryablewhen raisingExecutionError.src/server/dispatcher/worker_selector.py— tighten the worker-metrics dict todict[str, float]:worker_idis read from theWorkerobject rather than carried as a string inside a float-valued mapping, and the unusedbest_scorebinding is dropped.tests/worker/test_vllm_retryable_classification.py— new; covers memory-constrained vs deterministic failure classification.Design
The worker keeps a warm vLLM engine resident across tasks keyed by model, evicting only on a model change; the MP subprocess is reclaimed only by the idle checker after
WORKER_EXECUTOR_IDLE_CLEANUP_SEC(default 60s), and a controlledExecutionErrordeliberately keeps it warm. A task arriving seconds after a different-model task therefore sizes its engine while the previous one still holds the card, and dies during init.The dispatcher already implements the correct recovery — it requeues failed tasks, bounded by
record.max_attempts. It never fires here becauseExecutionErrordefaults toretryable=Falseand the raise site passes no flag, sofailed_task_can_retryshort-circuits. Marking only the memory-pressure case retryable lets the existing requeue path do its job while the idle checker frees the card.The discriminator is the executor's own memory-constrained signal rather than the exception type, because vLLM 0.28 collapses memory pressure and child-side deterministic failures (model not found, bad config) into one generic
RuntimeErrorin the parent. Deterministic failures on a card with free memory keepretryable=Falseand fail fast.Retry is the right shape here because the worker cannot evict what is holding the card. Measured on an idle RTX 5080 (vLLM 0.28):
_shutdown_llm()returns after 3.66s with the engine's 5856 MiB already back at baseline, released within 0.03s — so the executor reliably frees its own memory, synchronously, and tearing down the subprocess recovers nothing further. An allocation failure therefore means something else held the card at the same time, and if that holder is outside this worker there is nothing to tear down. Requeueing lets the task land once the card frees; evicting harder would not.A deeper fix exists and needs discussion before anyone builds it. The worker could report its actual free GPU memory rather than only
memory_total_bytes(whichGpuInfowrites once at register), letting admission refuse a placement that cannot fit. That is the honest root-cause fix for contention from a process FlowMesh does not manage. But it is a large change — schema, heartbeat and admission — and it may be solving a situation we would rather forbid than support: if nothing outside FlowMesh is ever supposed to share a card, the better answer may be to enforce that at deployment and keep the scheduler simple. Worth settling deliberately rather than implying by implementation.Alternatives rejected: