Repository navigation
Conversation
…rocessing
vLLM reasoning parsers expose adjust_request() so they can declare the sampling
params they need. Parsers that split thinking from the answer on text markers
require `skip_special_tokens=False`, otherwise the markers are stripped before
the parser ever sees them and the split silently does not happen.
`_prepare_request` calls the TOOL parser's adjust_request but there is no
equivalent for the reasoning parser -- it does not even receive
`reasoning_parser_class`, since the parser is constructed later, in
StreamingPostProcessor, long after the sampling request is finalised.
Net effect: `reasoning_content` comes back empty unless the CLIENT sends
`skip_special_tokens: false`. Ordinary OpenAI-compatible clients do not, so on
such models the reasoning channel silently does nothing for normal traffic. The
tool path is additionally gated on `tool_choice != "none"`, so a plain
tools-free chat request gets no adjust_request call from either parser.
Mirror the tool-parser path: construct the reasoning parser during request
preparation and let it adjust the request.
The gating matters and is deliberately identical to StreamingPostProcessor's own
parser construction (`reasoning_parser_class and not thinking_disabled`, where
`thinking_disabled = chat_template_kwargs.get("enable_thinking") is False`). If
the sampling params were adjusted for a parser that then ends up None, the
model's control markers would reach the client as visible text. The call is
therefore placed after chat_template_kwargs is resolved.
This is a no-op for parsers that do not need it: the base
ReasoningParser.adjust_request is `return request`.
Two files are required. prepost.py alone is inert -- vllm_processor.py must pass
reasoning_parser_class into preprocess_chat_request, otherwise the new parameter
defaults to None and the gate never fires.
Adds unit coverage for the wiring and, importantly, for the gate: the parser must
NOT be constructed or consulted when enable_thinking is False.
Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses review feedback on ai-dynamo#12541: adjust_request() can change more than skip_special_tokens. A reasoning parser that rewrites the caller's constraint -- converting response_format into the structural tag its model actually speaks -- had that rewrite dropped. parser_guided_decoding was gated on `tool_parser is not None`, and build_tool_call_guided_decoding returns early when the request has no tools, so the rewrite never reached guided_decoding. The request carried the structural tag while the engine was handed the pre-adjustment JSON schema captured before _prepare_request ran. Recompute the parser guidance when either parser is active, and let a rewrite that only the reasoning parser can have made (no tool parser in the request, so it is attributable) take precedence over the caller's pre-adjustment copy. Scoped deliberately: when a tool parser is also active the change is not attributable to either parser, and parser-vs-assistant precedence is already settled there -- the caller's constraint wins, pinned by test_assistant_guidance_takes_precedence_over_auto_tool_guidance and the response-format-precedence row of TOOL_GUIDANCE_PARITY_CASES. Revisiting that policy is a separate change. Also single-source the reasoning-parser gate as _reasoning_parser_enabled(), a TypeGuard now shared by _prepare_request, preprocess_chat_request and StreamingPostProcessor. The three must agree: if they drift, the sampling params get adjusted for a parser that then does not exist and the model's control markers reach the client as visible text. Tests: four cases in TestReasoningParserGuidanceForwarding covering the rewrite with no tool parser, a pass-through parser, no parser, and the thinking-disabled gate. The first fails without this change with the reported symptom ({"json": {"type": "object"}} instead of the structural tag). The reasoning-parser stubs move to a per-test fixture so no mutable registry is shared at module scope, per .ai/pytest-guidelines.md (CodeRabbit RUF012, Devin). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz
Removes the tradeoff in the previous commit rather than settling it.
Both parsers rewrite the same request inside _prepare_request -- tool first,
reasoning second -- and the code diffed the guidance across both calls at once.
That single diff cannot say which parser moved it, so attribution was inferred
from `tool_parser is None`. The inference is what forced a choice: with both
parsers active a reasoning rewrite was unattributable, so it lost to the
caller's copy, which is the bug this PR exists to fix.
Take the reading between the two calls and attribution stops being a guess:
client -> after_tool_parser changed by the tool parser
after_tool_parser -> final changed by the reasoning parser
Each rewrite then gets its own precedence, which is what was wanted all along.
A reasoning rewrite wins whether or not a tool parser is active, closing the
gap. A tool rewrite still loses to the caller's explicit constraint, so the
pinned policy is untouched. When both rewrote it there is nothing to arbitrate:
the reasoning parser ran last, on the already-tool-adjusted request, so the
final value IS the composition -- forwarding it is forwarding what the request
holds, which is the invariant the report was about.
_prepare_request takes an optional write-only guidance_snapshots dict rather
than a sixth return value: the return is unpacked at 22 call sites across the
two test files and widening it would have churned all of them.
The snapshot deliberately uses request_for_sampling.structured_outputs, the same
accessor as adjusted_structured_guidance, since it is only ever compared against
that value; extract_structured_outputs() is a different view and would have made
the diff spurious.
Deletes reasoning_only_guided_decoding and its scoping rationale.
Validated against the previous commit, same container, prepost.py swapped
underneath:
test_vllm_processor_unit.py before 11 failed / 117 passed
after 10 failed / 118 passed
regressions: none; the only delta is the new gap test
tests/frontend/test_prepost.py 15 passed both
The gap test fails on the previous commit with the exact symptom from the
report -- {'json': {'type': 'object'}} instead of the structural tag -- and the
three pinned cases pass under both.
Signed-off-by: Yifan Jiang <19356972+yifjiang@users.noreply.github.com>
…snapshot Addresses three review threads on ai-dynamo#12541. [P1] model_config. ReasoningParser.__init__ reads model_config out of **kwargs, and BaseCohereCommandReasoningParser.adjust_request returns the request untouched when it is absent (`if model_architecture is None: return request`). Constructing without it made this PR's whole adjust_request() call a no-op for the only shipped parser that overrides it. Verified against real vLLM: same parser, same request, model_config absent -> response_format kept and structured_outputs None; present -> structural tag produced and response_format cleared. The config is already reachable as self.input_processor.model_config (InputProcessor sets it from vllm_config), so no constructor change is needed. Threaded to all three construction sites -- _prepare_request, StreamingPostProcessor, and _build_reasoning_parser_metadata. The third does not read it, but constructing one of them differently is exactly the drift that hid this. NOT added to parser_kwargs: that dict goes on the wire to the worker and ModelConfig is not serializable. [P2] Snapshot cost. The guidance snapshot moved inside the _reasoning_parser_enabled() gate, since its only reader sits behind the same test and short-circuits. The deepcopy is gone: adjust_request() replaces request.structured_outputs with a fresh object rather than mutating it, so a plain reference still denotes the pre-adjustment value -- and aliasing lets the caller's `!=` short-circuit on identity instead of walking the tree. Measured on a 1.49 MB schema: 56.081 ms -> 0.0001 ms for the copy, plus 0.7155 ms -> 0.0001 ms on the comparison it feeds. Tool-only and no-parser requests now pay neither. [P2] Boundary test. test_generator_inner_forwards_reasoning_parser_and_model_config drives the real _generator_inner with a spy and asserts both wiring arguments arrive. Deleting either line fails only this test; before it, deleting reasoning_parser_class left all 143 green. Traded out the overlapping test_no_reasoning_parser_keeps_the_caller_constraint per the review. Also adds _ModelConfigGatedReasoningParser, a fake that mirrors Cohere's actual contract (no model_config => no rewrite). The existing fakes ignored model_config entirely, which is why a no-op passed them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LqtTc1QsXejXiEdpmLZ4Wz
Signed-off-by: Ryan McCormick <rmccormick@nvidia.com>
…r path Squashed from ai-dynamo#13947 for this release branch. dynamo ai-dynamo#12181 added reasoning-token usage only to the Rust OpenAIPreprocessor. `--dyn-chat-processor vllm` selects the Python path, which bypasses it, and the worker's _build_completion_usage() emits no `completion_tokens_details` key at all -- so `reasoning_tokens` was ABSENT from usage (NVBug 6678449b). The count is accumulated where the reasoning parser CLASSIFIES its output (StreamingPostProcessor.reasoning_token_total), not where the response is projected. Deriving it from the emitted choices loses it whenever a projection drops or defers the reasoning: include_reasoning=false suppresses the delta, and the non-streaming tool path buffers every chunk and releases on a zero-token terminal delta. Measured on Inkling-NVFP4 / GB300 with vLLM 0.28.0, reported vs an independent tokenization of the returned reasoning_content, 3 runs each: non-streaming tool call before 0.03/0.03/0.02 after 1.00/1.00/1.00 plain reasoning request before 1.04/1.03/1.02 after 1.00/1.03/1.04 Before, the tool case reported a constant 7 whatever the real count was. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
Bumps the vLLM runtime base to v0.28.0-ubuntu2404 and layers the Inkling structural-tag patch onto it, following the Nemotron-3 Ultra pattern (ai-dynamo#10234). Why 0.28.0: Inkling's tool parser and the structural-tag registry the patch extends only exist from that release. The tag is a multi-arch manifest list (linux/arm64 + linux/amd64), so GB300 aarch64 is covered. xpu and cpu stay at 0.27.1 -- untested at 0.28.0, and Inkling does not use them. Why the patch: vLLM ships an Inkling tool parser but registers no structural tag for it, so Dynamo finds no grammar to install for a tool request and silently degrades to tool_choice="auto" -- HTTP 200, no warning, and a forced tool choice is simply not honoured. Upstream as vllm-project/vllm#54120; the .patch here is cut against the exact v0.28.0 tree and applies with `patch --forward -p1` clean. validate_inkling_runtime.py asserts the POSTCONDITIONS rather than trusting patch's exit code: 'inkling' registered in both the builtin set and the builder registry, structural_tag_model == "inkling", and supports_required_and_named equal to the value AbstractToolParser.__init_subclass__ derives from VLLM_ENFORCE_STRICT_TOOL_CALLING (setting it by hand is redundant and silently overwritten). Verified against the real patched runtime, and verified to FAIL when the tag is unregistered or the vLLM version differs. The build-time version test is load-bearing: `patch --forward` would otherwise skip or fuzz a hunk on a different base and leave a half-patched image that still builds successfully. NOTE: the image alone is not sufficient. The worker must also run with --dyn-enable-structural-tag AND --structured-outputs-config '{"enable_in_reasoning": true}'; without the second, vLLM never fills the grammar bitmask for a reasoning model and the grammar is inert. That belongs in the recipe, not here. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
The pre-commit shebang hook requires an executable bit for a file that declares one. This validator is invoked as `python3 <file>` from the Dockerfile, exactly like its sibling validate_torch_compile_smoke.py, which carries no shebang and mode 644. Match the sibling rather than making the file executable. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…g parser ai-dynamo#12541 threads model_config into the reasoning-parser constructor, so the fake parser in the reasoning-accounting tests needs to accept it. Test-only; the production path already passes it. Caught by running the suite on the merged tree -- the cherry-pick auto-merged without conflict, which is not the same as being correct. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…uired
The previous note claimed Inkling's tool parser and structural-tag registry only
exist from 0.28.0. That is wrong -- both files, and every symbol the Inkling
structural-tag patch touches, are present in 0.27.1, and the patch applies there
cleanly. The bump is required for a different reason.
The Inkling NVIDIA path moved substantially after 0.27.1, and the MTP support
this recipe runs on is part of it:
* models/inkling/nvidia/mtp.py -- InklingMTP gains
SupportsMultiModalEmbeddings, from "Restore multimodal draft capability
detection" (vllm#50417), which also reworks v1/spec_decode/llm_base_proposer
* models/inkling/nvidia/ops/fa4_rel_attention.py (+400) and attention.py --
FA4 warmup migration (vllm#49315)
* model.py / moe.py / ops/lamport.py -- shared-expert partial addition fused
into the Lamport collective (vllm#50697)
* configs.py -- HF-config compat for Inkling (vllm#51850)
Three Inkling parser fixes land in the same range (vllm#49876, #50528, #51391),
which is the behaviour the frontend commits on this branch interact with.
Separately, every Inkling number we have -- p0, IFEval/GPQA-D, and the moontrace
throughput ladder -- was measured on 0.28.0, so shipping 0.27.1 would hand QA an
engine combination that has never been validated for this model.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
… marker Refreshes the vendored patch from vllm-project/vllm#54120, which now begins the tool/text tags at `<|content_invoke_tool_json|>` / `<|content_text|>` instead of at `<|message_model|>`. Inkling's chat template prefills the role marker into the generation prompt (it ends `...<|end_message|><|message_model|>`), so generation resumes inside the model message and never re-emits it. The tags are `tags_with_separator` with `at_least_one=true`, so they anchor rather than trigger -- the old grammar therefore required a SECOND role marker. Compiling the tag with xgrammar shows the inversion, for forced/required/auto alike: suffix-only with duplicate marker before rejected accepted after accepted rejected It appeared to work only because the parser absorbed the duplicate, while manufacturing exactly the unconsumed-control-marker residue ai-dynamo#12510 strips. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, 2-replica AGG, applied over the shipped image via ConfigMap: forced tool choice honoured 5/5 against a prompt matching the other tool; required = 1 call; parallel_tool_calls true/false = 2/1; no control-marker residue in content -- all in both streaming and non-streaming. Full p0 blocking count unchanged at 2 (the unrelated reasoning_budget_zero vLLM limitation). Note the probe suite could NOT have caught this: control_marker_residue passed before the fix too, because the parser was absorbing the duplicate. The grammar-level acceptance test is what exposed it. Reported by dynamo-review-agent on ai-dynamo#13954. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
`vllm-runtime / Test cuda13.0` failed on both amd64 and arm64:
RuntimeWarning: vLLM and vLLM-Omni appear to have mismatched major/minor
versions:
--> vLLM-Omni version 0.27.0rc1
--> vLLM version 0.28.0
vllm-omni asserts matching major/minor against the installed vLLM at import, so
`vllm_omni_ref` is coupled to the runtime image tag and has to move with it.
vllm-omni 0.28.0rc1 is published on PyPI (py3-none-any, not yanked).
Adds a comment recording the coupling so the next bump does not repeat it.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
`vllm_omni_ref` was global while only the cuda13.0 stanza moved to vLLM 0.28.0,
so the xpu and cpu images -- still on vLLM 0.27.1 -- would have installed
vLLM-Omni 0.28.0rc1 and hit the very major/minor mismatch the previous commit
pinned away from. Confirmed by rendering: before this change all three devices
emitted `ARG VLLM_OMNI_REF=v0.28.0rc1` while xpu/cpu kept
`RUNTIME_IMAGE_TAG=v0.27.1`, and install_vllm_omni.sh runs on every device.
Move the ref into each device stanza next to the runtime_image_tag it must
track, and read it in args.Dockerfile from the selected device context:
cuda13.0 vLLM v0.28.0-ubuntu2404 omni v0.28.0rc1
xpu vLLM v0.27.1 omni v0.27.0rc1
cpu vLLM v0.27.1 omni v0.27.0rc1
Chosen over bumping xpu/cpu to vLLM 0.28.0 (their 0.28.0 base images do exist)
because those targets are untested here and Inkling does not use them; this
keeps their behaviour byte-identical to the base branch.
The ARG sits inside `{% if framework == "vllm" %}`, so sglang/dynamo/trtllm
never evaluate it -- verified by rendering each. (trtllm/cuda13.0 is an invalid
combination on the base branch too; trtllm renders on cuda13.1, which is what CI
builds.)
Reported by dynamo-review-agent on ai-dynamo#13954.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…review fixes
Two problems, both found while checking the branch against what was actually
validated rather than waiting for another CI round.
1. transformers_version was global at 5.14.1, but the VALIDATED Inkling image
runs transformers 5.15.1. Shipping the pin would hand QA a transformers we
never measured -- which also undercuts the argument for carrying the existing
p0/accuracy/throughput numbers forward to the release container.
The pin's own removal condition is met on cuda: it exists because vLLM 0.27.1
lacks the Transformers 5.15 heterogeneous Gemma 4 support, and vllm#49797
landed that and IS in 0.28.0 (verified: in v0.28.0, not in v0.27.1). xpu/cpu
are still on vLLM 0.27.1 and still need 5.14.1, so -- exactly as with
vllm_omni_ref -- the value has to be per-device, not global:
cuda13.0 vLLM v0.28.0-ubuntu2404 omni v0.28.0rc1 transformers 5.15.1
xpu vLLM v0.27.1 omni v0.27.0rc1 transformers 5.14.1
cpu vLLM v0.27.1 omni v0.27.0rc1 transformers 5.14.1
TRANSFORMERS_VERSION is a hard `transformers==${...}` install plus a
build-time equality assertion, so an empty value is not an option; each
stanza carries a concrete version.
2. The frontend files had drifted from ai-dynamo#13947, which picked up two CodeRabbit
fixes after the squash landed here: a negative backend `reasoning_tokens`
survived the falsiness check (-1 is truthy) and `total` used a defensive
getattr on an attribute its own constructor guarantees. Forward-ported both,
with the parametrised regression test; the annotator block is now identical
between the two PRs.
Rendered all three vllm devices plus sglang / dynamo / trtllm(13.1) to confirm
the per-device lookup does not disturb other frameworks. Accounting suite 13/13.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…ing patch Syncs both of our unmerged fixes to their reviewed state and re-validates the container on real hardware. frontend (ai-dynamo#13947, P1 review): reasoning tokens are now counted by the pinned vLLM parser's own `count_reasoning_tokens` over a per-choice ledger of the ORIGINAL generated ids, appended before any branch in process_output. The per-chunk classifier and the terminal `tokenizer.encode(saved_reasoning)` retokenisation are both removed -- the latter drifted because detokenisation is not injective (generated id 77150 decodes to '１０' and re-encodes to two tokens). vLLM patch (vllm-project/vllm#54120): regenerated against the v0.28.0 tree, now also accepting `<|content_model_end_sampling|>`. The Inkling parser treats it as a co-equal terminator -- `is_reasoning_end` returns True on it and `count_reasoning_tokens` closes a block on either -- so permitting only `<|end_message|>` masked a valid natural stop. Applies clean with the Dockerfile's own `patch --batch --forward -p1`. Validated on Inkling-NVFP4 / GB300, vLLM 0.28.0, MTP 8, on BOTH topologies with the full patch set overlaid on the shipped image. p0: 13/15 with 2 blocking on each, and the two blockers are the same unpatchable vLLM limitation (reasoning_budget_zero, NVBug 6678449a): 2-replica AGG structural_tag_grammar pass · reasoning ratio 1.00 · kv_routing pass 1P1D disagg structural_tag_grammar pass · reasoning ratio 1.00 p0 was extended to cover these changes, because it structurally could not before: the tag can be wrong while every wire-level probe passes (the parser absorbs the malformed output -- control_marker_residue passed before BOTH tag defects), and reasoning_tokens can be non-zero but the wrong order (the pre-fix code reported a constant 7 against 324 real tokens). Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…onstraint
The multi-arch build failed at the vLLM-Omni install on both arches:
Because vllm-omni>=0.28.0rc1 depends on transformers>=5.10.1,<5.15 and
transformers==5.15.1, we can conclude that vllm-omni>=0.28.0rc1 cannot be
used.
vllm-omni 0.28.0rc1 declares a hard upper bound of <5.15, so pinning 5.15.1
makes the image unbuildable. Reverted to 5.14.1, which satisfies omni 0.28.0rc1
on cuda and omni 0.27.0rc1 on xpu/cpu.
The earlier bump was justified with bad evidence: "the validated image runs
transformers 5.15.1". That image is the thin pip-layer dev build, which does NOT
install vllm-omni at all -- so it could never have exercised this combination.
A running image only demonstrates the combination it actually contains.
vllm#49797 (Gemma 4 heterogeneous config) being in 0.28.0 makes 5.15 PERMISSIBLE
for vLLM, which needs only >=5.5.3 -- but not required. Omni is what binds here.
Revisit when an omni release relaxes the bound.
Adds check_pin_compatibility.py, which resolves the pins in each device stanza
against their PyPI metadata and fails on a conflict. It reproduces this exact
error in under a second instead of ~15 minutes into a multi-arch build, and it
is verified in both directions: the fixed context passes, and reintroducing
5.15.1 reproduces the failure. Note it only compares pins against EACH OTHER, so
a conflict with an unpinned transitive dependency still needs a real build.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
Dropping the preflight script. The review correctly found it fails OPEN -- a PyPI fetch error was printed and ignored, so an unreachable index would exit 0 and falsely validate. A gate that passes when it cannot check is worse than no gate. That is a symptom rather than the reason to remove it. The script only compared pinned packages against EACH OTHER, so it never covered transitive dependencies; the container build already catches this class and everything metadata cannot see. It was added reacting to my own mistake, and it costs reviewers a second source of truth about dependency resolution that can drift from what uv does. What prevents recurrence is the comment kept in container/context.yaml: it records that vLLM-Omni, not vLLM, is what binds transformers below 5.15, and that the thin dev image carrying 5.15.1 does not install omni at all -- so it was never evidence that the combination resolves. That sits at the point of edit, which is where the mistake was made. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…fusers breaks
Two independent CI failures, neither in the shipped code paths.
1. test_engine_core_struct_contract. vLLM 0.28.0 appends `session_id` to
EngineCoreRequest, after `abort_immediately`. The observed tuple differs from
the nearest allowed variant by that one field. Added session_id variants
(plain and vllm-omni), mirroring how the test already handles
abort_immediately and model_intermediate_buffer.
No production change: the test docstring claims vllm_processor.py reads
EngineCoreRequest positionally, but it does not -- it uses attribute access
(vllm_preproc.mm_features) and builds EngineCoreOutput by keyword
(EngineCoreOutput(**output_kwargs)). A trailing append is therefore
compatible, which is what the model_intermediate_buffer comment already says.
2. test_vllm_omni.py errored 4x and test_realtime_omni_bridge failed with "Main
server process exited with code 1", both from:
RuntimeError: Failed to import diffusers.pipelines.pipeline_utils ...
cannot import name 'get_cached_repo_tree' from 'huggingface_hub'
The omni guard caught (ImportError, OSError, NotImplementedError). diffusers
imports its pipelines lazily and re-raises ANY underlying failure as
RuntimeError, so a broken transitive dependency never reaches that tuple and
the module ERRORS instead of skipping. Added RuntimeError.
test_realtime_omni_bridge had no omni guard at all; its worker
(realtime_omni_mock_worker.py) imports vllm_omni and dynamo.vllm.omni at
module scope, so the same failure killed the worker and surfaced as an opaque
server-exit. It now probes those imports and skips.
These tests could not run at all before vllm-omni became installable on this
branch, so they were silently skipping rather than passing. Making omni
installable is what exposed the guard gap.
Measured, not inferred: the base image ships huggingface_hub 1.28.0, which DOES
export get_cached_repo_tree (verified in the wheel), and the build never
installs, moves or downgrades hub. So this is a diffusers-0.40.0-vs-hub-1.28.0
incompatibility, not a pin conflict introduced here -- worth reporting upstream
separately. Skipping restores the behaviour the suite already intended.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
The request-side assertion now passes; this is the output-side one behind it. vLLM 0.28.0 appends `mm_cache_miss_hashes` and `new_sampling_mask` after num_nans_in_logits -- after vLLM's own fields, before anything vllm-omni adds. No production change. vllm_processor.py builds EngineCoreOutput BY KEYWORD from four required kwargs and feature-detects optional fields through __struct_fields__, so appended fields take their msgspec defaults. That is not just an argument from the code: this exact path served ~7000 requests on vLLM 0.28.0 during validation (p0 plus a full 3541-request moontrace replay on both 2-replica AGG and 1P1D) with no error, so the defaults are demonstrably acceptable for detokenization. Rather than enumerate another four hand-written tuples, the variants are now generated as a product of (base | cached-token) x (with | without the 0.28.0 extras) x (with | without the omni extras) -- 8 variants from the same three building blocks the test already defined. Verified the CI-observed tuple is accepted and all four previously-valid shapes still are. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
The previous commit replaced the hand-written omni output tuples with a generated product, leaving omni_output_fields and omni_cached_token_output_fields assigned but unused -- ruff F841, which I pushed without reading the pre-commit result. They are referenced nowhere else; the generated variants cover the same shapes. Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
`pytest.skip(..., allow_module_level=True)` removes the module from
COLLECTION, not just from running. The CI step
`-m "(pre_merge and vllm and gpu_1) and not profiled_vram_gib"` has
exactly one match in the repo -- `test_omni_serve_deployment[omni_audio]`
in tests/serve/test_vllm_omni.py -- so once that module stopped being
collected, the step selected nothing and pytest exited 5 ("no tests ran"),
which the runner reports as a job failure.
The test was already marked `pytest.mark.skip`, so before this it was
collected and reported as skipped and the step exited 0. Guard the probe
import into a flag and skip via `pytest.mark.skipif` instead: same intent,
but the items stay collected and are reported as skipped.
Verified in the runtime container with the omni import blocked, running
that exact marker expression against tests/serve/test_vllm_omni.py:
module-level skip -> "1 skipped", exit 5; skipif -> "1 skipped, 5
deselected", exit 0.
tests/frontend/test_realtime_omni_bridge.py carried the same pattern and
gets the same treatment, so it cannot empty a selection later.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
…rocessor path
Two request-rejection paths in the Python chat processor
(`--dyn-chat-processor vllm`) surfaced as HTTP 500 with no usable
explanation. The Rust OpenAIPreprocessor handles both correctly; the
Python path does not go through it.
1. A worker-side rejection -- an unsupported sampling parameter, an
unparseable grammar -- crosses the Rust boundary as a plain exception
whose text is `BackendInvalidArgument: {"message":...,"code":400}`.
The generic `except Exception` turned that into an internal error,
discarding both the status and the reason.
`backend_invalid_argument_to_http_error()` recovers them, mirroring
`extract_backend_error_if_present` in
lib/llm/src/http/service/openai.rs.
2. `logprobs`/`top_logprobs` are not implemented on this path (see the
existing TODO), but the code warned and continued anyway, emitting a
response with an empty logprobs struct that the HTTP layer cannot
deserialize ("invalid length 0, expected struct ChatChoiceLogprobs
with 2 elements"). It now rejects up front.
The rejection is 400 rather than the 501 used for other unimplemented
fields (`previous_response_id`, `background`, `prompt`): those are raised
inside the Rust HTTP service, whereas a status asserted by a Python
engine is triaged by SanitizedError::for_backend_status, which forwards a
4xx verbatim but replaces the body of any 5xx. A 501 raised here reaches
the client as an opaque 500 with the reason lost -- observed, not assumed.
Measured on Inkling-NVFP4 / GB300 (2-replica AGG, vLLM 0.28.0): min_p,
malformed json_schema, logprobs and top_logprobs all go 500 -> 400
carrying the backend's own message; the other seven probe cases are
unchanged.
test_generator_preserves_zero_top_logprobs pinned the warn-and-continue
behaviour, so it is repointed at the rejection and renamed. It still
guards the subtlety it always did: `logprobs=True` with `top_logprobs=0`
projects to 0, so a truthiness guard would let exactly that request
through and re-create the 500 -- the guard tests `is not None`.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
(cherry picked from commit 894ee68)
…sagg
On a disaggregated deployment a request the prefill worker had correctly
rejected with 400 reached the client as a 500. The worker's error was
attached to `PrefillError::PrefillError` as a `#[source]` only, and every
path out of the prefill router reaches the frontend through `to_pyerr`,
which formats with `Display` and drops the error chain
(`lib/bindings/python/rust/lib.rs`). The frontend therefore saw the bare
"Prefill router returned error in output" and could not tell an invalid
argument from an engine fault.
Fold the worker's text into the message at the two sites that wrap a real
error, so the `BackendInvalidArgument: {...}` discriminator survives the
Display-only boundary. The HTTP layer and the Python chat processors
already key on it, so no other change is needed for the status to reach
the client. `match_error_chain` is unaffected -- it matches error TYPES
through the chain, never this string -- and the sourceless
"no output (stream ended)" case is left alone.
Also stop yielding untagged error dicts from the vLLM chat processor. The
binding only treats a dict as an `Annotated` envelope when it carries
`_dynamo_annotated` (`depythonize_annotated`); a bare `{"error": {...}}`
is parsed as a completion chunk, fails with "missing field `id`", and the
client gets an opaque 500 in place of the message -- so the reason never
arrives even when the frontend has one. `as_error_envelope()` tags it;
the shared helpers keep their shape, so the SGLang processor and the
existing tests are untouched.
Measured on Inkling-NVFP4 / GB300 1P1D disagg, where min_p and two
malformed json_schema payloads returned 500 3/3 while logprobs (rejected
frontend-side, so path-independent) correctly returned 400.
Signed-off-by: Sungsoo Ha <sungsooh@nvidia.com>
sungsooha
temporarily deployed
to
external_collaborator
August 31, 2026 17:49 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
sungsooha
temporarily deployed
to
external_collaborator
August 31, 2026 17:49 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
👋 Hi sungsooha! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
github-actions
Bot
added
the
external-contribution
github-actions
Bot
added
backend::vllm
sungsooha
changed the title
This branch was previously deployed
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.
⛔ DO NOT MERGE — this PR exists only to produce a CI container image
This is #13954's branch plus one commit. It is opened solely so CI builds a
vllm-runtimeimage carrying both the Inkling container work and an in-flight fix, so thatimage can be validated on GB300 and handed to QA. It is not for review and not for merge.
main(
inkling/prefill-router-preserve-backend-status, 4 files), once validated on hardware.The additional commit
fix(kv-router): preserve the prefill worker's error status through disaggOn a disaggregated deployment a request the prefill worker had correctly rejected with 400
reached the client as a 500. Two independent causes:
PrefillError::PrefillErroras a#[source]only, and everypath out of the prefill router reaches the frontend through
to_pyerr, which formats withDisplayand drops the error chain (lib/bindings/python/rust/lib.rs). The frontend saw the bare"Prefill router returned error in output"and could not distinguish an invalid argument from anengine fault. The two sites that wrap a real error now fold its text into the message, so the
BackendInvalidArgument: {...}discriminator survives — which the HTTP layer and the Python chatprocessors already key on.
match_error_chainis unaffected (it matches error types through thechain, never this string), and the sourceless "no output (stream ended)" case is unchanged.
{"error": {...}}dicts. The binding only treats a dict as anAnnotatedenvelope when it carries_dynamo_annotated(depythonize_annotated); untagged, it isparsed as a completion chunk, fails with
missing field `id`, and the client gets an opaque 500instead of the message.
as_error_envelope()tags it. The shared helpers keep their shape, so theSGLang processor and the existing tests are untouched.
Evidence
Reproduced on Inkling-NVFP4 / GB300 1P1D disagg:
min_pand two malformedjson_schemapayloadsreturned 500 3/3, while
logprobs— rejected frontend-side, so path-independent — correctly returned400. Aggregated was unaffected, which is why the earlier fix (#13980, now merged) passed on AGG and
missed this. Follow-up to NVBug 6678456.