Repository navigation
[Frontend] Scale Omni serving across API processes - #6923
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/observability.md. Module owners: @lishunyang12 @vraiti @Sy0307, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
These are automated triage suggestions only — the final decision belongs to the maintainers. |
07aa397 to
dc6d466
Compare
hsliuustc0106
added
frontend
congw729
left a comment
congw729
left a comment
There was a problem hiding this comment.
The implementation and validation look solid from my review, and I did not find a concrete correctness blocker. However, this introduces a substantial new serving-process model and support boundary, and I’m not in a position to confirm that RFC #988 has sufficient demand or project-level agreement to merge.
Could we please get a review from the relevant maintainers before approval—particularly @alex-jw-brooks for the frontend/entrypoint and usability boundary, @tzhouam for the shared EngineCore lifecycle, and ideally @hsliuustc0106 for whether RFC #988 is currently aligned with the project roadmap?
It would also help to link RFC #988 directly from the PR description so the motivation and design history are visible to future reviewers.
hsliuustc0106
added
high priority
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: Sy03 <1370724210@qq.com>
Sy0307
added
ready
linyueqian
left a comment
linyueqian
left a comment
There was a problem hiding this comment.
Re-approving at d8458b84. I approved this at 1ad95f99 on 09-10 after a two-GPU H20 e2e (single and dual API workers, even request split across both workers, 409 on the process-local routes only in multi mode). Since then the branch merged main twice and picked up amy-why-3459's two P2s, so I compared the PR patch at 1ad95f99 against the patch at this head rather than re-reading the whole change; every hunk that differs is accounted for below.
Device locks (amy's first P2): acquire_device_locks now takes the set of physical device ids already held and locks only the difference, adding each newly locked id to the set, and launch_stage_engines threads one locked_devices set through the whole launch loop instead of deduplicating by device-group string. With stage 0 on GPU 0 and stage 1 on GPUs 0,1 the second stage locks only GPU 1 and never waits on its own descriptor. test_stage_runtime_overlapping_devices_acquire_real_locks_once exercises exactly that shape with real flock calls on isolated lock files, fails if the runtime sleeps at all, proves both locks are held while the launch context is open, and proves both are released afterwards, for one and two clients.
Duplex sessions (amy's second P2): both /v1/duplex and the native-duplex branch of /v1/realtime now run _reject_multi_api_duplex before the warmup wait and before the handler, closing with 1008 and multi_api_duplex_unsupported when the worker count is above one. test_multi_api_duplex_reconnect_rejected_on_each_worker builds two independent apps to stand for two workers and checks the rejection on each for all three paths; the single-worker test keeps the handler reachable. The guard keys on app.state.api_server_count, which omni_run_server_worker sets once from the launcher's client configuration and cross-checks against the CLI value, so it cannot drift from the real topology.
The remaining differences are the three suggestions from my first round landing (video lifecycle routes behind the single-worker dependency, the guard reading the resolved count and returning 503 when the topology is missing, TrackingNamespace.__getattr__ reading the inner namespace through object.__getattribute__), the main merge fallout (ABORT_TIMEOUT_S import, prepare_stage_config_inputs, robot_openpi_idle_timeout pop moving to the worker), a comment block in the Qwen3-TTS high-concurrency profile, and the simplified single-client exit in launch_stage_engines. Two small notes are inline as suggestions; neither changes the verdict.
Validation: static comparison of the two PR patches plus a read of the changed functions and both new tests; no PR code executed at this head. The general lane is green at d8458b84 (build 15378), Read the Docs passed, and the branch is mergeable against current main with no file overlap. The AMD lane is red on the inherited ROCm signature and is informational.
| # Session credentials and replay state are local to each frontend; a shared | ||
| # listening socket cannot route reconnects back to the session owner. | ||
| count = getattr(websocket.app.state, "api_server_count", None) | ||
| if count is None or (type(count) is int and count == 1): |
There was a problem hiding this comment.
[suggestion] This fails open when api_server_count is missing: count is None returns False and the session reaches the process-local handler, while the HTTP guard right above treats the same missing topology as 503. In production the worker always sets the value before serving, so this only matters for an app built without omni_run_server_worker, but the message sent on rejection already says "with initialized topology", so rejecting None here too would make the two guards agree and match the wording. Not blocking.
|
|
||
| # Only relevant for models using XD-RoPE (e.g, HunYuan-VL) | ||
| if self.uses_xdrope_dim > 0: | ||
| if getattr(self, "uses_xdrope_dim", 0) > 0: |
There was a problem hiding this comment.
[suggestion] The three getattr(self, "uses_xdrope_dim", 0) reads here and in gpu_generation_model_runner.py survived the revert of the ROCm CI changes but have nothing to do with API-server scaling; they make the runner tolerate a vLLM build that never sets uses_xdrope_dim. Fine to keep if the AMD lane needs it, but it deserves one line in the PR description so the next person tracing the attribute knows why the direct read became defensive.
|
@amy-why-3459 both of your P2s are addressed at |
|
@Sy0307 one more thing landed under this branch: #7544 merged as |
amy-why-3459
left a comment
amy-why-3459
left a comment
There was a problem hiding this comment.
Went through the full diff (26 files, +1930 / -204) and cross-checked a few assumptions against main and the installed vLLM source. The direction and the boundary design are right, and the test coverage is solid — 32 new cases covering rollback, parallel admission, the 409/503 guards, and pickle round-tripping.
Items already raised in this thread are not repeated here (duplex guard failing open, the uses_xdrope_dim getattr drive-by, per-device lock dedup, parent/worker config duplication). The following are new.
P1
1. Engine resources are shut down twice on the failure path
launch_stage_engines calls launch.shutdown() in its except BaseException handler. Once the exception propagates, run_multi_api_server_omni's finally calls engine_launch.shutdown() again — engine_launch is already bound by with ... as engine_launch before the body runs.
StageEngineLaunch.shutdown() is not idempotent across calls, because its dedup set is a local:
def shutdown(self) -> None:
seen: set[int] = set() # rebuilt on every callIt only dedups manager/coordinator aliasing within one call. Suggest an instance-level flag:
_shutdown_done: bool = field(default=False, init=False)
def shutdown(self) -> None:
if self._shutdown_done:
return
self._shutdown_done = True
...2. Stage startup timing instrumentation was dropped entirely
The _initialize_local_llm_replica refactor removed the G2/G3 breakdown. Comparing the two revisions:
main stage_runtime.py: perf_counter x6, including
"[stage_init] Stage-%s G3 device-lock acquire took %.3fs"
"[stage_init] Stage-%s G2 launch-lock wait=%.3fs, spawn=%.3fs, READY=%.3fs"
"[StageRuntime] Stage %s engine startup completed" / "Stage %s initialized"
PR head stage_runtime.py: perf_counter x0, none of the above
For a PR whose premise is CPU/frontend scalability, losing the per-stage spawn / lock-wait / READY split makes the remaining 1/2/4 API-count A/B work (still outstanding per the PR description) considerably harder to attribute. Please restore equivalent debug timing inside the launch_stage_engines replica loop.
Related: the deleted G3 comment explained why the parent must not hold the device lock when parallel_stage_init is on (it would self-deadlock waiting for the child's READY handshake). The new code has only if not self._parallel_stage_init: with that rationale gone.
3. trust_remote_code normalization is incomplete on the parent path
In _build_multi_api_stage_runtime:
trust_remote_code = kwargs.get("trust_remote_code")
if trust_remote_code is False:
trust_remote_code = None # local variable onlyThe trust_remote_code: False entry stays in kwargs. prepare_stage_config_inputs does not remove it either — with_trust_remote_code_override(..., None) only adds a key, never deletes one — so it reaches resolve_omni_config(cli_overrides=...) as an explicit False override.
The worker path does not behave this way: OmniEngineBase.__init__ consumes trust_remote_code as a declared parameter, so the key is never present in its kwargs. That is exactly the parent/worker divergence this PR sets out to eliminate. kwargs.pop("trust_remote_code", None) would align them.
(This only bites if --trust-remote-code is a BooleanOptionalAction rather than store_true, since get_explicit_kwargs_dict() returns only explicitly passed keys. Worth confirming the action type before deciding the priority.)
P2
4. Multi-API address allocation can reuse the upstream helper
stage_engine_startup.py hand-rolls:
addresses = EngineZmqAddresses(
inputs=[get_open_zmq_ipc_path() for _ in range(num_api_servers)],
outputs=[get_open_zmq_ipc_path() for _ in range(num_api_servers)],
)get_engine_zmq_addresses(vllm_config, num_api_servers, defer_api_server_ports=False) already does this (vllm/v1/engine/utils.py:1005 — the body is inputs=[_addr() ...], outputs=[_addr() ...], where _addr() returns an IPC path when client_local_only and a fixed-port TCP URI otherwise). Reusing it has a bonus: defer_api_server_ports=False guarantees no tcp://host:0, which makes the address.rsplit(":", 1)[-1] == "0" defensive check in launch_stage_engines unnecessary.
5. "First stage" is derived independently on each side
The parent hands engine_launch.resources[0].addresses to APIServerProcessManager. The worker re-derives it in run_omni_api_server_worker_proc:
first_stage_id = min(stage_addresses)
first_replica_id = min(stage_addresses[first_stage_id])resources[0] means "first replica of the first plan"; min() means "lowest stage_id". These agree today because _prepare_stage_plans() returns plans in order, but it is an implicit coupling — if plan ordering ever changes, the worker silently overlays the manager's addresses onto the wrong stage with no error. Separately, OmniClientConfig.stage_addresses is typed dict[int | str, dict[int | str, ...]], and min() raises TypeError on genuinely mixed keys.
Suggest carrying an explicit manager_owned_stage: tuple[int, int] in the client config and reading it on both sides.
6. Unused parameter in _build_multi_api_stage_runtime
num_api_servers has no reference in the function body (grepped at PR head; signature only). Either drop it or use it to absorb logic such as the sleep-mode check.
7. _start_api_server_process_manager depends on an upstream private attribute
for pipe in getattr(manager, "_address_pipes", ()):_address_pipes does exist (vllm/v1/utils.py:210), but it is private, and the getattr(..., ()) fallback means an upstream rename degrades silently into not cleaning up the pipes rather than failing. The __new__ + manual __init__ construction is tightly bound to upstream's constructor flow as well. At minimum, an explicit assertion plus a note on the vLLM version this tracks would make an upstream change fail loudly.
P3 / nits
with_trust_remote_code_overrideis applied twice on the OmniBase path (once inOmniEngineBase.__init__, once insideprepare_stage_config_inputs). The function is pure and idempotent, so no bug — just redundant._resolve_api_server_countand_reject_process_local_state_with_multiple_api_workersusetype(x) is not int. I assume the intent is to rejectbool;isinstance(x, int) and not isinstance(x, bool)states that intent more directly and does not also reject int subclasses.- In
/v1/realtime,_wait_for_duplex_warmupmoved from the function entry into the duplex branch, so the non-duplex path no longer waits for warmup. This looks deliberate, but it is a behavior change unrelated to multi-API scaling and is not mentioned in the PR description — worth calling out explicitly. - A whitespace-only line was added in the ASGI middleware in
api_server.py— diff noise. args._omni_stage_client_configsuses a private Namespace attribute as an IPC channel, and every worker receives all N configs and picks its own by index. It works (__getstate__carriesunfiltered_ns, andget_explicit_kwargs_dict()filters the key out), but it is O(N^2) data at larger N and the contract is untyped. Shipping only the worker's own entry would be tighter.
What reads well
- Guard placement: diffusion, remote/headless, intra-replica DP, Ray, fault tolerance, and elastic EP are all rejected before device locks are taken and engines are spawned, rather than failing partway through startup.
- The process-local 409 coverage is complete — all five video routes, voice upload/delete, sleep/wakeup, and the duplex WS paths. Using
dependencies=[Depends(...)]on the video routes correctly rejects before form parsing. launch_stage_enginescollapsing the single- and multi-client paths into one entry (per @yinpeiqi's earlier comment) is the right call, and theentered_contexts/exited_contextspartial-rollback handling is correct.- I verified the
watched_frontend_processesmechanism against the installed vLLM: the field exists atvllm/v1/engine/utils.py:1066, it is consumed at:1247, and vLLM's ownserve.py:384uses the same assignment pattern. Not a no-op.
Signed-off-by: Sy03 <1370724210@qq.com>
Sy0307
added
ready
|
@Sy0307 thanks for the rebase to |
Signed-off-by: Sy03 <1370724210@qq.com>
Sy0307
added
ready
linyueqian
left a comment
linyueqian
left a comment
There was a problem hiding this comment.
Re-approving at 04ae6aa0. This head is the approved d8458b84 patch rebased across #6849 (f1a6e7ce), plus the adaptation that rebase required: the multi-API startup in entrypoints/cli/serve.py now reads enable_sleep_mode from the typed model_config and falls back to the legacy engine_args, and derives async_chunk as any stage's connector_config.async_chunk with the same legacy fallback, which is exactly how omni_engine_base.py computes it on main after #7544 and #6849, so the parent process and the engine agree on chunking for both config shapes. test_build_multi_api_stage_runtime_matches_current_constructor is now parametrised over typed and legacy stages and over the downstream stage's chunking, so both paths are pinned. Every other hunk is byte-identical to the approved patch; the only other differences are blank lines in the stage-init test and context from #7675's rename.
The branch is zero commits behind main with no conflicts. Validation: static comparison of the approved patch against this head, plus a read of the two changed functions and the parametrised test; no PR code executed. I will approve once lane 15447 is green. One caveat on timing rather than content: main is currently red on the merge-only Diffusion · Wan22 Test after #6849 (#7704), and this head now depends on #6849's typed startup path, so I intend to hold the merge until that is resolved with a forward fix, because a revert of #6849 afterwards would conflict with this branch in more than a dozen files.
vLLM 719284f (vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removed `uses_xdrope_dim` from the model runner, so on a vLLM containing that commit the Omni runners raise AttributeError where they read it. vllm-project#6923 guarded four of the six read sites with getattr. Put the default in `OmniGPUModelRunner.__init__` instead and read the flag plainly everywhere: one enforcement point, and it reaches the two NPU runners that inherit this constructor, which the guards did not cover. The pinned version's value is preserved. The test constructs both GPU runners through the real parent constructor and the installed vLLM's config classes, admits a request through `_update_states`, and checks the position buffers and `_preprocess` output for plain, image and cached input. One case deletes the attribute after the parent constructor runs, so the pinned version also covers what the removal does. Signed-off-by: MrlixiangWE <mrdanaer@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com>
vLLM 719284f (vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removed `uses_xdrope_dim` from the model runner, so on a vLLM containing that commit the Omni runners raise AttributeError where they read it. vllm-project#6923 guarded four of the six read sites with getattr. Put the default in `OmniGPUModelRunner.__init__` instead and read the flag plainly everywhere: one enforcement point, and it reaches the two NPU runners that inherit this constructor, which the guards did not cover. The pinned version's value is preserved. The test constructs both GPU runners through the real parent constructor and the installed vLLM's config classes, admits a request through `_update_states`, and checks the position buffers and `_preprocess` output for plain, image and cached input. One case deletes the attribute after the parent constructor runs, so the pinned version also covers what the removal does. Signed-off-by: MrlixiangWE <mrdanaer@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com>
vLLM 719284f (vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removed `uses_xdrope_dim` from the model runner, so on a vLLM containing that commit the Omni runners raise AttributeError where they read it. vllm-project#6923 guarded four of the six read sites with getattr. Put the default in `OmniGPUModelRunner.__init__` instead and read the flag plainly everywhere: one enforcement point, and it reaches the two NPU runners that inherit this constructor, which the guards did not cover. The pinned version's value is preserved. The test constructs both GPU runners through the real parent constructor and the installed vLLM's config classes, admits a request through `_update_states`, and checks the position buffers and `_preprocess` output for plain, image and cached input. One case deletes the attribute after the parent constructor runs, so the pinned version also covers what the removal does. Signed-off-by: MrlixiangWE <mrdanaer@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com>
vLLM 719284f (vllm-project/vllm#56078) unified XD-RoPE into M-RoPE and removed `uses_xdrope_dim` from the model runner, so on a vLLM containing that commit the Omni runners raise AttributeError where they read it. Read the flag with a default in `OmniGPUModelRunner.__init__`. vllm-project#6923 guards four of the six read sites for runners built with `object.__new__`, which this repo does in tests; those guards stay, and this default is what covers the paths they do not, including the two NPU runners that inherit this constructor. The pinned version's value is preserved, and a vLLM that never sets the flag says so once in the log rather than silently serving 1-D positions. The test constructs both GPU runners through the real parent constructor and the installed vLLM's config classes, admits a request through `_update_states`, and checks the position buffers and `_preprocess` output. One case drops the attribute after the parent constructor returns, so the pinned version also covers what the removal does. Signed-off-by: MrlixiangWE <mrdanaer@gmail.com> Co-authored-by: zijianc2 <157244773+zijianc2@users.noreply.github.com>
Signed-off-by: Sy03 <1370724210@qq.com> Signed-off-by: Matthieu Laneuville <matthieu.laneuville@surf.nl>
Signed-off-by: Sy03 <1370724210@qq.com>
7 participants
Summary
Enable
--api-server-count > 1for local vLLM-Omni EngineCore pipelines. Each API worker owns its frontend processor, Orchestrator, request state, and per-replica channels; the parent owns one shared set of GPU stage engines and their lifecycle. Outputs return to the originating worker through itsclient_indexchannel.This is opt-in infrastructure for CPU-limited frontend/orchestration workloads. The default remains one API server. Configuration normalization is shared by the parent, API workers, and headless entrypoint; transport and engine ownership remain opt-in for multi-API serving.
Related: RFC #988, issue #4680, and the frontend/performance infrastructure work tracked by #6494. This PR does not change native model correctness.
Behavior and supported boundary
--stage-idmode.OmniClientConfigacross internal frontend builders and the engine/runtime boundary while retaining the upstream process-manager dictionary interface.The refresh merges main at
6e79d582and adopts its unifiedresolve_omni_configAPI, including the richer pipeline resolution metadata. It removes the redundantOmniBasefrontend-count filter, constructs per-worker configs withOmniClientConfig(...), preserves optional config paths, and adds regression coverage for shared frontend-option normalization. Single-stage channel routing and early rejection of pure/mixed diffusion pipelines remain covered.Workloads and performance status
Concurrent cold image/video inputs can benefit when frontend processing is CPU-limited. Many replicas producing frequent text/audio outputs can benefit when API processing and orchestration compete for the GIL. Each worker still has a serial Orchestrator and its own processor/cache state, so cache duplication, CPU contention, and actual request distribution matter. A single cold request, GPU-bound generation, or model loading/compilation may not become faster.
No final throughput percentage is claimed on this head. Earlier measurements included generic frontend offloading that was removed from this PR. The subsequent single-API output-queue experiments are also not measurements of multi-API scaling.
The remaining performance validation should compare API counts 1/2/4 with identical hardware, media, model/stage configuration, sampling/output lengths, and offered load; separate cold and warmed caches and confirm requests reach every worker. Reproduce #4680's 1T1 through 4T4 setup with 200 requests, 0.3 offered requests/s per thinker replica, async chunking, and Orchestrator monitoring before claiming its scaling bottleneck is resolved. Record per-worker CPU/request counts, queue age/high-water marks, GPU utilization, first-output latency, and end-to-end latency.
Generic input admission/output waiting (#4855, #4177), media encoding offload (#6579, #7095, #7096), and diffusion video transport/overlap (#6477, #6872) remain separate work. Their benchmark results are not attributed to this PR.
Validation
5d9c4c86: Qwen3-TTS CustomVoice, API counts 1 and 2, one warmup plus 16 requests per mode at concurrency 4; all 34 responses were valid nonempty 24 kHz mono WAV. Both multi-API workers served requests, each stage had one EngineCore process, and owned processes/port were cleaned up. All 12 real HTTP checks passed for single-worker video access and multi-worker video/voice-delete/sleep/wakeup protection. Used the same reduced-capacity deployment overlay; this validates functionality, not throughput or video generation.qwen3_tts_high_concurrency.yamloverlay (15% GPU memory budget, max model length 2048, max sequences 4), shared Talker/Code2Wav pipeline, API counts 1 and 2, one warmup plus 16 requests per mode, concurrency 4, fresh HTTP connections. All requests returned valid 24 kHz mono WAV with nonempty audio. Both API workers served requests in multi-API mode; logs confirmed one EngineCore process per stage. Owned processes exited and the test port closed. GPU placement followed shared-host availability, so this is functionality validation, not a performance A/B.mypy-3.10hook reports 39 existing diagnostics, reproduced on clean main with no new diagnostics after normalizing line shifts (same 20 PR files on branch and main). The five files changed in the launch-lifecycle refactor pass all applicable hooks, including mypy, without skips. The repository CI already skips that hook; this does not claim a clean full type check.The CPU regression command, from the repository root with matching vLLM/pytest dependencies installed, is:
PYTHONPATH=. HF_HUB_OFFLINE=1 TRANSFORMERS_OFFLINE=1 python -m pytest -q \ tests/engine/test_async_omni_engine_stage_init.py \ tests/engine/test_stage_device_layout.py \ tests/engine/test_parallel_stage_init.py \ tests/engine/test_stage_engine_startup_cache_env.py \ tests/entrypoints/test_serve.py \ tests/entrypoints/openai_api/test_api_server_guards.py \ tests/entrypoints/test_utils.py -m 'core_model and cpu'Shared EngineCore startup
StageRuntime.launch_stage_engines(num_api_servers=1)is the common launch entry for local EngineCore clients and the multi-API parent. The single-API device-group scheduler passes prepared replica plans into this entry; it no longer maintains its own spawning, device-lock, readiness, and rollback implementation.initialize()retains client/output-processor/stage-pool initialization. The default frontend remains in its existing process.Single-client launches complete readiness before local attachment and transfer engine ownership to the client. Multi-client launches defer readiness until API workers attach; the serving parent retains ownership. Both use
StageEngineLaunch, scoped spawn environments, shared initialization-lock handling, and startup/attachment rollback. The multi-API parent now also forwardsparallel_stage_initand performs admission before spawning; child phase locks are used in that mode.Regression coverage includes one/two clients on one/two stages, readiness ordering, engine ownership, startup and attachment failures, lock release, and parallel admission. Diffusion initialization and remote attachment keep their backend-specific paths; the existing multi-API restrictions remain in effect.
The shared-launch review request is implemented. Future support for the currently restricted backend/distributed modes remains open. The PR is ready for review. The existing
readylabel was refreshed after the September 11 GPU rerun to trigger CUDA CI on the latest head; the PR has not been merged.