Skip to content

GitLab

Unverified Commit ffba61a1 authored by Chang Su's avatar Chang Su Committed by GitHub
Browse files

[router][grpc] Make harmony parser checks recipient first before channel (#12713)

parent 3c219eb0
Hide whitespace changes
Inline Side-by-side
Showing with 76 additions and 55 deletions
sgl-router/src/routers/grpc/harmony/parser.rs
View file @ ffba61a1
...@@ -106,7 +106,7 @@ impl HarmonyParserAdapter { ...@@ -106,7 +106,7 @@ impl HarmonyParserAdapter {
pub fn parse_messages( pub fn parse_messages(
messages: &[openai_harmony::chat::Message], messages: &[openai_harmony::chat::Message],
) -> (Option<String>, Option<Vec<ToolCall>>, String) { ) -> (Option<String>, Option<Vec<ToolCall>>, String) {
let mut analysis = None; let mut analysis: Option<String> = None;
let mut commentary: Option<Vec<ToolCall>> = None; let mut commentary: Option<Vec<ToolCall>> = None;
let mut final_text = String::new(); let mut final_text = String::new();
...@@ -119,6 +119,60 @@ impl HarmonyParserAdapter { ...@@ -119,6 +119,60 @@ impl HarmonyParserAdapter {
let channel = msg.channel.as_deref().unwrap_or(""); let channel = msg.channel.as_deref().unwrap_or("");
let recipient = msg.recipient.as_deref(); let recipient = msg.recipient.as_deref();
// IMPORTANT: Check recipient FIRST before channel
// The model sometimes generates tool calls with channel="analysis" + recipient="functions.*"
// instead of channel="commentary" + recipient="functions.*"
// We should trust the recipient field to determine if this is a tool call
if let Some(recipient_str) = recipient {
if recipient_str.starts_with("functions.") {
// This is a tool call, regardless of channel
let function_name = recipient_str.strip_prefix("functions.").unwrap();
// Process each content item separately
for content in &msg.content {
if let openai_harmony::chat::Content::Text(tc) = content {
let call_id = format!("call_{}", Uuid::new_v4());
let tool_call = ToolCall {
id: call_id,
tool_type: "function".to_string(),
function: FunctionCallResponse {
name: function_name.to_string(),
arguments: Some(tc.text.clone()),
},
};
match commentary.as_mut() {
Some(calls) => calls.push(tool_call),
None => commentary = Some(vec![tool_call]),
}
}
}
// Skip further channel processing for this message
continue;
} else if recipient_str.starts_with("python")
|| recipient_str.starts_with("browser")
|| recipient_str.starts_with("container")
{
// Built-in tools → treat as reasoning
// For Chat API, we add to analysis content
let text = Self::extract_text_from_content(&msg.content);
if !text.is_empty() {
// Append to analysis (built-in tools are reasoning)
match analysis.as_mut() {
Some(existing) => {
existing.push('\n');
existing.push_str(&text);
}
None => analysis = Some(text),
}
}
// Skip further channel processing
continue;
}
}
// Now process by channel (only if not already handled by recipient)
match channel { match channel {
"analysis" => { "analysis" => {
// Process each content item // Process each content item
...@@ -130,51 +184,25 @@ impl HarmonyParserAdapter { ...@@ -130,51 +184,25 @@ impl HarmonyParserAdapter {
} }
} }
"commentary" => { "commentary" => {
// Handle different recipient types // If we reach here, recipient was not "functions.*" or built-in tools
if let Some(recipient_str) = recipient { // Commentary channel should always have a recipient
if recipient_str.starts_with("functions.") { // This is likely a model bug - log warning and treat as reasoning
let function_name = recipient_str.strip_prefix("functions.").unwrap(); tracing::warn!(
channel = "commentary",
// Process each content item separately recipient = ?recipient,
for content in &msg.content { "Commentary message without valid recipient, treating as reasoning"
if let openai_harmony::chat::Content::Text(tc) = content { );
let call_id = format!("call_{}", Uuid::new_v4());
let tool_call = ToolCall { let text = Self::extract_text_from_content(&msg.content);
id: call_id,
tool_type: "function".to_string(), if !text.is_empty() {
function: FunctionCallResponse { match analysis.as_mut() {
name: function_name.to_string(), Some(existing) => {
arguments: Some(tc.text.clone()), existing.push('\n');
}, existing.push_str(&text);
};
match commentary.as_mut() {
Some(calls) => calls.push(tool_call),
None => commentary = Some(vec![tool_call]),
}
}
}
} else if recipient_str.starts_with("python")
|| recipient_str.starts_with("browser")
|| recipient_str.starts_with("container")
{
// Built-in tools → treat as reasoning
// For Chat API, we add to analysis content
let text = Self::extract_text_from_content(&msg.content);
if !text.is_empty() {
// Append to analysis (built-in tools are reasoning)
match analysis.as_mut() {
Some(existing) => {
existing.push('\n');
existing.push_str(&text);
}
None => analysis = Some(text),
}
} }
None => analysis = Some(text),
} }
// Unknown recipient would raise ValueError
// For now, we silently ignore (can add logging later)
} }
} }
"final" => { "final" => {
...@@ -215,16 +243,9 @@ impl HarmonyParserAdapter { ...@@ -215,16 +243,9 @@ impl HarmonyParserAdapter {
) -> Result<HarmonyChannelOutput, String> { ) -> Result<HarmonyChannelOutput, String> {
// Feed all tokens to the parser // Feed all tokens to the parser
for &token_id in output_ids { for &token_id in output_ids {
self.parser.process(token_id).map_err(|e| { self.parser
// Log the full output_ids context on error .process(token_id)
tracing::error!( .map_err(|e| format!("Failed to process token {}: {}", token_id, e))?;
token_id = token_id,
output_ids = ?output_ids,
error = %e,
"Harmony parser failed to process token"
);
format!("Failed to process token {}: {}", token_id, e)
})?;
} }
// Extract all completed messages from the parser // Extract all completed messages from the parser
...@@ -240,7 +261,7 @@ impl HarmonyParserAdapter { ...@@ -240,7 +261,7 @@ impl HarmonyParserAdapter {
let final_finish_reason = if commentary.is_some() { let final_finish_reason = if commentary.is_some() {
"tool_calls".to_string() "tool_calls".to_string()
} else { } else {
finish_reason finish_reason.clone()
}; };
Ok(HarmonyChannelOutput { Ok(HarmonyChannelOutput {
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment