chore(e2e): validation snapshot — combined #9830 + #9765 (do not merge) by tzulingk · Pull Request #10055 · ai-dynamo/dynamo

tzulingk · 2026-05-27T19:13:48Z

Purpose

Do not merge. This is a snapshot draft for end-to-end validation of the still-open embedding-stack PRs against a real vLLM embedding worker. The work belongs in the constituent PRs; this branch exists to make the combined stack easy to check out, build, and exercise as one image.

Constituent PRs

PR	Status	Branch	What it adds
#9887	✅ merged 2026-05-27	`feat/embedding-base64-e2e`	`encoding_format=base64` end-to-end (owned `Embedding`/`CreateEmbeddingResponse` in `lib/protocols`; postprocessor + Python handlers encode)
#9886	✅ merged 2026-05-27	`feat/vllm-per-worker-metric-gating`	vLLM: skip chat-shaped Prometheus collectors on embedding workers (`NoopStatLogger`)
#9830	🟡 open	`feat/per-worker-metric-gating`	SGLang side of the same metric gating
#9765	🟡 open	`feat/embedding-multi-model-test`	Multi-worker embedding routing tests + canary `/health` readiness (`VllmEmbeddingHealthCheckPayload`)

After #9886 and #9887 merged, this branch was rebuilt off the new main so the diff here only shows the still-open work in #9830 and #9765.

E2E validation

Full test results recorded in DIS-2138.

Image used (now superseded): nvcr.io/nvidian/dynamo-dev/tzulingk-vllm:embedding-stack-e2e (digest sha256:74118aa1334b5a642ff464c748bc731681abbbcab6a998db13a3e4a8c7e91b8c). Built from the previous 25-cherry-pick version of this branch (before feat(vllm): gate chat-shaped Prometheus collectors on embedding worker #9886 + feat(embeddings): honor OpenAI encoding_format=base64 end-to-end #9887 merged); the current branch represents the remaining un-merged delta from that image.
Cluster: dynamo-aks-dev (arm64 GB200, GKE).
Result: 9/9 functional tests pass (encoding_format=base64 round-trip, dimensions truncation + base64 byte-count, batch inputs, /health canary, chat-shaped Prometheus gauges correctly absent, invalid encoding_format → HTTP 400, etc.). One side-note logged in DIS-2138 about an unrelated pre-existing Rust frontend error-mapping bug (ValueError → HTTP 500 instead of 400) that does NOT come from any of these four PRs.

Rebuild instructions

If a coworker needs to rebuild this image in their own NVCR namespace:

git fetch origin
git checkout -b my-e2e origin/main

# #9830 (SGLang gating, 2 commits)
git cherry-pick -x e1b83e80b4 2029e201d0

# #9765 (multi-worker tests + canary, 9 commits)
git cherry-pick -x 7fc1a38ca2 cdd2ab2779 c85091bf68 051ecbbdf7 \\
                   f1a6263216 34b496c400 baf7777d5d e35a2d3430 8220851842

python3 container/render.py --framework vllm --target runtime \\
                            --platform linux/arm64 --output-short-filename
docker build -t <your-registry>/vllm-runtime:embedding-stack-e2e \\
             -f container/rendered.Dockerfile .
docker push <your-registry>/vllm-runtime:embedding-stack-e2e

Then deploy the self-contained Pod from DIS-2138 (single Pod runs nats-server + etcd + dynamo.frontend + dynamo.vllm; no DGD/operator needed for a one-shot e2e).

Review goes elsewhere

Don't review here — review the source PRs:

#9830 for SGLang gating
#9765 for multi-worker tests + canary readiness

🤖 Generated with Claude Code

Closes the first half of DIS-2107. The SGLang publisher previously registered chat-shaped collectors (SGLang's multiprocess ``sglang:*`` metrics, the Dynamo ``LLMBackendMetrics`` gauges ``total_blocks`` / ``gpu_cache_usage`` / ``model_load_time``, the ``DynamoSglangPublisher`` with its ZMQ scheduler pull + KV-event + FPM-relay wiring) on every worker, including embedding workers where they emit zeros forever. Operators monitoring an embedding fleet saw noise instead of signal. ``setup_sgl_metrics`` now short-circuits when ``config.dynamo_args.embedding_worker`` is set: no chat-shaped constructors run. It still returns a uniform ``(publisher_or_None, asyncio.Task, metrics_labels)`` shape so the embedding init path can keep the same ``await + cancel()`` cleanup sequence as the chat path. The corresponding ``dynamo_embedding_batch_size`` / ``dynamo_embedding_input_tokens`` collectors that DO belong on an embedding worker are registered separately by ``init_embedding.py`` via ``init_embedding_metrics`` (see PR #9753). Tests ----- ``test_sglang_publisher.py`` gains two new cases: - ``test_setup_sgl_metrics_skips_chat_pipeline_for_embedding_worker`` — patches ``setup_prometheus_registry``, ``register_engine_metrics_callback``, ``LLMBackendMetrics``, and ``DynamoSglangPublisher`` to ``raise`` if invoked. Setting ``embedding_worker=True`` must not trigger any of them, the return tuple must have ``publisher is None``, the task must be cancellable, and the model label still propagates. - ``test_setup_sgl_metrics_returns_publisher_for_chat_worker`` — sibling check that the chat path is unchanged: all chat-shaped constructors fire and a publisher comes back. vLLM publisher gating is intentionally NOT in this PR — vLLM has no embedding-worker code path on ``main`` today. The mirror change lands once the vLLM-side embedding worker (DIS-2092) merges and the vLLM-side embedding observation PR (DIS-2094 part 3) is on deck. Refs DIS-2107. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com> (cherry picked from commit e1b83e8)

Addresses three findings on PR #9830: 1. mypy: ``setup_sgl_metrics`` now declares ``Optional[DynamoSglangPublisher]`` in its return tuple, but the chat-worker call sites (init_llm.py x2, init_diffusion.py x1) pass the publisher to functions that expect the non-Optional type. Add ``assert publisher is not None`` at those three call sites — those code paths are reached only when ``embedding_worker=False``, so the assertion is correct by construction and narrows the type for mypy. 2. mypy: the embedding-worker no-op task used ``asyncio.create_task(asyncio.Event().wait())``. ``Event.wait()`` returns ``Literal[True]`` when set, which mypy then conflicts with the chat path's ``publisher.run() -> None`` return at the second ``create_task`` call. Wrap the wait in a local ``async _idle() -> None`` so both branches feed ``Coroutine[Any, Any, None]`` into ``create_task``. 3. Repository policy: scrub internal Linear ticket IDs from source. Removed ``DIS-2094`` from the publisher docstring and ``DIS-2107`` from the test-file section header. 4. Black: re-format the two files mentioned by pre-commit. No behavioral changes. Signed-off-by: Tzu-Ling <tzulingk@nvidia.com> (cherry picked from commit 2029e20)

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> (cherry picked from commit 7fc1a38)

…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> (cherry picked from commit cdd2ab2)

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> (cherry picked from commit c85091b)

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> (cherry picked from commit 051ecbb)

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> (cherry picked from commit f1a6263)

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> (cherry picked from commit 34b496c)

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> (cherry picked from commit baf7777)

@tmonty12

…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> (cherry picked from commit e35a2d3)

…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> (cherry picked from commit 8220851)

@biswapanda

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> (cherry picked from commit e9e8da5)

github-actions · 2026-06-28T10:17:28Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions · 2026-07-05T10:14:56Z

This PR has been closed due to inactivity. If you believe this PR is still relevant, please feel free to reopen it with additional context or information.

tzulingk added 11 commits May 27, 2026 14:13

pull-request-size Bot added the size/XXL label May 27, 2026

github-actions Bot added backend::vllm Relates to the vllm backend backend::sglang Relates to the sglang backend labels May 27, 2026

tzulingk changed the title ~~DRAFT: e2e validation snapshot — combined #9830 + #9765 (do not merge)~~ chore(e2e): validation snapshot — combined #9830 + #9765 (do not merge) May 27, 2026

github-actions Bot added the chore label May 27, 2026

copy-pr-bot Bot temporarily deployed to GITLAB May 29, 2026 00:46 Inactive

copy-pr-bot Bot temporarily deployed to GITLAB May 29, 2026 01:49 Inactive

github-actions Bot added the Stale label Jun 28, 2026

github-actions Bot closed this Jul 5, 2026

github-actions Bot deleted the feat/embedding-stack-e2e branch July 5, 2026 10:14

Navigation Menu

Uh oh!

chore(e2e): validation snapshot — combined #9830 + #9765 (do not merge) - #10055

chore(e2e): validation snapshot — combined #9830 + #9765 (do not merge)#10055
tzulingk wants to merge 12 commits into
mainfrom
feat/embedding-stack-e2e

tzulingk commented May 27, 2026

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

tzulingk commented May 27, 2026

Purpose

Constituent PRs

E2E validation

Rebuild instructions

Review goes elsewhere

Uh oh!

github-actions Bot commented Jun 28, 2026

Uh oh!

github-actions Bot commented Jul 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant