refactor(grpc): rename shared dispatcher stages, remove dead arms - #1004
Conversation
The three shared dispatcher stages (PreparationStage, RequestBuildingStage, ResponseProcessingStage) are only used by new_regular() and new_pd() pipelines, which only serve Chat + Generate requests. All other endpoints (Embedding, Classify, Messages, Completion) have dedicated pipelines that wire their own stages directly. Rename to ChatGenerate* prefix, remove dead arms: - ChatGeneratePreparationStage: remove Completion arm + field - ChatGenerateRequestBuildingStage: remove Embedding/Classify arms + field - ChatGenerateResponseProcessingStage: remove Embedding/Classify arms + fields Signed-off-by: Chang Su <chang.s.su@oracle.com>
📝 WalkthroughWalkthroughThe regular gRPC request pipeline stages were specialized: generic Preparation/RequestBuilding/ResponseProcessing stages were replaced with chat/generate-specific variants, and stage dispatch was narrowed to handle only Chat and Generate request types (Embedding and Classify handling removed). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 gRPC request pipeline by renaming generic stages to ChatGenerate specific implementations, such as ChatGeneratePreparationStage, ChatGenerateRequestBuildingStage, and ChatGenerateResponseProcessingStage. The changes also involve narrowing the scope of these stages by removing logic for handling unrelated request types like Completion, Embedding, and Classify, focusing them exclusively on Chat and Generate requests. I have no feedback to provide.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/regular/stages/request_building.rs`:
- Around line 18-20: The doc comment in request_building.rs incorrectly
references the removed constructor `new_regular_pd`; update that reference to
the current PD constructor `new_pd()` so readers are pointed to the actual
symbol (keep the other reference `new_regular` as-is), i.e., change the doc line
mentioning `new_regular_pd` to `new_pd()` to match the constructor defined in
the pipeline module.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef40323a-904b-4f46-ae55-81d7c1609375
📒 Files selected for processing (5)
model_gateway/src/routers/grpc/pipeline.rsmodel_gateway/src/routers/grpc/regular/stages/mod.rsmodel_gateway/src/routers/grpc/regular/stages/preparation.rsmodel_gateway/src/routers/grpc/regular/stages/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/response_processing.rs
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/regular/stages/request_building.rs`:
- Around line 55-57: The stage name returned by fn name(&self) -> &'static str
in the ChatGenerateRequestBuilding stage was changed to
"ChatGenerateRequestBuilding", so update any log-based filters, dashboards, and
alert rules that previously matched the old "RequestBuilding" label to now match
"ChatGenerateRequestBuilding"; verify any usages in pipeline.rs and related
alerting/aggregation configurations that rely on stage labels and adjust them
accordingly to avoid missing logs or false alerts.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2ff8e81-45dd-40a7-8506-ba9a582337c9
📒 Files selected for processing (1)
model_gateway/src/routers/grpc/regular/stages/request_building.rs
| fn name(&self) -> &'static str { | ||
| "RequestBuilding" | ||
| "ChatGenerateRequestBuilding" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stage name change affects log labels — update any log-based alerts if needed.
The stage name returned by name() is used throughout pipeline.rs for error and debug logging (as shown in the relevant code snippets). Any log aggregation, dashboards, or alerts filtering on the old "RequestBuilding" label will need to be updated to "ChatGenerateRequestBuilding".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model_gateway/src/routers/grpc/regular/stages/request_building.rs` around
lines 55 - 57, The stage name returned by fn name(&self) -> &'static str in the
ChatGenerateRequestBuilding stage was changed to "ChatGenerateRequestBuilding",
so update any log-based filters, dashboards, and alert rules that previously
matched the old "RequestBuilding" label to now match
"ChatGenerateRequestBuilding"; verify any usages in pipeline.rs and related
alerting/aggregation configurations that rely on stage labels and adjust them
accordingly to avoid missing logs or false alerts.
…g-project#1004) Signed-off-by: Chang Su <chang.s.su@oracle.com>
Description
Problem
The three shared dispatcher stages (
PreparationStage,RequestBuildingStage,ResponseProcessingStage) have misleading generic names and contain dead match arms for request types that never reach them.These dispatchers are only used by
new_regular()andnew_pd()pipelines, which exclusively serve Chat + Generate requests. All other endpoints have dedicated pipelines that wire their own stages directly:new_regular()/new_pd()new_regular()/new_pd()new_embeddings()new_classify()new_messages()new_completion()Solution
Rename dispatchers to reflect their actual scope and remove dead arms:
PreparationStage→ChatGeneratePreparationStage— remove Completion arm + fieldRequestBuildingStage→ChatGenerateRequestBuildingStage— remove Embedding/Classify arms + fieldResponseProcessingStage→ChatGenerateResponseProcessingStage— remove Embedding/Classify arms + fieldsChanges
mod.rsre-exportspipeline.rsto use new namesTest Plan
cargo checkpassescargo clippy --all-targets --all-features -- -D warningspassescargo +nightly fmtpassesChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit