test(vllm): multi-worker embedding routing tests (DIS-2098) - #9765
Conversation
d5a2053 to
c12c8be
Compare
34ccf25 to
7f1062c
Compare
c12c8be to
295335e
Compare
7f1062c to
a1dcfa6
Compare
a1dcfa6 to
4e92b32
Compare
4e92b32 to
63c58eb
Compare
63c58eb to
e495de7
Compare
8ad9834 to
3d974aa
Compare
e495de7 to
39be145
Compare
39be145 to
3f0a3d2
Compare
3f0a3d2 to
dd0260f
Compare
3d974aa to
5be8483
Compare
dd0260f to
66e5577
Compare
5be8483 to
894e159
Compare
WalkthroughThis PR adds complete multi-worker embedding infrastructure for vLLM, including a launcher script that orchestrates a routing frontend and two GPU-pinned workers, a new test payload that validates multi-worker dispatch via Prometheus metrics, unit tests for the payload logic, and end-to-end tests for load balancing and model-name-based routing. ChangesMulti-Worker Embedding Test Infrastructure
🎯 3 (Moderate) | ⏱️ ~25 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: 1
🤖 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 `@examples/backends/vllm/launch/agg_embed_multiworker.sh`:
- Around line 54-56: The script uses SYSTEM_PORT1="${DYN_SYSTEM_PORT1:-8081}"
which ignores a caller-supplied DYN_SYSTEM_PORT; change the assignment for
SYSTEM_PORT1 to prefer DYN_SYSTEM_PORT and fall back to DYN_SYSTEM_PORT1 and
then 8081 so callers can set a single DYN_SYSTEM_PORT for worker-1 while
preserving existing DYN_SYSTEM_PORT1 behavior; leave SYSTEM_PORT2 as-is
(DYN_SYSTEM_PORT2) so multi-worker distinct ports still work.
🪄 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: fc557b45-1e6f-4262-8a2a-04d75baec0a5
📒 Files selected for processing (4)
examples/backends/vllm/launch/agg_embed_multiworker.shtests/serve/test_vllm.pytests/utils/payloads.pytests/utils/test_embedding_dispatch_payload.py
Sibling launch scripts (``agg_embed.sh``, ``agg.sh``, etc.) use ``DYN_SYSTEM_PORT`` as the single-worker convention; multi-worker scripts use the numbered ``DYN_SYSTEM_PORT1`` / ``DYN_SYSTEM_PORT2``. Make the multiworker embedding launcher accept either form for worker 1 by falling through ``DYN_SYSTEM_PORT1 -> DYN_SYSTEM_PORT -> 8081`` so callers that only set the non-numbered env var still drive worker 1's port. ``SYSTEM_PORT2`` stays numbered-only -- there's no single-worker equivalent. Per CodeRabbit feedback on PR #9765. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
biswapanda
left a comment
There was a problem hiding this comment.
lgtm with a minor comment
|
we'd add a k8s based deploy test for a known embedding model + deterministic/greedy sampling |
biswapanda
left a comment
There was a problem hiding this comment.
Let's add a k8s based end-to-end deploy test for a known embedding model + deterministic/greedy sampling.
It can be this same PR or a follow up PR.
|
@biswapanda thanks for the review. The k8s e2e is actually already done — see DIS-2138, where I ran the 9-test matrix against a real Qwen3-Embedding-0.6B deployed on dynamo-aks-dev (arm64 GB200) and posted full results (8/9 pass; the 9th is a pre-existing Rust frontend HTTP-500-vs-400 mapping issue unrelated to this PR). Taking your wording "this same PR or a follow up PR" — turning that one-shot manual run into a recurring CI job is real infra work (new K8s test-runner harness, namespace lifecycle, image-build coordination). I've filed DIS-2153 as a follow-up so this PR stays scoped to its title and we don't bundle CI infra here. Happy to discuss whether DIS-2153 should be done before merging this — but my reading is the in-process |
Thanks @tzulingk. yes k8s tests are non-blocker and can be a follow up. |
Adds end-to-end + unit coverage for the two routing properties of the
embedding worker pool:
1. Same-model load balancing — two workers serving the same embedding
model weighted-randomly absorb a burst of /v1/embeddings traffic.
The test asserts both workers' dynamo_component_requests_total
counters move > 0 over a 20-request burst.
2. Multi-model dispatch — two workers serving two different models.
Each model's burst must land only on its registered worker; the
wrong-model worker observes zero delta. Verifies the name-keyed
get_embeddings_engine(model) -> select_worker_set_with() path
specifically for embedding traffic. (Chat-completions already
exercises the same router code; this is explicit embedding-side
coverage.)
Per-burst deltas, not absolute counts
=====================================
In the multi-model test, burst A leaves worker A's counter > 0;
burst B's check would falsely pass if it only required "B's worker
has > 0 traffic". The new payload snapshots /metrics at the start of
each burst and asserts the delta through the end of the burst, which
makes "wrong-model traffic stayed out of worker A during burst B"
actually expressible.
Pieces
======
- examples/backends/vllm/launch/agg_embed_multiworker.sh: frontend +
N embedding workers (one per GPU, distinct DYN_SYSTEM_PORT each).
Takes two model args. Mirrors agg_embed.sh's pooler config.
- tests/utils/payloads.py: new EmbeddingMultiWorkerDispatchPayload.
Takes a list of system ports (resolved from
DefaultPort.SYSTEM{1,2} placeholders by the harness) and a set of
INDICES (port mapping happens at runtime, so absolute port numbers
can't be compared). Snapshots on first request, deltas on last.
- tests/serve/test_vllm.py: two pytest.mark.gpu_2 / pre_merge test
functions: test_embedding_multi_worker_same_model_load_balance,
test_embedding_multi_worker_multi_model_dispatch.
- tests/utils/test_embedding_dispatch_payload.py: unit tests for the
payload's baseline+delta logic. Monkeypatches requests.get with a
canned per-port /metrics sequence; no GPU needed. Covers the six
branches that matter: load-balance pass/fail, dispatch pass/fail,
min-total-delta floor, baseline-snapshot timing.
Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…i-worker embed tests The previous fix used ``health_check_workers=True`` to wait for both embedding workers before sending requests, but the Dynamo ``/health`` endpoint on an embedding worker stays at HTTP 503 because the embedding worker handler in PR #9713 doesn't call ``set_health_status(True)`` after registering its model. The harness polled ``/health`` for the full pytest-timeout (420s) and the test failed with "Timeout (>420.0s) from pytest-timeout". Switch to ``delayed_start=90`` on both multi-worker configs. 90s is comfortably above the observed ~30s per-worker load time and gives both workers room to register their model names with the frontend before the first burst fires. Long-term fix is for the embedding worker handler to flip the readiness flag once the model is registered — tracked separately. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Dynamo enforces one model per endpoint path
(namespace/component/endpoint). The previous multi-worker script let
both workers default to the same ``dynamo/backend/generate`` endpoint;
that works for the same-model load-balance test (both workers register
the same model name) but the multi-model dispatch test fails with:
Failed to serve embedding worker endpoint: Cannot register model
'Qwen/Qwen3-Embedding-0.6B' on endpoint 'dynamo/backend/generate':
a different model 'BAAI/bge-small-en-v1.5' is already registered
there
Give worker 1 ``--endpoint dynamo/embed-worker-1/generate`` and worker
2 ``--endpoint dynamo/embed-worker-2/generate``. The frontend's
name-keyed ``get_embeddings_engine(model)`` dispatch still routes
correctly by the ``model`` field regardless of which component path
hosts each model.
Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Dynamo expects namespace.component.endpoint (dot-separated); the
previous commit used slashes and the worker died at startup with:
ValueError: Invalid endpoint format:
'dynamo/embed-worker-1/generate'. Expected
'dyn://namespace.component.endpoint' or
'namespace.component.endpoint'.
Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
The multi-worker embedding tests have been flaking on multi-GPU CI
because there is no real readiness signal -- ``delayed_start=90`` is
just a fixed sleep, and the per-worker cold-load time on the AMD GPU
pool varies enough that one worker can still be loading when the test
starts firing requests. The router only sees workers that have
finished registering, so every burst request lands on whichever
worker happened to be ready first. The recent failure mode is the
clear signature: ``Expected worker indices with delta {0, 1}, got
{1}``, with worker 0's per-port counter sitting at zero across both
baseline AND burst snapshots -- the test fixture never reached
worker 0 either, because worker 0 was not yet in the registry.
Fix
---
Mirror the chat worker pattern: give the embedding worker a real
canary payload that the runtime probes ``serve_endpoint``'s
registered handler with. Once the probe runs a successful embedding
forward pass, the runtime flips ``/health`` to 200, and the test
harness's ``health_check_workers=True`` path can be used as the
real readiness gate instead of a wall-clock sleep.
- ``components/src/dynamo/vllm/health_check.py`` -- new
``VllmEmbeddingHealthCheckPayload`` returns
``{"model": <served_model_name>, "input": "probe",
"_HEALTH_CHECK": True}``. Mirrors ``VllmHealthCheckPayload`` /
``VllmPrefillHealthCheckPayload`` shape so the runtime treats it
the same way (canary registration, periodic probe, /health gate).
- ``components/src/dynamo/vllm/worker_factory.py::_create_embedding_worker``
-- builds the payload and passes it through
``serve_endpoint(health_check_payload=...)`` alongside the
``register_vllm_model`` task. The chat-shaped payload would be
rejected by ``EmbeddingWorkerHandler.generate`` ("missing required
'input' field"); embedding-shaped payload goes through cleanly and
runs a real pooling pass on a tiny input.
- ``tests/serve/test_vllm.py`` -- ``test_embedding_multi_worker_same_model_load_balance``
and ``test_embedding_multi_worker_multi_model_dispatch`` both
switch from ``delayed_start=90`` to ``health_check_workers=True``.
The framework now polls each worker's ``/health`` until it returns
200 before firing the warmup or burst payloads. No more wall-clock
guessing.
Probe cost
----------
The canary runs one real ``engine.encode("probe")`` per probe cycle
on each worker. For Qwen3-Embedding-0.6B that is a single ~10ms
pooling pass; for BGE-small-en-v1.5 (33M params) the pass is
effectively free. Trades that minor steady-state GPU cycle for a
deterministic readiness signal.
Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
The multi-worker embedding tests have been flaking on multi-GPU CI with all burst requests landing on a single worker, even when both workers are alive and reachable. After the canary readiness fix ensured both workers reach ``/health = 200`` deterministically, the underlying cause surfaced cleanly: the previous "unique endpoint" workaround did not give the workers truly independent routing. Root cause ---------- The frontend's discovery layer keys ``Model.worker_sets`` by ``(namespace, model_type)`` (see ``worker_set_key`` in ``lib/llm/src/discovery/watcher.rs``) -- the endpoint component is NOT part of the key. ``add_worker_set`` on that key is an insert-overwrite (``DashMap::insert``), so two workers sharing a namespace -- no matter how their endpoint paths differ -- both hash to the same ``ws_key`` and the second registration silently replaces the first ``WorkerSet`` (along with its push_router). The first worker stays alive on its system port and the canary keeps it "healthy," but it is orphaned from the frontend's ``select_worker_set_with`` selector: 100% of routed traffic lands on whichever worker registered last. The same collision was the ORIGINAL symptom that drove the unique- endpoint workaround -- "Cannot register model 'X' on endpoint Y: a different model 'Z' is already registered there." Splitting the endpoint name only hid that symptom; the underlying WorkerSet collision was still there. Fix --- Give each worker its own Dynamo namespace (``embed-worker-1.vllm.generate`` and ``embed-worker-2.vllm.generate``). Different namespaces produce different ``ws_key``s, both WorkerSets coexist in ``Model.worker_sets``, and ``select_worker_set_with`` does its weighted-random fan-out across them as designed -- which is the routing path the test was authored to verify. The frontend discovers models across namespaces without configuration; ``python3 -m dynamo.frontend`` watches all namespaces by default, so no frontend change is needed. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Sibling launch scripts (``agg_embed.sh``, ``agg.sh``, etc.) use ``DYN_SYSTEM_PORT`` as the single-worker convention; multi-worker scripts use the numbered ``DYN_SYSTEM_PORT1`` / ``DYN_SYSTEM_PORT2``. Make the multiworker embedding launcher accept either form for worker 1 by falling through ``DYN_SYSTEM_PORT1 -> DYN_SYSTEM_PORT -> 8081`` so callers that only set the non-numbered env var still drive worker 1's port. ``SYSTEM_PORT2`` stays numbered-only -- there's no single-worker equivalent. Per CodeRabbit feedback on PR #9765. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…unit test Three follow-ups from @tmonty12's review on PR #9765: 1. ``components/src/dynamo/vllm/worker_factory.py`` -- drop the inline comment block above the ``VllmEmbeddingHealthCheckPayload`` construction. It was duplicating the class docstring almost verbatim. The construction line is now self-explanatory; readers who want the "why" go straight to the class. 2. ``tests/serve/test_vllm.py`` -- set ``DYN_HEALTH_CHECK_ENABLED=true`` on both multi-worker embedding test configs. Without that env var the runtime's canary never actually runs and ``/health`` returns 200 immediately after endpoint registration, so ``health_check_workers=True`` gates on a constant-true signal and we race startup just like the old ``delayed_start`` path. The whole point of the canary readiness fix is to gate on "engine produced an embedding" -- both knobs (flag + embedding-shaped probe payload) must be on. 3. ``components/src/dynamo/vllm/tests/test_vllm_health_check_payloads.py`` -- add ``VllmEmbeddingHealthCheckPayload`` to the marker / env-override parametrize and a new ``test_embedding_payload_shape_matches_handler_contract`` regression pin that asserts the payload produces ``{model, input}`` (the shape ``EmbeddingWorkerHandler.generate`` accepts) and explicitly NOT ``token_ids`` / ``sampling_options`` / ``stop_conditions`` (the chat shape that would trip the embedding handler's input validation). The parametrize list switches from ``PAYLOAD_CLASSES`` to ``PAYLOAD_FACTORIES`` because ``VllmEmbeddingHealthCheckPayload`` requires a ``model_name`` argument; the other three subclasses stay no-arg-constructable so they're passed directly. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
…ad call CI's pre-commit black 23.1.0 wants the constructor call split across multiple lines; my local black 26 considered the one-liner fine. Matching CI to unblock the pre-commit job on PR #9765. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Per @biswapanda's review on PR #9765: 1. ``health_check.py`` -- ``model_name`` is now ``Optional[str] = None``. When the caller doesn't pass a model name, the ``model`` key is omitted from the payload entirely and ``EmbeddingWorkerHandler.generate`` falls back to ``config.served_model_name`` (which the handler already does on the missing-field path). Existing callers that pass a name still get the same self-describing log behavior. 2. ``test_vllm_health_check_payloads.py``: - Switch ``payload[key]`` lookups to ``payload.get(key)`` so a missing key produces a clean ``None != expected`` assertion instead of a ``KeyError`` before the assertion runs. - The embedding factory entry can now use the class directly (no lambda) since the constructor takes no required args. - Add a sibling regression test (``test_embedding_payload_omits_model_when_no_name``) pinning the no-arg behavior: the ``model`` key is absent and the rest of the shape is intact. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com>
Overview:
Adds end-to-end + unit coverage for the two routing properties the embedding worker pool exposes today: load-balancing across multiple workers serving the same model, and name-keyed dispatch across workers serving different models. Both paths run through the same
model_manager→select_worker_set_with()machinery that already serves chat traffic; the work here is explicit embedding-side exercise of that machinery against/v1/embeddings.Linear: https://linear.app/nvidia/issue/DIS-2098
Architecture
The frontend dispatches each
/v1/embeddingsrequest through one lookup:A worker registers itself with the runtime under
--endpoint <namespace>.<component>.<endpoint>. The runtime indexes the (worker, model) pair by model name, so the frontend never needs to know endpoint paths — it asks "who serves model M?" and gets back the set of workers that registered M, then picks one.Two consequences shape the test setup:
One model per endpoint path. The runtime rejects a second worker that tries to register a different model on the same
namespace.component.endpoint. So the multi-worker launch script gives each worker its own unique--endpoint(here:dynamo.embed-worker-1.generateanddynamo.embed-worker-2.generate). They land in the same model registry from the frontend's perspective, but the runtime treats them as independent worker instances.Verification is structural, not behavioral. Each worker exposes
/metricson its ownDYN_SYSTEM_PORT. The test scrapesdynamo_component_requests_totalper worker — that counter increments on the worker that actually served the request, so the dispatch decision is observable from outside the frontend.What's checked
Same-model load balancing — two workers register
Qwen3-Embedding-0.6B(under different endpoint paths). The frontend's worker-set lookup returns both;select_worker_set_with()weighted-randomly picks one per request. The test sends a 20-request burst and asserts both workers'dynamo_component_requests_totalcounters moved.Multi-model dispatch — worker A registers
Qwen3-Embedding-0.6B, worker B registersBAAI/bge-small-en-v1.5. The frontend's name-keyedget_embeddings_engine(model)lookup must return only the matching worker. The test sends a burst withmodel="Qwen3...", snapshots per-worker counters, sends another burst withmodel="BAAI...", snapshots again. Burst A must increment worker A's counter only; burst B must increment worker B's counter only.Why per-burst deltas, not absolute counts
The multi-model test sends two bursts in sequence:
Each worker's
dynamo_component_requests_totalcounter increments on every request it serves. The interesting question to verify is: did burst B traffic stay out of worker A?A naive check on absolute totals cannot see a dispatch leak. Suppose dispatch were broken and burst B leaked 3 requests onto worker A:
An "is worker B's counter > 0?" assertion still passes (10 > 0), and an "is worker A's counter == 0?" assertion fails for the wrong reason — worker A is at 13 because of burst A, not because of the burst B leak. State from burst A dominates anything you can read from absolute totals.
The fix is to snapshot each worker's counter at the start of each burst and assert on the delta through the end:
Now "during burst B, only worker B's delta is positive" actually has bite:
delta[A] = 3is a real signal of dispatch breakage, not a remnant of burst A. "Wrong-model traffic stayed out of worker A during burst B" is a claim about a specific time window, so it needs two snapshots (start and end) to be testable at all.Why indices, not absolute ports
The harness assigns dynamic ports to each test run, so we can't hard-code
DYN_SYSTEM_PORT1 = 8081in expectations. The payload acceptsexpected_worker_indices_with_delta: set[int]where0 = first worker in system_ports list,1 = second. Port mapping (DefaultPort.SYSTEM{1,2}→ actual dynamic port) happens inside the harness before the payload runs.Pieces
examples/backends/vllm/launch/agg_embed_multiworker.sh— 1 frontend + 2 embedding workers (one per GPU). Each worker gets its ownDYN_SYSTEM_PORTand its own unique--endpoint <namespace>.<component>.<endpoint>so registration doesn't collide. Pooler config mirrorsagg_embed.sh(--runner pooling, MEAN pooler, fp32).tests/utils/payloads.py— newEmbeddingMultiWorkerDispatchPayload. Takes a list of system ports + a set of worker indices that should observe non-zero delta. Snapshots/metricson the first request of a burst, deltas against the snapshot on the last.tests/serve/test_vllm.py— twopytest.mark.gpu_2/pre_mergetest functions:test_embedding_multi_worker_same_model_load_balance,test_embedding_multi_worker_multi_model_dispatch.tests/utils/test_embedding_dispatch_payload.py— six unit tests for the baseline+delta logic. Monkeypatchesrequests.getwith a canned per-port/metricssequence so the payload runs without a GPU.Where should the reviewer start?
tests/utils/payloads.pyEmbeddingMultiWorkerDispatchPayload— the diff semantics + index-based assertion are the load-bearing pieces of the test surface.tests/utils/test_embedding_dispatch_payload.py— six small tests that exercise the payload in isolation, no GPU.examples/backends/vllm/launch/agg_embed_multiworker.sh— confirm the per-worker--endpoint+DYN_SYSTEM_PORTwiring matches what the harness expects.tests/serve/test_vllm.py— the two new test functions, modeled ontest_lora_aggregated_routerwhich already uses the 2-system-ports + 2-GPU pattern.Summary by CodeRabbit
New Features
Tests