fix(pd): route chat using full message history - #1929
Conversation
Signed-off-by: lixiang5 <lixiang5@sensetime.com>
📝 WalkthroughWalkthroughThe HTTP chat router now obtains routing text through ChangesChat routing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request simplifies the routing text extraction logic in pd_router.rs by replacing manual pattern matching on ChatMessage with a call to body.extract_text_for_routing(). Feedback was provided to prevent potential load imbalance issues by ensuring that empty strings returned by extract_text_for_routing() are mapped to None instead of being wrapped in Some("").
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let request_text = if self.policies_need_request_text() { | ||
| body.messages.first().and_then(|msg| match msg { | ||
| ChatMessage::User { content, .. } => match content { | ||
| MessageContent::Text(text) => Some(text.clone()), | ||
| MessageContent::Parts(_) => None, | ||
| }, | ||
| ChatMessage::Developer { content, .. } => match content { | ||
| MessageContent::Text(text) => Some(text.clone()), | ||
| MessageContent::Parts(_) => None, | ||
| }, | ||
| ChatMessage::System { content, .. } => Some(content.to_simple_string()), | ||
| _ => None, | ||
| }) | ||
| Some(body.extract_text_for_routing()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
If extract_text_for_routing() returns an empty string, wrapping it in Some results in Some(""). Passing an empty string to routing policies (like cache-aware or consistent hashing) can cause all requests with empty routing text to hash to the same worker, leading to load imbalance. It is better to map an empty string to None so that policies can correctly fall back to load-balancing or round-robin routing.
| let request_text = if self.policies_need_request_text() { | |
| body.messages.first().and_then(|msg| match msg { | |
| ChatMessage::User { content, .. } => match content { | |
| MessageContent::Text(text) => Some(text.clone()), | |
| MessageContent::Parts(_) => None, | |
| }, | |
| ChatMessage::Developer { content, .. } => match content { | |
| MessageContent::Text(text) => Some(text.clone()), | |
| MessageContent::Parts(_) => None, | |
| }, | |
| ChatMessage::System { content, .. } => Some(content.to_simple_string()), | |
| _ => None, | |
| }) | |
| Some(body.extract_text_for_routing()) | |
| } else { | |
| None | |
| }; | |
| let request_text = if self.policies_need_request_text() { | |
| let text = body.extract_text_for_routing(); | |
| (!text.is_empty()).then_some(text) | |
| } else { | |
| None | |
| }; |
Description
Closes #1928
Problem
The HTTP PD router only inspected the first chat message when building routing text. For multi-turn conversations, later messages did not participate in cache-aware or prefix-based worker selection. Multipart text in the first message could also produce no routing text.
Solution
Use
ChatCompletionRequest::extract_text_for_routing(), matching the regular HTTP router and including text from the complete message history.Changes
ChatMessageandMessageContentimports.Test Plan
cargo test -p smg --lib routers::http::pd_router::tests(8 passed, 0 failed).cargo +nightly fmt --all -- --check(passed).cargo clippy --workspace --all-targets -- -D warningsis currently blocked on macOS by two pre-existing unused imports inmodel_gateway/src/routers/grpc/multimodal/assemble.rs. The all-features variant additionally requires local OpenCV/pkg-config.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses (blocked by the environment/pre-existing warnings described above)Summary by CodeRabbit