Repository navigation
Conversation
|
👋 Hi agupta01! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: ai-dynamo/dynamo/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe request path now validates and forwards Changesvllm_xargs forwarding
Completion logprobs limit
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to No actionable issue is established for the changed request options or logprobs limit; the PR is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 4 files. (1 skipped: 1 too large.) Comment |
vLLM's OpenAI server accepts `vllm_xargs` and passes it to `SamplingParams.extra_args`, which custom extensions and model-specific request options read (for example DiffusionGemma structured reads: `diffusion_seed_canvas`, `diffusion_max_steps`, `diffusion_read_only`). Dynamo's frontend rejected the field as unsupported, or dropped it with DYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDS. Accept `vllm_xargs` as a passthrough extra field, validated against vLLM's schema (object of string/number/boolean values or arrays of them), forward it in extra_args.sampling_options, and merge it into SamplingParams.extra_args in the vLLM worker. `kv_transfer_params` and `ec_transfer_params` are rejected by the frontend and dropped by the worker, so clients cannot inject connector inputs through this path. Token-input vLLM workers advertise a `vllm_xargs` runtime capability. Chat and completions requests carrying the field select only WorkerSets that advertise it; when a model has none (other backends, text-input or older vLLM workers), the frontend returns 400 instead of silently dropping the field. Signed-off-by: Arunav Gupta <arunav.gupta@coreweave.com>
`/v1/completions` capped `logprobs` at the legacy OpenAI limit of 5 while chat accepts `top_logprobs` up to 20 and both map to the same backend output option. vLLM clients that read top-k candidates from token-id prompts (for example DiffusionGemma structured reads with continuation prompts) request top-20. Align the completions limit with chat and vLLM's default `--max-logprobs`. Signed-off-by: Arunav Gupta <arunav.gupta@coreweave.com>
52e41e7 to
34db9e1
Compare
| self.select_worker_set_with(|ws| ws.chat_engine.clone().map(|e| (e, ws.parsing_options()))) | ||
| .ok_or_else(|| self.engine_error(self.has_chat_engine())) | ||
| self.select_worker_set_with(|ws| { | ||
| supports_capability(ws, required_capability) |
There was a problem hiding this comment.
Enforce capability support on every destination worker
Summary: This checks the WorkerSet representative card, not every routable instance. vllm_xargs is excluded from the MDC checksum/cohort fingerprint, so upgraded and older replicas can share a WorkerSet during a rolling upgrade. A capable representative can therefore admit a request that reaches an older worker and silently loses its options; an older representative can reject requests despite available capable replicas. The selected decode set's prefill destinations are not capability-filtered either.
Example: Register an upgraded replica advertising vllm_xargs and an older replica at the same endpoint/model/role. A completion with vllm_xargs: {"diffusion_read_only": true} passes this check when the upgraded replica is representative, but can route to the older replica, whose sampling builder ignores the field.
Actionable fix: Carry the requirement through per-instance routing/admission, including prefill, or conservatively reject these requests until all eligible destinations support it. Preserve ordinary requests during mixed-version operation rather than treating the capability difference as an incompatible model revision. Add discovery/routing coverage for mixed capabilities inside one WorkerSet, both registration orders, worker removal, and upgraded decode with older prefill.
|
/ok to test de05f4c |
⏹️ Dynamo PR CI cancelled — run 37683231183 (attempt 1) on
|
| Other | Jobs |
|---|---|
| planner | ⏹️ 2 ✅ 3 |
Failure details
No jobs failed. 2 jobs were cancelled: planner / Compliance cpu, arm64 and planner / Compliance cpu, amd64 both hit the 15m job timeout in step Compliance scan (annotation: "The job has exceeded the maximum execution time of 15m0s"), so no compliance CSV was uploaded. These were not fail-fast or manual/concurrency cancels. The planner build and CPU tests passed and all gate checks are green. Rerunning the failed jobs is likely enough.
For agents
{"pr": 15281, "run_id": 37683231183, "run_attempt": 1, "head_sha": "7c6ad15dc9a838d7d8489c214e125a46b9f39c09",
"failures": [],
"cancelled": [{"job": "planner / Compliance cpu, arm64", "job_id": 113010744938, "cancelled_step": "Compliance scan", "signature": "job exceeded maximum execution time of 15m0s", "log_cmd": "gh run view --job 113010744938 -R ai-dynamo/dynamo --log"},
{"job": "planner / Compliance cpu, amd64", "job_id": 113010745080, "cancelled_step": "Compliance scan", "signature": "job exceeded maximum execution time of 15m0s", "log_cmd": "gh run view --job 113010745080 -R ai-dynamo/dynamo --log"}]}Posted automatically by Devin for run 37683231183. Updated on every full-CI run of this PR.
The vllm_xargs capability is excluded from the MDC checksum, so upgraded and older workers share one WorkerSet during a rolling upgrade. Checking only the representative card admitted requests that could reach an older worker and lose their options, and rejected requests a capable set could serve. Track each committed member's request capabilities per WorkerSet. Engine selection admits a vllm_xargs request only when every member, and every prefill peer of a decode set, advertises the capability: partial support is 503, none is 400. Decode and prefill dispatch refuse any worker not described as capable, which covers a worker admitted after selection. Signed-off-by: Arunav Gupta <arunav.gupta@coreweave.com>
|
/ok to test 7c6ad15 |
…-passthrough Signed-off-by: Arunav Gupta <arunav.gupta@coreweave.com> # Conflicts: # components/src/dynamo/vllm/handlers.py
Overview:
vLLM's OpenAI server accepts
vllm_xargsand passes it toSamplingParams.extra_args. Custom extensions and model-specific request options read that field. One example is DiffusionGemma's structured "read" requests, added in vllm-project/vllm#57250. They senddiffusion_seed_canvas,diffusion_max_steps,diffusion_read_onlyand similar fields. Dynamo's OpenAI frontend rejectsvllm_xargswithUnsupported parameter(s). WithDYN_IGNORE_OPENAI_FE_UNSUPPORTED_FIELDSset, it silently drops the field instead. Either way, these workloads can't run behind the Dynamo frontend and KV router.This PR forwards
vllm_xargsthe same waylogprob_token_idsand the other extra-body sampling fields are already forwarded. It also raises the/v1/completionslogprobscap from 5 to 20, the chattop_logprobslimit. Those structured reads also call/v1/completionswith token-id prompts and ask for the top 20 candidates.Details:
Commit 1:
feat(frontend): forward vllm_xargs to vLLM SamplingParams.extra_argslib/llm/src/protocols/openai/validate.rs:vllm_xargsis added toPASSTHROUGH_EXTRA_FIELDS. It is validated against vLLM's schema: an object whose values are strings, numbers or booleans, or arrays of those. Booleans are accepted because vLLM's pydantic model coerces them.kv_transfer_paramsandec_transfer_paramsare rejected. vLLM fills those keys from dedicated request fields, and Dynamo only forwards router-generated connector inputs.lib/llm/src/preprocessor.rs:vllm_xargsis forwarded inextra_args.sampling_options, next tologprob_token_ids.components/src/dynamo/vllm/handlers.py:build_sampling_paramspopsvllm_xargsbefore the genericsetattrloop. It then merges the field over any defaultextra_args. The worker drops the two connector keys as well, as a second line of defense, so the router'skv_hinthandling is unchanged.vllm_xargsruntime capability (VLLM_XARGS_CAPABILITYinruntime_config.rs; published bydynamo.vllmfor base and LoRA model cards). Requests carryingvllm_xargsreach only workers that advertise it (see the per-worker enforcement commit below). When no worker serving the model advertises it (SGLang, TensorRT-LLM, text-input or older vLLM workers), the newModelManagerError::CapabilityUnsupportedbecomes a 400 instead of the field being dropped silently. The text-input path is covered too, since it forwards the raw request and drops passthrough fields.Commit 2:
feat(frontend): allow completions logprobs up to 20MAX_LOGPROBSchanges from 5 to 20, matchingMAX_TOP_LOGPROBSand vLLM's default--max-logprobs.top_logprobs, which maps to the sameoutput_options.logprobs. So this doesn't add any new backend-facing range.Commit 3:
fix(frontend): require vllm_xargs support on every destination workerThe capability is not part of the MDC checksum, so upgraded and older workers share a WorkerSet during a rolling upgrade. Checking the WorkerSet's representative card is therefore not enough, so support is enforced per worker:
MemberCapabilities(lib/llm/src/discovery/worker_set.rs) records which committed members of a WorkerSet advertise each request capability.ModelManager::{commit,replace}_discovery_groupupdate it. The controller calls these before it admits a newcomer and after it withdraws a departed worker, so every admitted worker is described.Model::get_{chat,completions}_engine_with_parsing) admits avllm_xargsrequest only when every member advertises it. For a decode WorkerSet, every prefill WorkerSet in its namespace must as well. Partial support, as during a rolling upgrade, is a 503; no support anywhere is a 400. Ordinary requests keep using mixed-version WorkerSets.RoutingHosts check the final destination worker on every path (KV, builtin modes, Direct, LoRA, transport fallback). A worker that is not described as capable is refused withUnavailable, which covers a worker admitted between engine selection and dispatch.Requests without
vllm_xargsare unaffected on every backend.Where should the reviewer start?
validate_vllm_xargsinlib/llm/src/protocols/openai/validate.rs: the accepted value shapes and the reserved keys.MemberCapabilitiesinlib/llm/src/discovery/worker_set.rsanddescribe_membersinmodel_manager.rs: per-member capability tracking and when it is updated relative to admission.CapabilityFilter/capability_engine_errorinlib/llm/src/discovery/model.rs: WorkerSet selection, including prefill peers, and the 400 vs 503 decision.RoutingHost::ensure_worker_capabilityand its call sites inrouting_host/builtin.rsandrouting_host/kv.rs: the per-destination check at dispatch.build_sampling_paramsincomponents/src/dynamo/vllm/handlers.py: how the merge interacts withdefault_sampling_params["extra_args"]and with the laterkv_hint/kv_transfer_paramshandling.Validation
Unit and lint checks ran on this branch, on top of upstream
main, insidevllm/vllm-openai:nightly-e9757321527ca1ecd514c07c1418dd2c53da3d19(vLLM0.29.1rc1.dev573, which includes vllm-project/vllm#57250):Commit 3 was validated on macOS (
--no-default-features, per the repo guidance fordynamo-llm):New regression tests:
vllm_xargs_requires_every_worker_set_member(mixed members in both registration orders, worker removal and rejoin, 503 vs 400),vllm_xargs_requires_capable_prefill_peers(upgraded decode with older prefill), and{kv,builtin}_dispatch_requires_worker_capability(dispatch to an admitted worker without the capability is refused while ordinary requests still reach it). All four fail with the enforcement disabled.End-to-end, the same change backported to v1.5.0 has been serving DiffusionGemma NVFP4 behind
dynamo.frontend --router-mode kv --router-kv-events. The setup is 8dynamo.vllmworkers, each on its own RTX PRO 6000 Blackwell GPU, on Kubernetes through the Dynamo operator:vllm_xargs,logprob_token_idsandreturn_tokens_as_token_idsreturned correct per-slot label distributions. The prompt Dynamo rendered from the chat template had the same token count as Hugging Face's rendering (206 tokens).Two other things came up during that integration. They are not changed in this PR, and I can open issues for them if useful:
temperature: 0.0) for diffusion models, whose sampler runs a fixed schedule. Workers never become healthy unlessDYN_HEALTH_CHECK_PAYLOADis overridden.ignore_eosinternally, but Dynamo's own EOS stop handling still truncates the emitted canvas. Clients have to sendignore_eos: trueexplicitly.Related Issues