refactor(grpc): replace PreparationOutput struct with typed enum - #1144
Conversation
Replace the flat PreparationOutput struct (9 fields, many always-None) with a 6-variant enum (Chat, Messages, Completion, Generate, Embedding, Harmony). Each variant carries only the fields relevant to its pipeline. Key improvements: - processed_messages is non-optional in Chat/Messages (type-guaranteed) - Chat/Messages no longer clone original_text (routing_text() borrows from processed_messages.text) - Harmony request_building uses .take() + move instead of .as_ref() + clone - Worker selection uses routing_text() instead of if/else on harmony_mode - Box<ChatCompletionRequest> in Chat/Harmony variants reduces enum size for Completion/Generate/Embedding paths - debug_assert + graceful error replaces panic for variant checks Signed-off-by: Chang Su <chang.s.su@oracle.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis pull request refactors the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request refactors the PreparationOutput struct into an enum with specific variants for each request type, such as Chat, Messages, and Harmony. This change improves type safety by eliminating optional fields that were only relevant to specific pipelines. The review feedback correctly identifies several compilation issues in the request building stages where owned values were moved inside match arms or where type mismatches occurred between Option types and Arc references.
Description
Problem
PreparationOutputis a flat struct with 9 fields, many of which are alwaysNonefor certain request types. For example, Completion/Generate/Embedding never useprocessed_messages,tool_constraints,filtered_request, orharmony_*fields — yet they must set them all toNone. This wastes memory (the enum is sized to its largest field,ChatCompletionRequestat ~1448 bytes) and provides no type-level guarantee that required fields are present.Solution
Replace the struct with a 6-variant enum (
Chat,Messages,Completion,Generate,Embedding,Harmony), where each variant carries only the fields relevant to its pipeline. Add helper methods (token_ids(),routing_text()) for cross-variant access used by worker selection and simple request builders.Changes
PreparationOutputstruct → enum with 6 variants + helper methodsNonefieldsrouting_text()replacesif harmony_mode { selection_text } else { original_text }routing_text()replacesoriginal_text.as_deref()Key improvements:
processed_messagesis non-optional in Chat/Messages (type-guaranteed, removes runtime.ok_or_else()check)original_text(routing_text()borrows fromprocessed_messages.text).take()+ move instead of.as_ref()+ cloneBox<ChatCompletionRequest>in Chat/Harmony variants reduces enum size for Completion/Generate/Embedding pathsdebug_assert+ graceful gRPC error replacesunreachable!()for variant checksTest Plan
All existing tests pass (1015 tests across workspace). This is a purely mechanical refactor — no behavioral changes.
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit