From 285cbce6bd0982f5936d729d3fa2ac310dfeca59 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Thu, 10 Sep 2026 11:19:30 +1000 Subject: [PATCH] [Rust Frontend] Resolve unified and split parser selections consistently (#56018) Signed-off-by: Bugen Zhao --- rust/src/chat/src/error.rs | 4 ++ rust/src/chat/src/lib.rs | 48 +------------ rust/src/chat/src/output/default/mod.rs | 74 ++++++++++++--------- rust/src/chat/src/parser/mod.rs | 64 ++++++++++++++++++ rust/src/chat/src/parser/reasoning/mod.rs | 24 ------- rust/src/chat/src/parser/reasoning/tests.rs | 2 - rust/src/chat/src/parser/tool/mod.rs | 24 ------- rust/src/chat/src/parser/tool/tests.rs | 12 ---- rust/src/cmd/src/cli.rs | 10 +-- rust/src/cmd/src/cli/tests.rs | 15 +++++ 10 files changed, 129 insertions(+), 148 deletions(-) diff --git a/rust/src/chat/src/error.rs b/rust/src/chat/src/error.rs index 80e3a7f074..082e665dbb 100644 --- a/rust/src/chat/src/error.rs +++ b/rust/src/chat/src/error.rs @@ -34,6 +34,10 @@ pub enum Error { }, #[error("{kind} parsing is disabled by frontend configuration")] ParserDisabled { kind: &'static str }, + #[error( + "unified parsing requires the tool and reasoning selections to resolve to the same parser; resolved tool={tool}, reasoning={reasoning}" + )] + IncompatibleParserSelections { tool: String, reasoning: String }, #[error( "{kind} parser `{name}` is not registered{}", available_parser_hint(.available_names) diff --git a/rust/src/chat/src/lib.rs b/rust/src/chat/src/lib.rs index 756a1851ab..6bb87cfec4 100644 --- a/rust/src/chat/src/lib.rs +++ b/rust/src/chat/src/lib.rs @@ -26,11 +26,11 @@ pub use output::{ ChatOutputProcessor, DefaultChatOutputProcessor, DynChatOutputProcessor, HarmonyChatOutputProcessor, }; -pub use parser::ParserSelection; pub use parser::reasoning::{ ReasoningDelta, ReasoningError, ReasoningParser, ReasoningParserFactory, }; pub use parser::tool::{ToolParser, ToolParserError, ToolParserFactory}; +pub use parser::{ParserSelection, validate_parser_overrides}; pub use renderer::hf::ChatTemplateContentFormatOption; pub use renderer::{ ChatRenderer, DeepSeekV4ChatRenderer, DeepSeekV32ChatRenderer, DynChatRenderer, @@ -62,36 +62,6 @@ use vllm_engine_core_client::protocol::request::ReasoningParserKwargs; use vllm_llm::Llm; use vllm_text::{Prompt, TextLlm, TextRequest}; -/// Validate explicit parser override names without starting request processing. -pub fn validate_parser_overrides( - tool_call_parser: &ParserSelection, - reasoning_parser: &ParserSelection, -) -> Result<()> { - let tool_parser_factory = ToolParserFactory::global(); - if let ParserSelection::Explicit(name) = tool_call_parser - && !tool_parser_factory.contains(name) - { - return Err(Error::ParserUnavailableByName { - kind: "tool", - name: name.clone(), - available_names: tool_parser_factory.list(), - }); - } - - let reasoning_parser_factory = ReasoningParserFactory::global(); - if let ParserSelection::Explicit(name) = reasoning_parser - && !reasoning_parser_factory.contains(name) - { - return Err(Error::ParserUnavailableByName { - kind: "reasoning", - name: name.clone(), - available_names: reasoning_parser_factory.list(), - }); - } - - Ok(()) -} - /// Chat request preparation shared by inference and render-only frontends. pub struct ChatRequestProcessor { backend: DynChatBackend, @@ -326,24 +296,12 @@ impl ChatLlm { /// Effective tool-call parser name for this model, if parsing is enabled. pub fn tool_call_parser_name(&self) -> Option<&str> { - match &self.processor.tool_call_parser { - ParserSelection::Auto => { - ToolParserFactory::global().resolve_name_for_model(self.model_id()) - } - ParserSelection::None => None, - ParserSelection::Explicit(name) => Some(name), - } + self.processor.tool_call_parser.resolve_tool_name(self.model_id()) } /// Effective reasoning parser name for this model, if parsing is enabled. pub fn reasoning_parser_name(&self) -> Option<&str> { - match &self.processor.reasoning_parser { - ParserSelection::Auto => { - ReasoningParserFactory::global().resolve_name_for_model(self.model_id()) - } - ParserSelection::None => None, - ParserSelection::Explicit(name) => Some(name), - } + self.processor.reasoning_parser.resolve_reasoning_name(self.model_id()) } /// Render, tokenize, and submit one chat request. diff --git a/rust/src/chat/src/output/default/mod.rs b/rust/src/chat/src/output/default/mod.rs index 7c983fefd1..a731153063 100644 --- a/rust/src/chat/src/output/default/mod.rs +++ b/rust/src/chat/src/output/default/mod.rs @@ -50,13 +50,12 @@ impl DefaultChatOutputProcessor { tool_call_parser: &ParserSelection, reasoning_parser: &ParserSelection, ) -> ChatResult { - let parser = if tool_call_parser == reasoning_parser - && let Some(parser) = Self::resolve_optional_unified_parser( - request.tools(), - model_id, - tokenizer.clone(), - tool_call_parser, - )? { + let parser = if let Some(parser) = Self::resolve_optional_unified_parser( + request.tools(), + tokenizer.clone(), + tool_call_parser.resolve_tool_name(model_id), + reasoning_parser.resolve_reasoning_name(model_id), + )? { parser } else { let tool_parsing_enabled = request.tool_parsing_enabled(); @@ -105,7 +104,7 @@ impl DefaultChatOutputProcessor { ) -> ChatResult> { let factory = ToolParserFactory::global(); let parser_name = match selection { - ParserSelection::Auto => factory.resolve_name_for_model(model_id).ok_or_else(|| { + ParserSelection::Auto => selection.resolve_tool_name(model_id).ok_or_else(|| { Error::ParserUnavailableForModel { kind: "tool", model_id: model_id.to_string(), @@ -123,21 +122,22 @@ impl DefaultChatOutputProcessor { fn resolve_optional_unified_parser( tools: &[ChatTool], - model_id: &str, tokenizer: DynTokenizer, - selection: &ParserSelection, + tool_name: Option<&str>, + reasoning_name: Option<&str>, ) -> ChatResult>> { let factory = UnifiedParserFactory::global(); - let parser_name = match selection { - ParserSelection::Auto => factory.resolve_name_for_model(model_id), - ParserSelection::None => None, - ParserSelection::Explicit(name) if factory.contains(name) => Some(name.as_str()), - ParserSelection::Explicit(_) => None, - }; - - let Some(parser_name) = parser_name else { + let Some(parser_name) = + tool_name.into_iter().chain(reasoning_name).find(|name| factory.contains(name)) + else { return Ok(None); }; + if tool_name != reasoning_name { + return Err(Error::IncompatibleParserSelections { + tool: tool_name.unwrap_or("none").to_owned(), + reasoning: reasoning_name.unwrap_or("none").to_owned(), + }); + } let parser = factory.create(parser_name, tools, tokenizer)?; @@ -151,11 +151,7 @@ impl DefaultChatOutputProcessor { selection: &ParserSelection, ) -> ChatResult>> { let factory = ReasoningParserFactory::global(); - let parser_name = match selection { - ParserSelection::Auto => factory.resolve_name_for_model(model_id), - ParserSelection::None => None, - ParserSelection::Explicit(name) => Some(name.as_str()), - }; + let parser_name = selection.resolve_reasoning_name(model_id); let Some(parser_name) = parser_name else { REASONING_PARSER_LOG_ONCE.call_once(|| info!("reasoning parsing disabled")); @@ -195,7 +191,6 @@ mod tests { use vllm_tokenizer::test_utils::TestTokenizer; use super::DefaultChatOutputProcessor; - use crate::Error; use crate::parser::ParserSelection; use crate::request::ChatRequest; @@ -237,11 +232,29 @@ mod tests { } #[test] - fn mixed_gemma4_selection_uses_split_dummy_error() { + fn auto_and_explicit_gemma4_selections_use_unified_parser() { + let explicit = ParserSelection::Explicit("gemma4".to_string()); + for (tool, reasoning) in [ + (&ParserSelection::Auto, &explicit), + (&explicit, &ParserSelection::Auto), + ] { + DefaultChatOutputProcessor::new( + &mut ChatRequest::for_test(), + "google/gemma-4-27b-it", + tokenizer(), + tool, + reasoning, + ) + .unwrap(); + } + } + + #[test] + fn conflicting_unified_parser_selections_report_resolved_names() { let mut request = ChatRequest::for_test(); let error = match DefaultChatOutputProcessor::new( &mut request, - "other-model", + "Qwen/Qwen3-8B", tokenizer(), &ParserSelection::Auto, &ParserSelection::Explicit("gemma4".to_string()), @@ -250,12 +263,7 @@ mod tests { Err(error) => error, }; - let Error::ParserInitialization { error, .. } = error else { - panic!("expected parser initialization error"); - }; - assert_eq!( - error.to_string(), - "`gemma4` only provides a unified parser; the same reasoning parser and tool parser should be specified together" - ); + expect_test::expect!["unified parsing requires the tool and reasoning selections to resolve to the same parser; resolved tool=qwen3_xml, reasoning=gemma4"] + .assert_eq(&format!("{error}")); } } diff --git a/rust/src/chat/src/parser/mod.rs b/rust/src/chat/src/parser/mod.rs index 7684aa9d97..c10a4f11bf 100644 --- a/rust/src/chat/src/parser/mod.rs +++ b/rust/src/chat/src/parser/mod.rs @@ -12,6 +12,10 @@ use std::str::FromStr; use serde_with::{DeserializeFromStr, SerializeDisplay}; +use self::reasoning::ReasoningParserFactory; +use self::tool::ToolParserFactory; +use self::unified::UnifiedParserFactory; + /// Specify which reasoning or tool-call parser implementation to use. #[derive(Debug, Clone, PartialEq, Eq, Default, DeserializeFromStr, SerializeDisplay)] pub enum ParserSelection { @@ -27,6 +31,66 @@ pub enum ParserSelection { impl ParserSelection { pub const AUTO_LITERAL: &str = "auto"; pub const NONE_LITERAL: &str = "none"; + + /// Resolve a reasoning parser name, preferring a matching unified parser. + pub fn resolve_reasoning_name(&self, model_id: &str) -> Option<&str> { + self.resolve_name(model_id, ReasoningParserFactory::global()) + } + + /// Resolve a tool parser name, preferring a matching unified parser. + pub fn resolve_tool_name(&self, model_id: &str) -> Option<&str> { + self.resolve_name(model_id, ToolParserFactory::global()) + } + + fn resolve_name<'a, C>( + &'a self, + model_id: &str, + factory: &'a ParserFactory, + ) -> Option<&'a str> { + match self { + Self::Auto => UnifiedParserFactory::global() + .resolve_name_for_model(model_id) + .or_else(|| factory.resolve_name_for_model(model_id)), + Self::None => None, + Self::Explicit(name) => Some(name), + } + } +} + +/// Validate explicit parser override names without starting request processing. +pub fn validate_parser_overrides( + tool_call_parser: &ParserSelection, + reasoning_parser: &ParserSelection, +) -> crate::Result<()> { + validate_selection(tool_call_parser, "tool", ToolParserFactory::global())?; + validate_selection( + reasoning_parser, + "reasoning", + ReasoningParserFactory::global(), + ) +} + +fn validate_selection( + selection: &ParserSelection, + kind: &'static str, + factory: &ParserFactory, +) -> crate::Result<()> { + let unified = UnifiedParserFactory::global(); + if let ParserSelection::Explicit(name) = selection + && !factory.contains(name) + && !unified.contains(name) + { + let mut available_names = factory.list(); + available_names.extend(unified.list()); + available_names.sort_unstable(); + available_names.dedup(); + return Err(crate::Error::ParserUnavailableByName { + kind, + name: name.clone(), + available_names, + }); + } + Ok(()) } impl FromStr for ParserSelection { diff --git a/rust/src/chat/src/parser/reasoning/mod.rs b/rust/src/chat/src/parser/reasoning/mod.rs index 461e34b22f..e37e92054a 100644 --- a/rust/src/chat/src/parser/reasoning/mod.rs +++ b/rust/src/chat/src/parser/reasoning/mod.rs @@ -22,15 +22,10 @@ pub mod names { pub const DEEPSEEK_R1: &str = "deepseek_r1"; pub const DEEPSEEK_V3: &str = "deepseek_v3"; pub const DEEPSEEK_V4: &str = "deepseek_v4"; - pub const GEMMA4: &str = "gemma4"; - pub const INKLING: &str = "inkling"; pub const GLM45: &str = "glm45"; pub const GLM47: &str = "glm47"; - pub const HY_V3: &str = "hy_v3"; - pub const HY_V4: &str = "hy_v4"; pub const KIMI: &str = "kimi"; pub const KIMI_K2: &str = "kimi_k2"; - pub const KIMI_K3: &str = "kimi_k3"; pub const MINIMAX_M2: &str = "minimax_m2"; pub const MINIMAX_M3: &str = "minimax_m3"; pub const NEMOTRON_V3: &str = "nemotron_v3"; @@ -67,15 +62,10 @@ impl ReasoningParserFactory { .register_parser::(names::DEEPSEEK_R1) .register_parser::(names::DEEPSEEK_V3) .register_parser::(names::DEEPSEEK_V4) - .register_unified_dummy(names::GEMMA4) - .register_unified_dummy(names::INKLING) .register_parser::(names::GLM45) .register_parser::(names::GLM47) - .register_unified_dummy(names::HY_V3) - .register_unified_dummy(names::HY_V4) .register_parser::(names::KIMI) .register_parser::(names::KIMI_K2) - .register_unified_dummy(names::KIMI_K3) .register_parser::(names::MINIMAX_M2) .register_parser::(names::MINIMAX_M3) .register_parser::(names::NEMOTRON_V3) @@ -89,16 +79,12 @@ impl ReasoningParserFactory { .register_pattern("deepseek-v4", names::DEEPSEEK_V4) .register_pattern("deepseek_v4", names::DEEPSEEK_V4) .register_pattern("deepseek-v3", names::DEEPSEEK_V3) - .register_pattern("gemma-4", names::GEMMA4) - .register_pattern("gemma4", names::GEMMA4) .register_pattern("qwq", names::DEEPSEEK_R1) .register_pattern("qwen3", names::QWEN3) .register_pattern("glm-5", names::GLM47) .register_pattern("glm-4.7", names::GLM47) .register_pattern("glm-4.6", names::GLM45) .register_pattern("glm-4.5", names::GLM45) - .register_pattern("hy3", names::HY_V3) - .register_pattern("hy4", names::HY_V4) .register_pattern("kimi-k2", names::KIMI_K2) .register_pattern("kimi", names::KIMI) // step3p5 patterns must precede `step3`: substring matching would @@ -129,16 +115,6 @@ impl ReasoningParserFactory { self.register_creator(name, Arc::new(T::create)) } - /// Register one unified-only parser name in the split reasoning registry. - pub fn register_unified_dummy(&mut self, name: &str) -> &mut Self { - let name = name.to_string(); - let registered_name = name.clone(); - self.register_creator( - ®istered_name, - Arc::new(move |_| Err(ReasoningError::DummyUnifiedParser { name: name.clone() })), - ) - } - /// Construct a parser from an exact name. pub fn create( &self, diff --git a/rust/src/chat/src/parser/reasoning/tests.rs b/rust/src/chat/src/parser/reasoning/tests.rs index e392f40679..f2db9b08c7 100644 --- a/rust/src/chat/src/parser/reasoning/tests.rs +++ b/rust/src/chat/src/parser/reasoning/tests.rs @@ -15,13 +15,11 @@ fn factory_contains_and_lists_registered_parsers() { assert!(factory.contains(names::SEED_OSS)); assert!(factory.contains(names::STEP3P5)); assert!(factory.contains(names::MINIMAX_M3)); - assert!(factory.contains(names::GEMMA4)); assert!(factory.list().contains(&names::QWEN3.to_string())); assert!(factory.list().contains(&names::DEEPSEEK_V4.to_string())); assert!(factory.list().contains(&names::SEED_OSS.to_string())); assert!(factory.list().contains(&names::STEP3P5.to_string())); assert!(factory.list().contains(&names::MINIMAX_M3.to_string())); - assert!(factory.list().contains(&names::GEMMA4.to_string())); } #[test] diff --git a/rust/src/chat/src/parser/tool/mod.rs b/rust/src/chat/src/parser/tool/mod.rs index d64a49b2ec..bf76a4b315 100644 --- a/rust/src/chat/src/parser/tool/mod.rs +++ b/rust/src/chat/src/parser/tool/mod.rs @@ -24,17 +24,12 @@ pub mod names { pub const DEEPSEEK_V4: &str = "deepseek_v4"; pub const GLM45: &str = "glm45"; pub const GLM47: &str = "glm47"; - pub const GEMMA4: &str = "gemma4"; - pub const INKLING: &str = "inkling"; pub const GRANITE4: &str = "granite4"; pub const HERMES: &str = "hermes"; - pub const HY_V3: &str = "hy_v3"; - pub const HY_V4: &str = "hy_v4"; // Matches the Python CLI name `--tool-call-parser internlm`, which Python // also routes to `Internlm2ToolParser` despite the version-agnostic name. pub const INTERNLM: &str = "internlm"; pub const KIMI_K2: &str = "kimi_k2"; - pub const KIMI_K3: &str = "kimi_k3"; pub const LLAMA3_JSON: &str = "llama3_json"; pub const LLAMA4_JSON: &str = "llama4_json"; pub const MINIMAX_M2: &str = "minimax_m2"; @@ -73,15 +68,10 @@ impl ToolParserFactory { .register_parser::(names::DEEPSEEK_V4) .register_parser::(names::GLM45) .register_parser::(names::GLM47) - .register_unified_dummy(names::GEMMA4) - .register_unified_dummy(names::INKLING) .register_parser::(names::GRANITE4) .register_parser::(names::HERMES) - .register_unified_dummy(names::HY_V3) - .register_unified_dummy(names::HY_V4) .register_parser::(names::INTERNLM) .register_parser::(names::KIMI_K2) - .register_unified_dummy(names::KIMI_K3) .register_parser::(names::LLAMA3_JSON) .register_parser::(names::LLAMA4_JSON) .register_parser::(names::MINIMAX_M2) @@ -101,8 +91,6 @@ impl ToolParserFactory { .register_pattern("qwen2.5", names::HERMES) .register_pattern("qwen3", names::QWEN3_XML) .register_pattern("hermes", names::HERMES) - .register_pattern("hy3", names::HY_V3) - .register_pattern("hy4", names::HY_V4) // Narrow to `internlm2` substring so it matches `internlm2-chat-7b` // and `internlm2_5-7b-chat` but NOT `internlm-chat-7b` (InternLM v1, // routes to Llama), `internlm3-*` (also Llama-architecture per @@ -122,8 +110,6 @@ impl ToolParserFactory { .register_pattern("glm-4.7", names::GLM47) .register_pattern("glm-4.6", names::GLM45) .register_pattern("glm-4.5", names::GLM45) - .register_pattern("gemma4", names::GEMMA4) - .register_pattern("gemma-4", names::GEMMA4) .register_pattern("granite-4", names::GRANITE4) .register_pattern("kimi-k2", names::KIMI_K2) .register_pattern("minimax-m3", names::MINIMAX_M3) @@ -144,16 +130,6 @@ impl ToolParserFactory { self.register_creator(name, Arc::new(T::create)) } - /// Register one unified-only parser name in the split tool registry. - pub fn register_unified_dummy(&mut self, name: &str) -> &mut Self { - let name = name.to_string(); - let registered_name = name.clone(); - self.register_creator( - ®istered_name, - Arc::new(move |_| Err(ToolParserError::DummyUnifiedParser { name: name.clone() })), - ) - } - /// Construct a parser from an exact name. pub fn create(&self, name: &str, tools: &[ChatTool]) -> crate::Result> { let creator = self.creator(name).ok_or_else(|| crate::Error::ParserUnavailableByName { diff --git a/rust/src/chat/src/parser/tool/tests.rs b/rust/src/chat/src/parser/tool/tests.rs index 5c7ee3d85e..381a3bbf3b 100644 --- a/rust/src/chat/src/parser/tool/tests.rs +++ b/rust/src/chat/src/parser/tool/tests.rs @@ -167,10 +167,6 @@ fn factory_new_resolves_default_patterns() { factory.resolve_name_for_model("glm-4.7"), Some(names::GLM47) ); - assert_eq!( - factory.resolve_name_for_model("google/gemma-4-27b-it"), - Some(names::GEMMA4) - ); assert_eq!( factory.resolve_name_for_model("ibm-granite/granite-4.0-h-tiny"), Some(names::GRANITE4) @@ -179,14 +175,6 @@ fn factory_new_resolves_default_patterns() { factory.resolve_name_for_model("NousResearch/Hermes-3-Llama-3.1-8B"), Some(names::HERMES) ); - assert_eq!( - factory.resolve_name_for_model("tencent/Hy3-preview"), - Some(names::HY_V3) - ); - assert_eq!( - factory.resolve_name_for_model("tencent/Hy4-preview"), - Some(names::HY_V4) - ); assert_eq!( factory.resolve_name_for_model("MiniMax/MiniMax-M3-Text"), Some(names::MINIMAX_M3) diff --git a/rust/src/cmd/src/cli.rs b/rust/src/cmd/src/cli.rs index 55374c3f29..c0b4c6d295 100644 --- a/rust/src/cmd/src/cli.rs +++ b/rust/src/cmd/src/cli.rs @@ -23,8 +23,8 @@ use serde_json::Value; use serde_with::{DefaultOnNull, OneOrMany, serde_as}; use thiserror_ext::AsReport as _; use uuid::Uuid; +use vllm_chat::GenerationConfigMode; use vllm_chat::multimodal::MmLimitPerPrompt; -use vllm_chat::{GenerationConfigMode, ReasoningParserFactory}; use vllm_engine_core_client::TransportMode; use vllm_managed_engine::ManagedEngineConfig; use vllm_managed_engine::cli::{ManagedEngineArgs, repartition_managed_engine_args}; @@ -756,13 +756,7 @@ impl ServeArgs { } fn effective_engine_reasoning_parser(selection: &ParserSelection, model: &str) -> Option { - match selection { - ParserSelection::Auto => ReasoningParserFactory::global() - .resolve_name_for_model(model) - .map(str::to_string), - ParserSelection::None => None, - ParserSelection::Explicit(name) => Some(name.clone()), - } + selection.resolve_reasoning_name(model).map(str::to_owned) } /// Allocate fresh IPC endpoints for one managed frontend instance. diff --git a/rust/src/cmd/src/cli/tests.rs b/rust/src/cmd/src/cli/tests.rs index 0ab6882f34..2474a1e142 100644 --- a/rust/src/cmd/src/cli/tests.rs +++ b/rust/src/cmd/src/cli/tests.rs @@ -413,6 +413,21 @@ fn serve_args_resolve_auto_reasoning_parser_for_managed_engine() { .assert_debug_eq(&config.python_args); } +#[test] +fn serve_args_resolve_unified_reasoning_parser_for_managed_engine() { + let cli = Cli::try_parse_from(["vllm-rs", "serve", "moonshotai/Kimi-K3"]).unwrap(); + let Command::Serve(args) = cli.command else { + panic!("expected serve args"); + }; + expect![[r#" + [ + "--reasoning-parser", + "kimi_k3", + ] + "#]] + .assert_debug_eq(&args.to_managed_engine_config(5555).python_args); +} + #[test] fn serve_args_forward_explicit_reasoning_parser_to_managed_engine() { let cli = Cli::try_parse_from([