[INVESTIGATE] sglang + DSv4: kwarg workaround breaks live — needs diagnosis - #8671
[INVESTIGATE] sglang + DSv4: kwarg workaround breaks live — needs diagnosis#8671ishandhanani wants to merge 21 commits into
Conversation
Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: ayushag <ayushag@nvidia.com>
Signed-off-by: ayushag <ayushag@nvidia.com>
…el_type
Previously `PromptFormatter::from_mdc` activated the V4 / V3.2 native
formatters based on a substring match against `mdc.display_name`:
name_lower.contains("deepseek") && name_lower.contains("v4")
Two problems:
1. **Rename-fragile** — `display_name` is overwritten by
`--served-model-name`, so a valid DeepSeek-V4 checkpoint served as
`dsflash` (or any other operator-chosen alias) silently fell through
to the HF Jinja path and the native Rust formatter never fired.
2. **Loose** — `contains("v4")` mis-matched composite names like
`deepseek-v3.2-v4-foo`, short-circuiting to V4 before the V3.2
branch.
Rework detection to key off `config.json` `model_type`, which
DeepSeek-V4-Pro / V4-Flash both ship as `"deepseek_v4"` (verified on
the HF repo). The config value is set by the model author and survives
any rename. `display_name` is kept only as a fallback for MDCs that
don't have a loadable config.json (tokenizer-only deployments), and a
concrete-but-different config.json value now authoritatively suppresses
the fallback — a `model_type: "llama"` model served as
`deepseek-v4-flash` no longer gets the V4 formatter.
Also tightens the display-name fallback to an anchored match equivalent
to `^deepseek(?:[-_.])?v4(?:[-_.]|$)` (no `regex` crate dep), closing
the composite-name hole flagged as N5 in the V4 review.
Unit tests cover: canonical V4 variants (hyphen / underscore / dot /
concat separator), negative cases (composite / non-V4 / mis-prefixed),
config-primary precedence, and the V3.2 symmetric case.
`HFConfig::model_type` is a required field at parse time, so a missing entry fails deserialization and `get_model_info().ok()` drops the whole `ModelInfo` — we correctly fall back to the display-name heuristic. But `"model_type": ""` (rare, legal JSON) parses cleanly into `Some(String::new())`, hits the `Some(_) => false` arm in `is_deepseek_v4`, and *suppresses* the display-name fallback. A DeepSeek-V4 model served as `deepseek-v4-flash` with an empty `model_type` would silently miss the native formatter. Normalize empty strings to `None` via `.filter(|s| !s.is_empty())` so "no signal from config.json" behaves uniformly regardless of whether the field was absent or blank.
github-actions
Bot
added
backend::sglang
`to_json` serializes tool schemas, response-format bodies, and arbitrary message-content fallbacks through a custom `PythonFormatter`. The buffer was `Vec::with_capacity(64)` — too small for anything beyond a trivial value, and worse than `Vec::new()` for tiny values since it forces an up-front heap alloc that's immediately outgrown. For a ~5 KB tool schema (200-field array), 64 bytes triggers 6 sequential doublings (64→128→256→512→1024→2048→4096→8192), each a memcpy of the current contents. Callers hit this on every request that includes tools or a response_format, so the overhead is per-request on the hot path. Pre-size the buffer from one compact `serde_json::to_string` pass. The `PythonFormatter` adds exactly 1 byte per structural separator (`,`→`, ` and `:`→`: `), which bounds the final length by ~12.5% over compact — `compact_len + compact_len/8` is a tight upper bound that eliminates all reallocations in the formatted pass. A 256-byte floor keeps tiny payloads from under-allocating after empty-map round-trips. Adds a ~5 KB round-trip test (`test_to_json_handles_large_payload`) that pins: - Output parses back to the input (no truncation). - Python-style spacing survives deep nesting (no bare `",", / `":"`). All 7 deepseek_v4 unit tests pass including the new one.
WalkthroughThis pull request introduces comprehensive DeepSeek V4 model support, including a native Rust prompt formatter with DSML tool-calling, message normalization, and role-based rendering. It adds reasoning-mode detection, updates formatter selection logic, extends parser registries, and includes extensive test fixtures and streaming integration tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/src/dynamo/sglang/request_handlers/llm/decode_handler.py (1)
275-277:⚠️ Potential issue | 🟠 MajorRemove the unused
return_routed_expertsvariable to fix Ruff F841 pipeline failure.The assignment at lines 275-277 is dead code: both
async_generatecall sites (311, 349) have the parameter commented out. The downstream stream processors (_process_token_streamat line 431 and_process_text_streamat line 513) do attempt to extract and handlerouted_expertsfrom the engine response, but because the kwarg is disabled, the engine will never populate that field—effectively disabling the feature. Either remove this assignment and clean up the commented-out parameters, or uncomment the parameters and resolve any sglang API compatibility issues. There is no TODO explaining the temporary disable, so the intent is unclear.🔧 Proposed fix (remove the dead variable)
sampling_params = self._build_sampling_params(request) input_param = self._get_input_param(request) - return_routed_experts = getattr( - self.config.server_args, "enable_return_routed_experts", False - ) priority = (request.get("routing") or {}).get("priority")Also applies to: 311-311, 349-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py` around lines 275 - 277, Remove the dead local assignment return_routed_experts in decode_handler.py and clean up the related commented-out kwarg usage: delete the return_routed_experts = getattr(...) line, remove or permanently uncomment the commented 'return_routed_experts' kwargs at the async_generate call sites (the calls to async_generate in this module), and ensure downstream processors (_process_token_stream and _process_text_stream) either rely on the engine-populated routed_experts field or are updated to not expect it; this removes the unused variable and resolves the Ruff F841 failure while keeping the async_generate and stream-processing flow consistent.
🧹 Nitpick comments (9)
lib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_mixed_param_types.json (1)
3-3: Minor: inconsistentfinish_reasonassertion across sibling fixtures.
expected_outputhere omitsfinish_reason, whereaschat_completion_stream_special_chars.jsonasserts"finish_reason": "tool_calls". Since the final stream delta (line 15) does emitfinish_reason: "tool_calls", consider adding the assertion here (and inchat_completion_stream_tool.json) for parity — otherwise a regression that drops the terminal finish event would silently pass this fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_mixed_param_types.json` at line 3, The fixture's expected_output is missing the terminal "finish_reason" assertion which other sibling fixtures (e.g., chat_completion_stream_special_chars.json) include; update the expected_output object to include "finish_reason": "tool_calls" alongside the existing "normal_content", "reasoning_content", and "tool_calls" entries so the final stream delta that emits finish_reason is validated (refer to the "expected_output" object, the "tool_calls" array and the "call_1" entry to locate the right fixture and place the "finish_reason" key).lib/llm/src/preprocessor/prompt/deepseek_v4.rs (6)
806-866:renderinjects tools/response_format without re-normalizing the synthetic system message.
normalize_message_contentsruns at Line 818 before the block at Lines 832-863 that either augments an existing system message or inserts a new one with"content": "". The injected/augmentedtoolsandresponse_formatfields are rawserde_json::Valuefromreq.tools()/req.response_format()and skip any normalization. This is fine today becauserender_toolsandRESPONSE_FORMAT_TEMPLATEconsume them as JSON, but if normalization is ever extended to cover tool/response_format shapes (e.g., stripping nulls), the pre/post ordering will silently diverge.Minor: also note that when no system message exists,
insert(0, system_msg)pushes the ASSISTANT_START transition logic one index down; this is tested implicitly via fixtures, but worth a brief comment explaining why the order matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 806 - 866, The render function injects raw tools/response_format JSON into the system message after normalize_message_contents already ran, so those fields skip normalization; update render (in deepseek_v4.rs) to normalize the injected/augmented system message contents after adding tools/response_format (e.g., call normalize_message_contents on the modified messages_array or explicitly normalize the new system_msg object) so injected fields receive the same normalization rules as existing messages, and add a brief comment near the messages_array.insert(0, system_msg) noting that inserting at index 0 shifts the ASSISTANT_START transition index.
163-177: Document the-1-sentinel semantics alongside the helper, not just insiderender_message.The doc comment says callers should use
is_none_or(|u| idx >= u)to mirror Python'sidx >= -1, but the two primary call sites (Lines 397, 477, 677) each re-derive this pattern with subtly different boundary conditions (>,>=). A regression here already has a dedicated test (test_assistant_reasoning_preserved_when_no_user_in_history), which is great — consider adding a small helper likefn at_or_after_last_user(idx: usize, last_user_idx: Option<usize>) -> boolto centralize the sentinel logic and prevent the next drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 163 - 177, The helper find_last_user_index currently documents Python's "-1" sentinel but callers re-implement the boundary logic inconsistently; add a small helper fn at_or_after_last_user(idx: usize, last_user_idx: Option<usize>) -> bool that returns true when last_user_idx is None or idx >= last_user_idx (matching the intended idx >= -1 semantics), update the three call sites that currently compare idx using > or >= to call at_or_after_last_user instead, and keep or move the sentinel explanation to the new helper's doc comment so the behavior is centralized and tested.
208-222: Redundant normalization for already-string content.
normalize_message_contentscallsextract_visible_texteven whencontentis already aJsonValue::String, then replaces the original with a clone of itself. Minor but adds a needless allocation per message on a hot path.♻️ Proposed refactor
for msg in messages { let Some(content) = msg.get("content") else { continue; }; - // Leave non-string/non-array content untouched (null, etc.) - if !content.is_string() && !content.is_array() { + // Only arrays (multimodal chunks) need rewriting; strings are already + // text and other shapes (null, etc.) are left as-is. + if !content.is_array() { continue; } let normalized = extract_visible_text(content);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 208 - 222, normalize_message_contents is needlessly calling extract_visible_text for JsonValue::String values causing an extra allocation; update the function so it returns early for string content (leave it untouched) and only call extract_visible_text when content.is_array() (or other non-string types you intend to normalize), then replace the "content" key only in that branch; reference normalize_message_contents, msg, content, and extract_visible_text when making the change.
147-158: Chained.replaceonTOOLS_TEMPLATEis order-sensitive and scans rendered schemas.Since
{tool_schemas}is substituted first, any subsequent.replace("{dsml_token}", ...)/.replace("{thinking_start_token}", ...)also scans the just-substituted tool-schema text. Extremely unlikely to collide (tool schemas are user-provided but the placeholders are unusual), but formally incorrect and easy to tighten.♻️ Proposed refactor
Substitute the static tokens first and the user-controlled schemas last:
- TOOLS_TEMPLATE - .replace("{tool_schemas}", &tools_json.join("\n")) - .replace("{dsml_token}", tokens::DSML_TOKEN) - .replace("{thinking_start_token}", tokens::THINKING_START) - .replace("{thinking_end_token}", tokens::THINKING_END) + TOOLS_TEMPLATE + .replace("{dsml_token}", tokens::DSML_TOKEN) + .replace("{thinking_start_token}", tokens::THINKING_START) + .replace("{thinking_end_token}", tokens::THINKING_END) + .replace("{tool_schemas}", &tools_json.join("\n"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 147 - 158, The current render_tools function chains .replace on TOOLS_TEMPLATE in an order that substitutes {tool_schemas} first which causes subsequent .replace calls to scan user-provided schema text; update render_tools so it first replaces the static placeholders ({dsml_token}, {thinking_start_token}, {thinking_end_token}) on TOOLS_TEMPLATE (using tokens::DSML_TOKEN, tokens::THINKING_START, tokens::THINKING_END) and only after that injects the user-controlled {tool_schemas} (the tools_json.join result) to avoid scanning or altering user content; keep references to the same TOOLS_TEMPLATE and tools_from_openai_format/to_json pipeline but swap the replace order accordingly.
869-1006: Inline unit coverage is light relative to the surface area.The inline tests exercise
test_simple_conversation, reasoning-effort prefix, tool-result merging, auto-disable, and two regression cases — solid foundation. But several non-trivial branches have no dedicated unit coverage and are only exercised (if at all) by the external fixture tests intests/deepseek_v4_encoding.rs:
"developer"role rendering + thetask/task_tokenbranches (especiallytask == "action").wo_eos=trueon assistant messages.sort_tool_results_by_call_orderwith multiple assistants / stalelast_order(see the other comment).encode_arguments_to_dsmlJSON-parse fallback path ({"arguments": raw_string}).DeepSeekV4Formatter::rendertools/response_format injection when no system message exists.Given the PR's own title flags bug-fixing in flight, consider pulling 2–3 of these into dedicated
#[test]s so regressions surface without needing the large fixture files to be perfectly aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 869 - 1006, Add focused unit tests to cover untested branches: write new #[test]s in the same tests module that exercise (1) rendering of messages with role "developer" and a message having task == "action" to hit the task/task_token branch (target encode_messages/DeepSeekV4Formatter rendering of developer/task), (2) an assistant message with wo_eos=true to ensure the assistant EOS suppression path is exercised (target encode_messages/encode_messages_with_options usage), and (3) the encode_arguments_to_dsml fallback where the arguments are a raw JSON string (target encode_arguments_to_dsml to exercise its JSON-parse fallback). Also add one test invoking DeepSeekV4Formatter::render with no system message to verify tools/response_format injection path; each test should assert the specific expected token snippets so regressions fail locally rather than relying on external fixtures.
93-136:to_jsonsilently returns"{}"on serializer/UTF-8 failure.Both
value.serialize(&mut ser).is_err()andString::from_utf8(buf).unwrap_or_else(|_| "{}"...)swallow the underlying error and return a literal"{}". Forserde_json::Valueinput this is effectively unreachable, but if anyone extends callers (e.g., passing something non-roundtrippable), the result silently becomes"{}"embedded in the prompt with zero log/trace — a very hard bug to diagnose in production prompt output.Consider either propagating the error via
Result<String>(most of the callers already returnResult<String>), or at minimum logging atracing::error!with context so this never fails silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs` around lines 93 - 136, The to_json function currently swallows serialization and UTF-8 errors and returns the literal "{}"; change to_json so it surfaces failures instead of hiding them: either (preferred) change its signature to return Result<String, serde_json::Error> (or a suitable Box<dyn Error>) and propagate errors from value.serialize(...) and String::from_utf8(...).map_err(...), or if you must keep a String return, log the error via tracing::error! including context (e.g., the JsonValue or what operation failed) before returning a fallback; update all callers of to_json in this module to handle the Result or accommodate the logged failure. Ensure you modify the to_json function and call sites (references to to_json) accordingly.lib/llm/tests/deepseek_v4_encoding.rs (1)
43-76: Diff output only surfaces the first differing line.On mismatch the
breakat Line 64 stops printing after the first divergence, which is noisy when fixtures drift across many lines (common when the DSML renderer changes). Consider printing all differing line pairs, or falling back topretty_assertions::assert_eq!which produces a proper multiline diff — it's already used elsewhere in the crate if added, and the test reads much better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/tests/deepseek_v4_encoding.rs` around lines 43 - 76, The test helper run_official_test currently stops after printing the first differing line because of the break inside the loop; remove the break so the loop continues and prints all differing line pairs (or alternatively replace the manual comparison with a call to pretty_assertions::assert_eq!(exp, act) to get a full multiline diff); ensure you import pretty_assertions if you choose that route and drop the manual per-line panic so the assertion provides the failure message from assert_eq! instead of the current panic at the end.lib/llm/src/preprocessor/prompt/template.rs (1)
234-243:is_deepseek_v3_2_non_expprecedence is subtly different fromis_deepseek_v4.When
model_type_lower == Some("deepseek_v3_2"), the function still consultsdisplay_nameto veto on"exp", but whenmodel_type_lower == Some(other)the fallbackname_matchis ignored. That's internally consistent with the "V3.2-Exp is a separate family" note, but the variablename_matchis now computed on every call and only read in theNonebranch.Minor nit: either inline the computation inside the
Nonearm (so the helper doesn't do thecontainsscan for the commonSome(_)case), or note explicitly in the doc comment that the"exp"suppression is intentional even when config is authoritative.♻️ Proposed refactor
fn is_deepseek_v3_2_non_exp(model_type_lower: &Option<String>, display_name_lower: &str) -> bool { - let name_match = display_name_lower.contains("deepseek") - && display_name_lower.contains("v3.2") - && !display_name_lower.contains("exp"); match model_type_lower.as_deref() { Some("deepseek_v3_2") => !display_name_lower.contains("exp"), Some(_) => false, - None => name_match, + None => { + display_name_lower.contains("deepseek") + && display_name_lower.contains("v3.2") + && !display_name_lower.contains("exp") + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/llm/src/preprocessor/prompt/template.rs` around lines 234 - 243, The helper is_deepseek_v3_2_non_exp currently computes name_match eagerly yet only uses it in the None branch, causing an unnecessary contains scan and obscuring intent; change it to compute the display_name_lower checks (contains "deepseek", contains "v3.2", and !contains "exp") inline inside the None arm of the match so the Some("deepseek_v3_2") and Some(_) branches remain fast and behavior is unchanged, or alternatively update the doc comment on is_deepseek_v3_2_non_exp to explicitly state that the "exp" suppression is intentionally applied when model_type_lower == Some("deepseek_v3_2").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/src/dynamo/frontend/sglang_prepost.py`:
- Around line 175-178: The call to parser.get_structure_constraint(...)
currently has the parallel_tool_calls kwarg commented out causing inconsistent
behavior vs get_json_schema_constraint(...); inspect
FunctionCallParser.get_structure_constraint and either re-add the
parallel_tool_calls=parallel_tool_calls argument if the signature still accepts
it, or remove the commented line and add a short comment explaining that
get_structure_constraint no longer accepts parallel_tool_calls (and why) so the
omission is explicit; ensure parity in handling of the parallel_tool_calls
setting across the tool_choice branches (reference
parser.get_structure_constraint, parser.get_json_schema_constraint,
parallel_tool_calls, and tool_choice).
In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs`:
- Around line 225-260: encode_arguments_to_dsml currently injects raw parameter
keys and string argument values into DSML which can break XML/DSML parsing; add
proper XML escaping for attribute and element text: implement or reuse an
escape_xml function and apply it to the parameter name (key) and to string
values before interpolating into the tokenized DSML fragment (leave non-string
values using to_json as-is). Update the formatting call sites inside
encode_arguments_to_dsml (the loop that builds params using tokens::DSML_TOKEN)
to use escaped_key and escaped_value for the name and element content
respectively, ensuring &, <, >, " and ' are converted to XML entities.
In `@lib/llm/src/preprocessor/prompt/template.rs`:
- Line 266: Clippy flags manual-pattern-char-comparison for the predicate used
in starts_with; change the pattern-match on a char to direct char comparisons in
both occurrences (the one using after_v4 and the identical one around line 259)
by replacing matches!(c, '-' | '_' | '.') with explicit comparisons like c ==
'-' || c == '_' || c == '.' so the starts_with closure becomes e.g.
after_v4.is_empty() || after_v4.starts_with(|c: char| c == '-' || c == '_' || c
== '.').
In `@lib/llm/tests/data/deepseek-v4/test_output_1.txt`:
- Line 36: Replace the non-conforming tool output tag sequence
<｜User｜><tool_result>...</tool_result> in the fixture with the DeepSeek V4
template tags <｜tool▁output▁begin｜>...<｜tool▁output▁end｜>; locate the string
occurrence (e.g., the fragment containing "<｜User｜><tool_result>") and swap it
to the canonical "<｜tool▁output▁begin｜>{result content}<｜tool▁output▁end｜>"
format so the fixture matches sglang/DeepSeek V4 expectations.
In `@lib/llm/tests/deepseek_v4_encoding.rs`:
- Around line 27-34: In load_messages (the block that reads raw.get("messages")
and tries messages.get_mut(0) to insert "tools"), add an explicit check that
when raw.get("tools") is Some but the messages array is empty you fail loudly
(panic or assert) with a descriptive message instead of silently dropping tools;
i.e., detect tools present && messages.is_empty() and panic with context
(mention tools and fixture) otherwise proceed to insert tools into the first
message as the existing obj.insert("tools", ...).
In `@lib/llm/tests/test_streaming_tool_parsers.rs`:
- Around line 1208-1216: The test only validates the first tool call because
run_deepseek_v4_tool_call_fixture delegates to assert_tool_calls which currently
compares only tool_calls[0] against expected_tool_calls[0]; update the assertion
logic so all tool call pairs are checked: modify assert_tool_calls to iterate
over all indices (e.g., for i in 0..tool_calls.len()) and compare
tool_calls[i].name and tool_calls[i].arguments to expected_tool_calls[i], or
alternatively add explicit assertions in test_deepseek_v4_e2e_multi_tool_vllm
that assert tool_calls[1] matches expected_tool_calls[1]; use the existing
identifiers (run_deepseek_v4_tool_call_fixture, assert_tool_calls, tool_calls,
expected_tool_calls, test_deepseek_v4_e2e_multi_tool_vllm) to locate and change
the code.
---
Outside diff comments:
In `@components/src/dynamo/sglang/request_handlers/llm/decode_handler.py`:
- Around line 275-277: Remove the dead local assignment return_routed_experts in
decode_handler.py and clean up the related commented-out kwarg usage: delete the
return_routed_experts = getattr(...) line, remove or permanently uncomment the
commented 'return_routed_experts' kwargs at the async_generate call sites (the
calls to async_generate in this module), and ensure downstream processors
(_process_token_stream and _process_text_stream) either rely on the
engine-populated routed_experts field or are updated to not expect it; this
removes the unused variable and resolves the Ruff F841 failure while keeping the
async_generate and stream-processing flow consistent.
---
Nitpick comments:
In `@lib/llm/src/preprocessor/prompt/deepseek_v4.rs`:
- Around line 806-866: The render function injects raw tools/response_format
JSON into the system message after normalize_message_contents already ran, so
those fields skip normalization; update render (in deepseek_v4.rs) to normalize
the injected/augmented system message contents after adding
tools/response_format (e.g., call normalize_message_contents on the modified
messages_array or explicitly normalize the new system_msg object) so injected
fields receive the same normalization rules as existing messages, and add a
brief comment near the messages_array.insert(0, system_msg) noting that
inserting at index 0 shifts the ASSISTANT_START transition index.
- Around line 163-177: The helper find_last_user_index currently documents
Python's "-1" sentinel but callers re-implement the boundary logic
inconsistently; add a small helper fn at_or_after_last_user(idx: usize,
last_user_idx: Option<usize>) -> bool that returns true when last_user_idx is
None or idx >= last_user_idx (matching the intended idx >= -1 semantics), update
the three call sites that currently compare idx using > or >= to call
at_or_after_last_user instead, and keep or move the sentinel explanation to the
new helper's doc comment so the behavior is centralized and tested.
- Around line 208-222: normalize_message_contents is needlessly calling
extract_visible_text for JsonValue::String values causing an extra allocation;
update the function so it returns early for string content (leave it untouched)
and only call extract_visible_text when content.is_array() (or other non-string
types you intend to normalize), then replace the "content" key only in that
branch; reference normalize_message_contents, msg, content, and
extract_visible_text when making the change.
- Around line 147-158: The current render_tools function chains .replace on
TOOLS_TEMPLATE in an order that substitutes {tool_schemas} first which causes
subsequent .replace calls to scan user-provided schema text; update render_tools
so it first replaces the static placeholders ({dsml_token},
{thinking_start_token}, {thinking_end_token}) on TOOLS_TEMPLATE (using
tokens::DSML_TOKEN, tokens::THINKING_START, tokens::THINKING_END) and only after
that injects the user-controlled {tool_schemas} (the tools_json.join result) to
avoid scanning or altering user content; keep references to the same
TOOLS_TEMPLATE and tools_from_openai_format/to_json pipeline but swap the
replace order accordingly.
- Around line 869-1006: Add focused unit tests to cover untested branches: write
new #[test]s in the same tests module that exercise (1) rendering of messages
with role "developer" and a message having task == "action" to hit the
task/task_token branch (target encode_messages/DeepSeekV4Formatter rendering of
developer/task), (2) an assistant message with wo_eos=true to ensure the
assistant EOS suppression path is exercised (target
encode_messages/encode_messages_with_options usage), and (3) the
encode_arguments_to_dsml fallback where the arguments are a raw JSON string
(target encode_arguments_to_dsml to exercise its JSON-parse fallback). Also add
one test invoking DeepSeekV4Formatter::render with no system message to verify
tools/response_format injection path; each test should assert the specific
expected token snippets so regressions fail locally rather than relying on
external fixtures.
- Around line 93-136: The to_json function currently swallows serialization and
UTF-8 errors and returns the literal "{}"; change to_json so it surfaces
failures instead of hiding them: either (preferred) change its signature to
return Result<String, serde_json::Error> (or a suitable Box<dyn Error>) and
propagate errors from value.serialize(...) and
String::from_utf8(...).map_err(...), or if you must keep a String return, log
the error via tracing::error! including context (e.g., the JsonValue or what
operation failed) before returning a fallback; update all callers of to_json in
this module to handle the Result or accommodate the logged failure. Ensure you
modify the to_json function and call sites (references to to_json) accordingly.
In `@lib/llm/src/preprocessor/prompt/template.rs`:
- Around line 234-243: The helper is_deepseek_v3_2_non_exp currently computes
name_match eagerly yet only uses it in the None branch, causing an unnecessary
contains scan and obscuring intent; change it to compute the display_name_lower
checks (contains "deepseek", contains "v3.2", and !contains "exp") inline inside
the None arm of the match so the Some("deepseek_v3_2") and Some(_) branches
remain fast and behavior is unchanged, or alternatively update the doc comment
on is_deepseek_v3_2_non_exp to explicitly state that the "exp" suppression is
intentionally applied when model_type_lower == Some("deepseek_v3_2").
In
`@lib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_mixed_param_types.json`:
- Line 3: The fixture's expected_output is missing the terminal "finish_reason"
assertion which other sibling fixtures (e.g.,
chat_completion_stream_special_chars.json) include; update the expected_output
object to include "finish_reason": "tool_calls" alongside the existing
"normal_content", "reasoning_content", and "tool_calls" entries so the final
stream delta that emits finish_reason is validated (refer to the
"expected_output" object, the "tool_calls" array and the "call_1" entry to
locate the right fixture and place the "finish_reason" key).
In `@lib/llm/tests/deepseek_v4_encoding.rs`:
- Around line 43-76: The test helper run_official_test currently stops after
printing the first differing line because of the break inside the loop; remove
the break so the loop continues and prints all differing line pairs (or
alternatively replace the manual comparison with a call to
pretty_assertions::assert_eq!(exp, act) to get a full multiline diff); ensure
you import pretty_assertions if you choose that route and drop the manual
per-line panic so the assertion provides the failure message from assert_eq!
instead of the current panic at the end.
🪄 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: c83103aa-a3b7-4a52-b391-08db337b6d03
📒 Files selected for processing (28)
.github/workflows/copyright-check.ps1components/src/dynamo/frontend/sglang_prepost.pycomponents/src/dynamo/sglang/request_handlers/llm/decode_handler.pylib/llm/src/preprocessor.rslib/llm/src/preprocessor/prompt.rslib/llm/src/preprocessor/prompt/deepseek_v4.rslib/llm/src/preprocessor/prompt/template.rslib/llm/tests/data/deepseek-v4/test_input_1.jsonlib/llm/tests/data/deepseek-v4/test_input_2.jsonlib/llm/tests/data/deepseek-v4/test_input_3.jsonlib/llm/tests/data/deepseek-v4/test_input_4.jsonlib/llm/tests/data/deepseek-v4/test_output_1.txtlib/llm/tests/data/deepseek-v4/test_output_2.txtlib/llm/tests/data/deepseek-v4/test_output_3.txtlib/llm/tests/data/deepseek-v4/test_output_4.txtlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_content_before_tool.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_fragmented_tokens.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_mixed_param_types.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_multi_tool.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_no_tool.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_special_chars.jsonlib/llm/tests/data/vllm/deepseek-v4/chat_completion_stream_tool.jsonlib/llm/tests/deepseek_v4_encoding.rslib/llm/tests/test_streaming_tool_parsers.rslib/parsers/src/reasoning/mod.rslib/parsers/src/tool_calling/config.rslib/parsers/src/tool_calling/dsml/parser.rslib/parsers/src/tool_calling/parsers.rs
| <｜DSML｜parameter name="location" string="true">Beijing</｜DSML｜parameter> | ||
| <｜DSML｜parameter name="unit" string="true">celsius</｜DSML｜parameter> | ||
| </｜DSML｜invoke> | ||
| </｜DSML｜tool_calls><｜end▁of▁sentence｜><｜User｜><tool_result>{"temperature": 22, "condition": "sunny", "humidity": 45}</tool_result><｜Assistant｜><think>Got the weather data. Let me format a nice response.</think>The weather in Beijing is currently sunny with a temperature of 22°C and 45% humidity.<｜end▁of▁sentence｜> No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DeepSeek V4 chat template tool result role sentinel format
💡 Result:
The DeepSeek V4 chat template follows the established pattern from DeepSeek V3/V3.1 models, using special tokens for roles and tool handling. No specific V4 documentation was found, but V4 repos like deepseek-ai/DeepSeek-V4-Pro exist on Hugging Face, likely inheriting the template. Key elements from V3.1 chat_template.jinja (used in V4 as preview models build on prior architecture): Roles: - User: <｜User｜>{content} - Assistant: <｜Assistant｜>{content}<｜end▁of▁sentence｜> Tool calls (assistant role with tool_calls): - Starts with <｜tool▁calls▁begin｜> - Each tool: <｜tool▁call▁begin｜>{function_name}<｜tool▁sep｜>{arguments (JSON)}<｜tool▁call▁end｜> - Ends with <｜tool▁calls▁end｜><｜end▁of▁sentence｜> Tool results (tool role): - <｜tool▁output▁begin｜>{result content}<｜tool▁output▁end｜> System messages are prepended before the conversation. No explicit "sentinel format" or "<|tool|>" found; uses <｜tool▁...｜> tokens (fullwidth pipes). For API, uses standard OpenAI roles: system, user, assistant, tool. Example formatted prompt for tool interaction: <｜begin▁of▁sentence｜><｜User｜>What's the weather?<｜Assistant｜><｜tool▁calls▁begin｜><｜tool▁call▁begin｜>get_weather<｜tool▁sep｜>{"location":"SF"}<｜tool▁call▁end｜><｜tool▁calls▁end｜><｜end▁of▁sentence｜><｜tool▁output▁begin｜>Sunny, 72F<｜tool▁output▁end｜><｜Assistant｜>The weather in SF is sunny, 72F.<｜end▁of▁sentence｜> Confidence high for pattern match from official V3.1 sources; medium for exact V4 as docs point to inheritance.
Citations:
- 1: https://huggingface.co/nvidia/DeepSeek-V3.1-NVFP4/blob/main/assets/chat_template.jinja
- 2: https://huggingface.co/deepseek-ai/DeepSeek-V3.1/blob/main/assets/chat_template.jinja
- 3: https://huggingface.co/deepseek-ai/DeepSeek-V3.1/blob/main/tokenizer_config.json
- 4: https://huggingface.co/deepseek-ai/DeepSeek-V4-Pro
- 5: https://api-docs.deepseek.com/guides/tool_calls
🏁 Script executed:
# Check the fixture file at the specified location
if [ -f "lib/llm/tests/data/deepseek-v4/test_output_1.txt" ]; then
echo "=== File exists, checking line 36 ==="
sed -n '36p' "lib/llm/tests/data/deepseek-v4/test_output_1.txt" | head -c 500
echo ""
echo "=== Context around line 36 (lines 34-38) ==="
sed -n '34,38p' "lib/llm/tests/data/deepseek-v4/test_output_1.txt"
else
echo "File not found at specified path"
# Search for the file
find . -name "test_output_1.txt" -type f 2>/dev/null
fiRepository: ai-dynamo/dynamo
Length of output: 859
Fix tool result format in fixture to match DeepSeek V4 reference template.
The tool result on line 36 uses <｜User｜><tool_result>...</tool_result>, but the official DeepSeek V4 chat template (inherited from V3.1) specifies <｜tool▁output▁begin｜>...<｜tool▁output▁end｜>. The current fixture format will pass encoder assertions but diverges from what sglang expects, risking inference-time incompatibility.
Reference format from official template
Tool results should use:
<｜tool▁output▁begin｜>{result content}<｜tool▁output▁end｜>
instead of:
<｜User｜><tool_result>...</tool_result>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/llm/tests/data/deepseek-v4/test_output_1.txt` at line 36, Replace the
non-conforming tool output tag sequence <｜User｜><tool_result>...</tool_result>
in the fixture with the DeepSeek V4 template tags
<｜tool▁output▁begin｜>...<｜tool▁output▁end｜>; locate the string occurrence (e.g.,
the fragment containing "<｜User｜><tool_result>") and swap it to the canonical
"<｜tool▁output▁begin｜>{result content}<｜tool▁output▁end｜>" format so the fixture
matches sglang/DeepSeek V4 expectations.
There was a problem hiding this comment.
not fixing — the fixture format is actually correct per the DeepSeek V4 reference. Per `encoding_dsv4.py` (https://huggingface.co/deepseek-ai/DeepSeek-V4-Pro/blob/main/encoding/encoding_dsv4.py):
```python
tool_output_template: str = "<tool_result>{content}</tool_result>" # line 60-62
line 425: "Convert tool message to a user message with tool_result block"
```
So V4 explicitly uses `<｜User｜><tool_result>…</tool_result>`. The `<｜tool▁output▁begin｜>…<｜tool▁output▁end｜>` shape from your web query is V3.1 heritage; V4 redesigned this to ride on the `<｜User｜>` sentinel. Fixture matches the reference, sglang serving V4-Pro weights will expect this shape.
There was a problem hiding this comment.
@keivenchang, you're absolutely right — I apologize for the incorrect review comment. The confusion arose because my web search returned V3.1 template patterns, which do not apply to V4.
DeepSeek-V4-Pro does not include a Jinja-format chat template; instead, it provides a dedicated encoding folder with Python scripts (encoding_dsv4.py) as the authoritative encoding reference. Per your citation of that script, V4 explicitly redesigned tool result handling to wrap results inside a <｜User｜> sentinel using <tool_result>{content}</tool_result> — distinct from the V3.1 <｜tool▁output▁begin｜>…<｜tool▁output▁end｜> pattern I mistakenly flagged.
The fixture is correct. Withdrawing this comment.
✏️ Learnings added
Learnt from: keivenchang
Repo: ai-dynamo/dynamo PR: 8671
File: lib/llm/tests/data/deepseek-v4/test_output_1.txt:36-36
Timestamp: 2026-04-24T19:23:51.720Z
Learning: In DeepSeek V4 (deepseek-ai/DeepSeek-V4-Pro), tool results are encoded as `<｜User｜><tool_result>{content}</tool_result>` — the tool message is converted into a user message with a `<tool_result>` block. This is specified in `encoding_dsv4.py` (tool_output_template at line 60-62, conversion logic at line 425). This differs from DeepSeek V3.1 which uses `<｜tool▁output▁begin｜>…<｜tool▁output▁end｜>`. DeepSeek V4 also does not use a Jinja chat template at all; it uses `encoding_dsv4.py` as the authoritative encoding reference instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…per call
`extract_tool_calls`, `extract_invokes`, `parse_parameters` each built a
regex pattern from `format!("...{}...", regex::escape(config.field))` and
`Regex::new`'d it on every invocation. On streaming hot paths this
recompiled three regexes per chunk — independently of what the chunk
actually contained — despite the config strings being effectively fixed
per backend (V3.2 and V4 are the only variants in practice).
Introduce a `OnceLock<RwLock<HashMap<DsmlRegexKey, Arc<DsmlRegexes>>>>`
module cache keyed on the six config strings, and look up (with a shared
read lock on the fast path) before falling through to compile-and-insert.
The cache has at most two entries for the process lifetime.
`extract_invokes` and `parse_parameters` now take `&DsmlRegexes` instead
of `&DsmlParserConfig`, so the inner call tree carries compiled regexes
rather than repeatedly hashing and re-looking-up the same entry.
All 19 DSML parser unit tests still pass unchanged — the cache is a pure
behavioral no-op.
…mat!
`render_tools` built the tool-section of the system prompt by running
four sequential `String::replace` passes over `TOOLS_TEMPLATE`:
TOOLS_TEMPLATE
.replace("{tool_schemas}", ...) // allocates a fresh String
.replace("{dsml_token}", ...) // allocates again
.replace("{thinking_start_token}", ...)
.replace("{thinking_end_token}", ...)
Each `replace` allocates a new `String` sized for the whole template —
and after the first call the template already contains the inlined
schemas, so each subsequent pass copies the full (potentially kB-scale)
payload. Four tool-schema-inlined copies per render.
Collapse into one `format!` with named arguments: a single allocation
sized by the macro from the final length. Four placeholders, one pass.
The standalone `TOOLS_TEMPLATE` const is no longer referenced and is
removed — the template body now lives inside the `format!` macro where
it's used.
Byte-identical output: all four fixture-driven tests in
`tests/deepseek_v4_encoding.rs` pass unchanged, including the
tools-present fixtures that exercise every placeholder.
`merge_tool_messages` cloned every input message at loop top (`let msg = msg.clone();`) regardless of which branch handled it: - `tool` role: extracts two fields into a fresh `tool_result` JSON — never pushes `msg`. The full clone was discarded. - `user` role: extracts the text content and preserves three named fields via per-field `v.clone()` into a fresh message — never pushes the original. The full clone was discarded. - other roles: passes `msg` through into `merged`. Only this branch actually needs to own the value. On long chat histories this cloned the entire conversation's JSON tree once per turn — tool calls with large payloads paid the worst cost. Iterate by reference (`for msg in messages` already binds `&JsonValue` from a slice), drop the top-level clone, and move the clone into the pass-through `else` branch where ownership is actually needed. The two "consecutive-user merge" branches are also tightened with let-chains to avoid the `merged.last_mut().unwrap()` unwrap that was guarded by the `can_merge` flag a few lines earlier — original malformed-data behavior (silent drop rather than fall-through) is preserved with an explicit comment. All 7 deepseek_v4 unit tests and all 4 fixture-driven integration tests pass unchanged — the output JSON is byte-identical for every shape the existing suite covers.
`encode_messages_with_options` used to scan the message list twice:
1. `drop_thinking_messages` computed `find_last_user_index(&input)` at
the top of its body to decide which pre-last-user messages to drop.
2. After drop (or instead of it), the encode loop computed
`find_last_user_index(&full)` again to pass into `render_message`.
Each `find_last_user_index` is a full O(n) reverse scan of a freshly-
allocated JSON message vec. On long chat histories this is two redundant
scans per request — one when drop runs, one always.
Dedup: hoist `find_last_user_index(&full)` to the single site in
`encode_messages_with_options` and thread both its input (used by drop
to decide) and its *output* (computed inline while iterating) through
`drop_thinking_messages`:
- Caller computes `let mut last_user_idx = find_last_user_index(&full)`
once on the post-merge/sort list.
- `drop_thinking_messages(full, last_user_idx)` now returns
`(Vec<JsonValue>, Option<usize>)` — the dropped list plus the
post-drop position of the same "last user/developer" message,
tracked as `out.len()` at the moment it's pushed.
- The encode loop reads the returned index directly — no rescan.
The post-drop index is mathematically equivalent to a full
`find_last_user_index(&dropped)` rescan: `drop_thinking_messages`
never drops or reorders the message at `last_user_idx`, and every
surviving message before it counts toward the new position by 1. Three
new unit tests pin this equivalence for the three cases that matter:
- `test_drop_thinking_messages_returns_post_drop_last_user_idx` —
shape where a pre-last-user `developer` is dropped, forcing the
trailing developer to shift from idx 3 to idx 2. Asserts the
returned index equals a full rescan.
- `test_drop_thinking_messages_preserves_idx_when_nothing_dropped` —
no-shift sanity: returned idx equals the input idx.
- `test_drop_thinking_messages_no_user_in_history` — edge case where
both pre- and post-drop indices are `None`.
All 10 deepseek_v4 unit tests and all 4 fixture-driven integration
tests pass unchanged.
Focused pass over the DeepSeek-V4 formatter + reasoning registration. No behavior changes are intended for any fixture-covered input; every existing unit and integration test in `deepseek_v4` and `reasoning` passes unchanged. All items come from the review of 88478b4..HEAD. Addressed: - Smell 4 / idiomaticity 4 — `to_json` now returns `Result<String>` instead of silently collapsing serialization / UTF-8 errors to `"{}"`. Cascaded through `render_tools`, `extract_visible_text`, `normalize_message_contents`, `render_tool_result_content`, the two `render_message` tool/response-format sites, `encode_arguments_to_dsml`, and the `impl OAIPromptFormatter::render` entry point. Errors now surface as `anyhow::Error` with context instead of corrupting the prompt. - Smell 7 — `value.as_str().unwrap().to_string()` (guarded a few lines earlier by `.is_string()`) replaced with a `match value { JsonValue:: String(s) => ... }` pattern so the invariant holds at the type level rather than by convention. - Smell 8 — magic `&["user","system","tool","latest_reminder", "direct_search_results"]` list inside `drop_thinking_messages` hoisted to module-scope `const KEEP_ROLES: &[&str]` with a doc comment explaining what the list means. - Smell 9 — `"[Unsupported {}]"` literal injected into user-visible prompts now goes through a shared `UNSUPPORTED_PLACEHOLDER_FMT` constant and emits a `tracing::warn!` at both call sites (user-content blocks and tool_result items), matching the existing `extract_visible_text` drop-path warn. - Smell 10 — `ReasoningParserType::DeepSeekV4` dedicated variant added, replacing the silent `Qwen` alias. Verified against deepseek-ai/DeepSeek-V4-Pro/encoding/encoding_dsv4.py: V4 uses the same `<think>` / `</think>` delimiters as Qwen today, so the new variant's match arm points at the same `BasicReasoningParser` config — but future V4-specific divergence (max-thinking mode, different tokens) now has a place to land without rippling through Qwen. - Smell 11 — the three-alias entries (`deepseek_v4` / `deepseek-v4` / `deepseekv4`) in the reasoning parser map now carry an inline comment explaining why (callers wire the name through from heterogeneous sources: `--dyn-reasoning-parser` flag, vLLM recipes, chat-template authors; accepting all three avoids a canonical-form requirement). - Smell 12 — `RESPONSE_FORMAT_TEMPLATE` const + `replace("{schema}", ..)` collapsed into a direct `format!("## Response Format:...{}", schema)` at the two system/developer call sites. One fewer indirection, one fewer allocation per render. - Smell 13 — `TOOL_CALLS_BLOCK_NAME` const now has a docstring explaining its role (one DSML tag is extracted; the others are inline because they appear exactly once). The summary's alternative — also constifying `INVOKE_BLOCK_NAME` / `PARAMETER_BLOCK_NAME` — would introduce consts used in exactly one place each, net-negative for readability. Deferred (tracked for follow-up commits): - Smell 2 (V3.2/V4 helper duplication). The V4 versions of `to_json`, `extract_visible_text`, `normalize_message_contents`, etc. are not byte-identical to V3.2 — V4's `to_json` uses `PythonFormatter` while V3.2 uses a character-scan that has a known escape-handling bug. Extracting a shared module would change V3.2 behavior; needs its own commit with V3.2 regression coverage first. - Smell 3 (`render_message` split into `render_assistant_body` / `render_user_body` / `append_transition_token`). Worth doing, but mechanically sizable; keeping this commit focused. - Smell 5 (test move: keep invariants in-module, move scenarios to integration). Opinionated; deferred. Tests: 10 unit + 4 integration + 91 parsers reasoning tests green.
Pure-style pass over the DeepSeek-V4 formatter. No behavior changes; all
10 unit + 4 integration tests still green.
- Idiom 1: `find_last_user_index` role matcher
`.map(|r| r == "user" || r == "developer").unwrap_or(false)`
→ `.is_some_and(|r| matches!(r, "user" | "developer"))`
(closer to the `Python` `is_some_and` sentinel semantics the file uses
elsewhere for last_user_idx comparisons.)
- Idiom 2: `resolve_thinking_mode` nested-`if let` over the same `args`
collapsed with 2024-edition let-chains; the outer scope `args` is
matched twice but it's a cheap reference comparison and the flatter
shape matches the rest of the file (e.g. the `can_merge` branches in
`merge_tool_messages` already use this form).
- Idiom 3: `.and_then(|v| v.as_str())` / `.as_array()` / `.as_bool()` /
`.as_array_mut()` closures across ~30 sites replaced with
`JsonValue::as_str` / `as_array` / `as_bool` / `as_array_mut` fn
pointers. No closure allocation, one less turbofish-free indirection.
- Idiom 5: `anyhow::bail!("Unknown role: {}", other)` →
`anyhow::bail!("Unknown role: {other}")` (2024-edition captured
identifier; matches `"{text_so_far}"` / `"{idx}"` usage elsewhere in
the file).
- Idiom 6: `#[inline]` on `ThinkingMode::as_str` (two-branch match
returning `&'static str`) and `task_token` (seven-branch match
returning `&'static str`). Both are on the per-message render hot path
and smaller than the call overhead; inlining lets LLVM fold them into
caller string-equality checks.
Deferred from the idiomaticity list:
- Idiom 4 (`to_json` → `Result<String>`) — already landed in the
previous cleanup commit alongside Smell 4.
- Idiom 7 (`extract_visible_text` → `Cow<'_, str>`). The text-only path
already avoids re-allocating (`text.clone()` is a `String::clone`,
which is still O(n) but unavoidable without a `Cow`). Wrapping in
`Cow<'_, str>` would change the return type through
`normalize_message_contents` / `render` ripple and intermittently save
one clone per message. Worth its own focused commit with a benchmark.
Tests: 10 unit + 4 integration, all green.
Signed-off-by: ayushag <ayushag@nvidia.com>
Introduces a `CASE.<N>` / `CASE.<family>.<N>` taxonomy for parser test
categories:
- new `lib/parsers/README.md` — crate landing page (parser families,
request-flow diagram, how-to-add-a-parser)
- new `lib/parsers/TESTING.md` — 16 generic + 2 XML-family + 1
Harmony categories, with per-category definitions, applicability,
and example anchors
Adds 4 DSML pinning tests that document current behavior on truncated
V4 output:
- `CASE.5` missing outer `</｜DSML｜tool_calls>` fence — whole block
silently dropped (same structural class as Kimi K2 pre-recovery)
- `CASE.4` missing inner `</｜DSML｜invoke>` fence — call silently
dropped
- `CASE.4` malformed JSON in `string="false"` param — falls back to
raw string (intentional, pins the `unwrap_or_else` behavior)
Annotates every existing V4 test with a `/// CASE.<N>` doc comment
naming the category it pins, across 4 files (dsml parser tests,
parsers registry, reasoning registry, streaming e2e). Adds a coverage
manifest block above the V4 test section listing what's still NOT
covered so future contributors see the gaps without reading the full
test file:
- `CASE.5` mid-stream truncation recovery (TODO)
- `CASE.4` parameter-close and middle-invoke-bleed variants
- `CASE.11` tool_choice auto/required/named/none
- `CASE.12` `FinishReason::Length`
- `CASE.14` empty-content / null at e2e
- `CASE.15` duplicate calls (universal gap)
- `CASE.16` regression (V4 is brand new, no bugs filed yet)
No production code changes. All 389 dynamo-parsers tests + 4 V4
encoding golden tests + 24 V4 streaming tests pass.
Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
…t const strings Addresses biswapanda's review comments on #8665: - L389 "split into separate funcs" — render_message's 221-line match is split into six per-role helpers: render_system_role, render_developer_role, render_user_role, render_latest_reminder_role, render_assistant_role, plus append_response_format / append_tools_section to dedup the sys/developer response-format and tools blocks. render_message itself stays as a slim dispatcher that handles the reasoning-effort prefix and the transition-token tail. - L400 "const strings" — hard-coded `<tool_result>` / `</tool_result>` tags extracted to TOOL_RESULT_OPEN / TOOL_RESULT_CLOSE consts alongside the other wire-format constants. The response-format preamble (previously duplicated inline in the system and developer branches) becomes RESPONSE_FORMAT_PREAMBLE with a {} placeholder. Already addressed by prior commits on the branch (no-op here): - L32 "Should be ReasoningParserType::DeepSeekV4" — the three deepseek_v4 aliases now map to ReasoningParserType::DeepSeekV4. - L899 "hard coded path" — the `/home/ayush-lab/...` absolute path in the test has been removed. - L266 "clippy manual_pattern_char_comparison" — fixed in 77602e4 by using a char array. Pure refactor — behavior is identical. All 4 V4 encoding golden tests + 7 V4 streaming tests pass unchanged. Clippy clean on dynamo-llm. Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
9d3c913 to
a627a21
Compare
|
Picking this up from @ishandhanani. Commit Short version of what was wrong: sglang 0.5.10.post1 didn't drop either kwarg. Ishan was looking in the right place but the diagnosis wasn't quite right. Two other CR items resolve without me doing anything: the Everything else in CR's review — the DSML escaping, the silent Base is /cc @ishandhanani |
keivenchang
changed the title
The fixture's expected_output carried a finish_reason field that the streaming test harness never reads — finish_reason validation is hardcoded against the stream chunks themselves, not the expected_output struct. None of the other 6 deepseek-v4 fixtures have it. Drop it for consistency. Addresses CodeRabbit comment 2 on #8665. Signed-off-by: Keiven Chang <keivenc@nvidia.com> Signed-off-by: Keiven Chang <keivenchang@users.noreply.github.com>
|
@CodeRabbit can you verify that the fixes/updates addressed your concerns? If so, you can Resolve Conversation. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10899
All addressed concerns are resolved. Resolving conversations now. [resolve] |
a627a21 to
8aa616e
Compare
8aa616e to
d8b2e8c
Compare
d8b2e8c to
44fb33a
Compare
keivenchang
changed the title
|
Reverted Backing out to just Ishan's original Next steps (not doing now):
|
2aab621 to
8411f3e
Compare
github-actions
Bot
added
the
documentation
|
@ishandhanani just want to confirm with you |
4 participants
Status: INVESTIGATE — do not merge
Stacked on #8665 (DeepSeek V4 parser). One commit on top — Ishan's original
investigate tmrw— kept as-is.What's wrong
When dynamo runs against a live sglang + DSv4-Pro stack, two kwargs have to be commented out for things to work at all:
sglang_prepost.py:178—parallel_tool_callsonFunctionCallParser.get_structure_constraint().decode_handler.py:311, 349—return_routed_expertson bothEngine.async_generate()call sites.Signature inspection on
sglang==0.5.10.post1says both kwargs are accepted:So "sglang dropped the kwarg" isn't the story. I tried a patch that restored both (with a
None → Truenormalization forparallel_tool_calls) and it broke against a real sglang + DSv4 run — that patch has been reverted (d8b2e8ce2b1). The commented-out workaround is the current known-working state.What needs diagnosing
parallel_tool_calls=Trueintoget_structure_constraint()for the DSv4 parser: where does it actually fail? xgrammar structural-tag construction? Grammar compile? Downstream generation?return_routed_experts=Trueintoasync_generate(): rejected at sglang level, or downstream? Does it depend on whether MoE routed-experts are actually enabled in server_args?Repro steps (TBD)
deepseek-ai/DeepSeek-V4-Provia sglang 0.5.10.post1 with a DSv4-compatible tool-call parser (fallbackdeepseek_v31until sglang registersdeepseek_v4).tool_choice="auto"tool-calling request through dynamo's sglang frontend withparallel_tool_callsuncommented — capture the actual failure mode (traceback, sglang log, xgrammar error).return_routed_expertswithenable_return_routed_experts=Truein server_args.Related