feat(completions): add native gRPC pipeline typing for /v1/completions - #840
Conversation
Add protocol-level validation for CompletionRequest and switch the /v1/completions HTTP handler to ValidatedJson so invalid requests fail before entering router-specific execution paths. Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Introduce CompletionRequest as a first-class gRPC pipeline request type and extend shared pipeline orchestration to carry completion requests and completion responses natively. Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenAI-style Completion support: CompletionRequest gains field and schema validation and normalization; RequestType/FinalResponse gain Completion variants; RequestContext, routing pipeline, dispatch, Harmony/regular stages, and the HTTP v1_completions handler are updated to accept, route, or reject completions as appropriate. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as HTTP Server (v1_completions)
participant Validation as ValidatedJson Layer
participant Pipeline as RequestPipeline
participant Context as RequestContext
participant Stages as Pipeline Stages
participant ResponseHandler as Response Handler
Client->>Server: POST /v1/completions (CompletionRequest)
Server->>Validation: extract & validate CompletionRequest
Validation->>Validation: field & cross-field validation
Validation-->>Server: validated request
Server->>Pipeline: execute_completion(request, headers, model_id)
Pipeline->>Context: RequestContext::for_completion(...)
Pipeline->>Stages: run pipeline stages (dispatch, routing, etc.)
Stages-->>Pipeline: stage outputs
Pipeline->>ResponseHandler: inspect ctx.state.response.final_response
alt FinalResponse::Completion present
ResponseHandler-->>Client: axum::Json(CompletionResponse)
else No or wrong response
ResponseHandler-->>Client: internal error (logged, metrics)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly advances the integration of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces native typing for /v1/completions requests within the gRPC pipeline, laying the groundwork for full support. Key changes include adding CompletionRequest and CompletionResponse to the pipeline's core enums, implementing validation for CompletionRequest parameters, and adding a new execute_completion pipeline entrypoint. The changes are well-structured and consistent with the existing pipeline design. My main feedback is a suggestion to refactor the duplicated logic across the execute_* functions in pipeline.rs to improve maintainability, which could be handled in a follow-up.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/pipeline.rs (2)
1015-1023:⚠️ Potential issue | 🟡 MinorUpdate error message to include Completion.
The match pattern now includes
FinalResponse::Completion(_), but the error message at line 1022 only mentions "Generate/Embedding/Classify/Messages". This inconsistency could cause confusion when debugging.📝 Proposed fix
error!( function = "execute_chat_for_responses", - "Wrong response type: expected Chat, got Generate/Embedding/Classify/Messages" + "Wrong response type: expected Chat, got Generate/Completion/Embedding/Classify/Messages" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 1015 - 1023, The error log inside execute_chat_for_responses mismatches the match arm that now includes FinalResponse::Completion(_); update the processLogger/error! message in the error! call within execute_chat_for_responses to list "Completion" alongside Generate/Embedding/Classify/Messages (i.e., change the string "Wrong response type: expected Chat, got Generate/Embedding/Classify/Messages" to include "Completion") so the message accurately reflects the matched variants of FinalResponse.
765-776: 🧹 Nitpick | 🔵 TrivialConsider aligning with the new error helper pattern.
The
execute_embeddingsandexecute_classifymethods (lines 765-776 and 865-876) still use inline error handling withSome(_)catch-all patterns, while the otherexecute_*methods now use the centralizedwrong_response_typeandno_response_producedhelpers with explicit variant enumeration.This inconsistency is not blocking, but aligning these methods in a follow-up would improve code consistency and make the wrong-response logging more descriptive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 765 - 776, In execute_embeddings and execute_classify replace the catch-all Some(_) and None branches with the same centralized error helper pattern used elsewhere: enumerate the specific response enum variants you expect and call the shared wrong_response_type helper with the explicit unexpected variant (rather than a blanket Some(_)), and call no_response_produced for the None case; update the error logging to include the variant names and function="execute_embeddings"/function="execute_classify" context so the handlers (wrong_response_type, no_response_produced) are used consistently across all execute_* methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 1015-1023: The error log inside execute_chat_for_responses
mismatches the match arm that now includes FinalResponse::Completion(_); update
the processLogger/error! message in the error! call within
execute_chat_for_responses to list "Completion" alongside
Generate/Embedding/Classify/Messages (i.e., change the string "Wrong response
type: expected Chat, got Generate/Embedding/Classify/Messages" to include
"Completion") so the message accurately reflects the matched variants of
FinalResponse.
- Around line 765-776: In execute_embeddings and execute_classify replace the
catch-all Some(_) and None branches with the same centralized error helper
pattern used elsewhere: enumerate the specific response enum variants you expect
and call the shared wrong_response_type helper with the explicit unexpected
variant (rather than a blanket Some(_)), and call no_response_produced for the
None case; update the error logging to include the variant names and
function="execute_embeddings"/function="execute_classify" context so the
handlers (wrong_response_type, no_response_produced) are used consistently
across all execute_* methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53cd7ec3-8277-4b85-9e26-e8d5e687331f
📒 Files selected for processing (9)
crates/protocols/src/completion.rsmodel_gateway/src/routers/grpc/common/stages/dispatch_metadata.rsmodel_gateway/src/routers/grpc/context.rsmodel_gateway/src/routers/grpc/harmony/stages/request_building.rsmodel_gateway/src/routers/grpc/harmony/stages/response_processing.rsmodel_gateway/src/routers/grpc/pipeline.rsmodel_gateway/src/routers/grpc/regular/stages/request_building.rsmodel_gateway/src/routers/grpc/regular/stages/response_processing.rsmodel_gateway/src/server.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e33908e39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
github-actions
Bot
added
grpc
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Hi @vschandramourya, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/pipeline.rs (2)
867-877: 🧹 Nitpick | 🔵 TrivialSame inconsistency as
execute_embeddings: missing metrics for internal error paths.The error handling here also doesn't use the new helpers and misses error metrics recording.
♻️ Proposed fix to use helper methods
match ctx.state.response.final_response { Some(FinalResponse::Classify(response)) => { Metrics::record_router_duration( metrics_labels::ROUTER_GRPC, self.backend_type, metrics_labels::CONNECTION_GRPC, &model, metrics_labels::ENDPOINT_CLASSIFY, start.elapsed(), ); axum::Json(response).into_response() } - Some(_) => { - error!(function = "execute_classify", "Wrong response type"); - error::internal_error("wrong_response_type", "Internal error: wrong response type") - } - None => { - error!( - function = "execute_classify", - "No final response produced by pipeline." - ); - error::internal_error("no_response_produced", "No response produced") - } + Some( + response_type @ (FinalResponse::Chat(_) + | FinalResponse::Generate(_) + | FinalResponse::Completion(_) + | FinalResponse::Embedding(_) + | FinalResponse::Messages(_)), + ) => self.wrong_response_type( + "execute_classify", + "Classify", + &response_type, + &model, + metrics_labels::ENDPOINT_CLASSIFY, + ), + None => self.no_response_produced( + "execute_classify", + &model, + metrics_labels::ENDPOINT_CLASSIFY, + ), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 867 - 877, The execute_classify error paths currently log errors directly and return internal errors without recording metrics or using the shared helpers; update the Some(_) and None branches in execute_classify to call the same helper functions used by execute_embeddings (e.g., the internal error helper and metrics-increment helper), replace raw error! and error::internal_error calls with the centralized helper for creating internal errors and ensure the corresponding error metric is incremented (matching the pattern in execute_embeddings) so all internal error paths record metrics and use the common error construction.
769-779: 🧹 Nitpick | 🔵 TrivialInconsistent error handling: missing metrics recording for internal error paths.
The new
wrong_response_type()andno_response_produced()helpers recordMetrics::record_router_error()for observability, butexecute_embeddingsstill uses inline error handling without metrics. This creates an observability gap compared toexecute_chat,execute_generate,execute_completion, andexecute_messages.Consider updating to use the helpers for consistency:
♻️ Proposed fix to use helper methods
match ctx.state.response.final_response { Some(FinalResponse::Embedding(response)) => { Metrics::record_router_duration( metrics_labels::ROUTER_GRPC, self.backend_type, metrics_labels::CONNECTION_GRPC, &model, metrics_labels::ENDPOINT_EMBEDDINGS, start.elapsed(), ); axum::Json(response).into_response() } - Some(_) => { - error!(function = "execute_embeddings", "Wrong response type"); - error::internal_error("wrong_response_type", "Internal error: wrong response type") - } - None => { - error!( - function = "execute_embeddings", - "No final response produced by pipeline." - ); - error::internal_error("no_response_produced", "No response produced") - } + Some( + response_type @ (FinalResponse::Chat(_) + | FinalResponse::Generate(_) + | FinalResponse::Completion(_) + | FinalResponse::Classify(_) + | FinalResponse::Messages(_)), + ) => self.wrong_response_type( + "execute_embeddings", + "Embedding", + &response_type, + &model, + metrics_labels::ENDPOINT_EMBEDDINGS, + ), + None => self.no_response_produced( + "execute_embeddings", + &model, + metrics_labels::ENDPOINT_EMBEDDINGS, + ), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 769 - 779, Replace the inline error handling inside execute_embeddings that currently calls error!(...) and error::internal_error(...) for the "Wrong response type" and "No final response produced" cases with the existing helper functions wrong_response_type() and no_response_produced() (the same helpers used by execute_chat/execute_generate/execute_completion/execute_messages) so that Metrics::record_router_error() is invoked and metrics/observability remain consistent across all internal-error paths.
🤖 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/pipeline.rs`:
- Around line 588-680: The code unnecessarily clones model_id when calling
RequestContext::for_completion in execute_completion; remove the clone and pass
the original model_id (or a reference if for_completion's signature requires
&Option<String>) so you don't allocate an extra String—update the call in
execute_completion (where model_id.clone() is used) to pass model_id directly
and adjust ownership or borrowing to match RequestContext::for_completion's
parameter.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 867-877: The execute_classify error paths currently log errors
directly and return internal errors without recording metrics or using the
shared helpers; update the Some(_) and None branches in execute_classify to call
the same helper functions used by execute_embeddings (e.g., the internal error
helper and metrics-increment helper), replace raw error! and
error::internal_error calls with the centralized helper for creating internal
errors and ensure the corresponding error metric is incremented (matching the
pattern in execute_embeddings) so all internal error paths record metrics and
use the common error construction.
- Around line 769-779: Replace the inline error handling inside
execute_embeddings that currently calls error!(...) and
error::internal_error(...) for the "Wrong response type" and "No final response
produced" cases with the existing helper functions wrong_response_type() and
no_response_produced() (the same helpers used by
execute_chat/execute_generate/execute_completion/execute_messages) so that
Metrics::record_router_error() is invoked and metrics/observability remain
consistent across all internal-error paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41f83bde-9eb3-4e43-8365-8777e42b9c60
📒 Files selected for processing (1)
model_gateway/src/routers/grpc/pipeline.rs
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
mergify
Bot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
model_gateway/src/routers/grpc/pipeline.rs (1)
613-614: 🧹 Nitpick | 🔵 TrivialAvoid unnecessary clone.
model_idis not used after this line, so the.clone()is unnecessary.♻️ Proposed fix
- let mut ctx = - RequestContext::for_completion(request, headers, model_id.clone(), components); + let mut ctx = RequestContext::for_completion(request, headers, model_id, components);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 613 - 614, The call to RequestContext::for_completion unnecessarily clones model_id; since model_id is not used afterwards, remove the `.clone()` and pass model_id by value (move) into RequestContext::for_completion to avoid the extra allocation; update the call site where RequestContext::for_completion(request, headers, model_id, components) is invoked.
🤖 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/context.rs`:
- Around line 273-289: The for_completion constructor currently takes model_id:
Option<String> and assigns it to RequestInput.model_id (a String), causing a
type mismatch; change for_completion to extract the model id from the
CompletionRequest (use request.model or equivalent) and pass that String into
RequestInput.model_id (make signature model_id: String to match other
constructors or remove the param and derive it inside for_completion), update
the for_completion function body to use the extracted String for
RequestInput.model_id, and then update the caller execute_completion in
pipeline.rs to stop passing model_id (remove that argument) so callers match the
new for_completion usage.
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 1065-1070: RequestContext::for_responses is being passed an
Option<String> (Some(request.model.clone())) for the model_id parameter but it
expects a plain String; replace any occurrences of Some(request.model.clone())
with request.model.clone() — update the call site in the block that constructs
ctx (using RequestContext::for_responses with Arc::new(request.clone()) and
harmony_ctx.components.clone()) and the other identical call later that also
uses RequestContext::for_responses so both pass request.model.clone() directly.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 613-614: The call to RequestContext::for_completion unnecessarily
clones model_id; since model_id is not used afterwards, remove the `.clone()`
and pass model_id by value (move) into RequestContext::for_completion to avoid
the extra allocation; update the call site where
RequestContext::for_completion(request, headers, model_id, components) is
invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d0fb261-1cc7-42ed-8a23-ef605e7570b6
📒 Files selected for processing (4)
model_gateway/src/routers/grpc/common/stages/dispatch_metadata.rsmodel_gateway/src/routers/grpc/context.rsmodel_gateway/src/routers/grpc/pipeline.rsmodel_gateway/src/server.rs
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/pipeline.rs (1)
1015-1027: 🧹 Nitpick | 🔵 TrivialError message doesn't mention Completion.
The match arm now rejects
FinalResponse::Completionbut the error message on line 1022 still says"expected Chat, got Generate/Embedding/Classify/Messages". Consider updating it to include "Completion" for accurate diagnostics.♻️ Update error message
error!( function = "execute_chat_for_responses", - "Wrong response type: expected Chat, got Generate/Embedding/Classify/Messages" + "Wrong response type: expected Chat, got Generate/Completion/Embedding/Classify/Messages" );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 1015 - 1027, The error message in execute_chat_for_responses lists rejected FinalResponse variants but omits Completion; update the processLogger/error call inside the match arm handling Some(FinalResponse::Generate/_Completion/_Embedding/_Classify/_Messages) to include "Completion" in the string (e.g., "expected Chat, got Generate/Completion/Embedding/Classify/Messages") so the error accurately reflects all rejected FinalResponse variants.
♻️ Duplicate comments (1)
model_gateway/src/routers/grpc/pipeline.rs (1)
610-611: 🧹 Nitpick | 🔵 TrivialUnnecessary clone of
model_id.The
model_idvariable is not used after being passed tofor_completion, so the.clone()is unnecessary. Sincemodel_idis an ownedOption<String>, it can be moved directly.♻️ Remove unnecessary clone
- let mut ctx = - RequestContext::for_completion(request, headers, model_id.clone(), components); + let mut ctx = RequestContext::for_completion(request, headers, model_id, components);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 610 - 611, The call to RequestContext::for_completion currently clones model_id unnecessarily; since model_id is an owned Option<String> and not used afterward, remove the .clone() and move model_id into RequestContext::for_completion (i.e., pass model_id directly), ensuring no other code later expects model_id to remain usable.
🤖 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/context.rs`:
- Around line 273-290: The for_completion constructor currently accepts
model_id: Option<String> while other constructors use model_id: String; change
for_completion to take model_id: String (matching the other for_* methods),
remove the unwrap_or_else usage and set RequestInput.model_id from the provided
model_id, and then update callers (e.g., in pipeline.rs) to pass a String
model_id (or remove the extra argument if callers should instead extract
request.model there) so signatures are consistent; locate the code by searching
for for_completion, RequestInput, and RequestType::Completion to make the edits.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 1015-1027: The error message in execute_chat_for_responses lists
rejected FinalResponse variants but omits Completion; update the
processLogger/error call inside the match arm handling
Some(FinalResponse::Generate/_Completion/_Embedding/_Classify/_Messages) to
include "Completion" in the string (e.g., "expected Chat, got
Generate/Completion/Embedding/Classify/Messages") so the error accurately
reflects all rejected FinalResponse variants.
---
Duplicate comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 610-611: The call to RequestContext::for_completion currently
clones model_id unnecessarily; since model_id is an owned Option<String> and not
used afterward, remove the .clone() and move model_id into
RequestContext::for_completion (i.e., pass model_id directly), ensuring no other
code later expects model_id to remain usable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7704d2bb-37d1-4ca5-9c8c-dcefa9315fd4
📒 Files selected for processing (2)
model_gateway/src/routers/grpc/context.rsmodel_gateway/src/routers/grpc/pipeline.rs
Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/pipeline.rs (1)
1014-1022:⚠️ Potential issue | 🟡 MinorKeep the wrong-type log aligned with the new Completion branch.
Line 1021 still says
Generate/Embedding/Classify/Messages, so ifFinalResponse::Completion(_)ever leaks here the diagnostic reports the wrong variant. Reusing%response_typelikewrong_response_type()would keep this from drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/pipeline.rs` around lines 1014 - 1022, The error message in execute_chat_for_responses is stale — it lists "Generate/Embedding/Classify/Messages" but the match also includes FinalResponse::Completion(_); update the diagnostic to derive the wrong response type dynamically (e.g., compute a response_type string from the matched variant or add a helper like wrong_response_type()) and use that value in the processLogger/error call so the log always reflects the actual FinalResponse variant (reference execute_chat_for_responses and FinalResponse::{Generate,Completion,Embedding,Classify,Messages}).
🤖 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/pipeline.rs`:
- Around line 645-675: The match arm handling FinalResponse::Completion in
execute_completion is unreachable because Completion requests are rejected
earlier by PreparationStage and ResponseProcessingStage; fix by either (A)
adding Completion handling into the regular pipeline: update PreparationStage
and ResponseProcessingStage to accept and pass through Completion requests
(remove or conditionalize the "wrong_pipeline"/"should use its dedicated
pipeline" rejections) so they can produce FinalResponse::Completion, or (B)
route completion requests into the dedicated completion pipeline before reaching
execute_completion so execute_completion only receives responses the pipeline
can produce; locate the logic in PreparationStage, ResponseProcessingStage and
the execute_completion match and make the chosen change so
FinalResponse::Completion can be produced and returned.
---
Outside diff comments:
In `@model_gateway/src/routers/grpc/pipeline.rs`:
- Around line 1014-1022: The error message in execute_chat_for_responses is
stale — it lists "Generate/Embedding/Classify/Messages" but the match also
includes FinalResponse::Completion(_); update the diagnostic to derive the wrong
response type dynamically (e.g., compute a response_type string from the matched
variant or add a helper like wrong_response_type()) and use that value in the
processLogger/error call so the log always reflects the actual FinalResponse
variant (reference execute_chat_for_responses and
FinalResponse::{Generate,Completion,Embedding,Classify,Messages}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cde28ec-aedc-4730-9b13-36de7ac5808c
📒 Files selected for processing (2)
model_gateway/src/routers/grpc/context.rsmodel_gateway/src/routers/grpc/pipeline.rs
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
CatherineSue
left a comment
CatherineSue
left a comment
There was a problem hiding this comment.
LGTM. We can merge once CI passes
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
smg-project#840) Signed-off-by: VS Chandra Mourya <msrinivasa@together.ai>
2 participants
Description
Problem
This PR continues the
/v1/completionsrollout by making completions a first-class native request type in the gRPC pipeline. PRs combined: #761 #768Solution
Introduce
CompletionRequestas a native gRPC pipeline request type and extend shared pipeline orchestration to carry completion requests and completion responses natively.Changes
RequestType::CompletionFinalResponse::Completionexecute_completion()to the gRPC request pipelineTest Plan
cargo test -p smg completion --quietcargo clippy -p smg --all-targets --all-features -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes