feat(embed): add vLLM gRPC embedding support, clean up proto - #1001
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
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:
📝 WalkthroughWalkthroughRemoved Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ModelGateway
participant BackendSelector
participant SglangServicer
participant VllmServicer
participant VllmEngine
Client->>ModelGateway: Send EmbedRequest
ModelGateway->>BackendSelector: Select backend (SGLang | VLLM)
alt SGLang
BackendSelector->>SglangServicer: Forward EmbedRequest
SglangServicer-->>BackendSelector: EmbedResponse { embedding, prompt_tokens, embedding_dim }
else VLLM
BackendSelector->>VllmServicer: Forward EmbedRequest
VllmServicer->>VllmEngine: engine.encode(TokensPrompt, PoolingParams)
VllmEngine-->>VllmServicer: streaming outputs -> final output
VllmServicer-->>BackendSelector: EmbedResponse { embedding, prompt_tokens, embedding_dim }
end
BackendSelector-->>ModelGateway: ProtoEmbedComplete (Sglang | Vllm)
ModelGateway-->>Client: Return completion (embedding, prompt_tokens, embedding_dim)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
Comment |
There was a problem hiding this comment.
Code Review
This pull request implements embedding support for the vLLM backend and simplifies the gRPC EmbedResponse structure by flattening it across both SGLang and vLLM. It removes the log_metrics field from embedding and classification requests and updates the model gateway to handle the unified response format. A high-severity issue was identified in the vLLM servicer where calling .tolist() on a standard Python list would cause a runtime error.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 208-212: The handler constructs a TokensPrompt by dereferencing
request.tokenized without checking whether the field is present; add a guard
using request.HasField("tokenized") before accessing request.tokenized in the
vLLM request handler (the block that creates prompt: TokensPrompt and reads
tokenized.input_ids and tokenized.original_text) and if the field is missing
abort the RPC with an INVALID_ARGUMENT grpc error (or raise the appropriate gRPC
exception) so malformed calls return INVALID_ARGUMENT instead of producing an
empty submessage or INTERNAL errors.
In `@model_gateway/src/routers/grpc/regular/stages/embedding/request_building.rs`:
- Around line 87-95: The current EmbeddingRequestBuildingStage::execute branch
for GrpcClient::Trtllm returns error::internal_error which incorrectly surfaces
unsupported TensorRT-LLM as a server failure; change this to a client-facing
error (e.g., error::bad_request or error::not_implemented) and update the logged
message accordingly so callers and metrics can distinguish
misrouting/configuration from real outages—locate the GrpcClient::Trtllm match
arm and replace the error::internal_error(...) return with
error::bad_request(...) or error::not_implemented(...), keeping the explanatory
string ("TensorRT-LLM embedding is not yet supported via gRPC") and the existing
error log context in EmbeddingRequestBuildingStage::execute.
🪄 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: 5b1a9f71-cd7d-417a-a31d-08c6effe7be7
📒 Files selected for processing (14)
crates/grpc_client/proto/sglang_scheduler.protocrates/grpc_client/src/sglang_scheduler.rscrates/grpc_client/src/vllm_engine.rscrates/protocols/src/classify.rscrates/protocols/src/embedding.rse2e_test/embeddings/test_basic.pye2e_test/embeddings/test_correctness.pygrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/client.rsmodel_gateway/src/routers/grpc/common/stages/request_execution.rsmodel_gateway/src/routers/grpc/proto_wrapper.rsmodel_gateway/src/routers/grpc/regular/stages/embedding/request_building.rsmodel_gateway/tests/spec/embedding.rs
💤 Files with no reviewable changes (4)
- crates/protocols/src/classify.rs
- crates/protocols/src/embedding.rs
- model_gateway/tests/spec/embedding.rs
- crates/grpc_client/src/sglang_scheduler.rs
- Add HasField("tokenized") guard in vLLM Embed servicer to return
INVALID_ARGUMENT instead of silently using empty submessage
- Change TensorRT-LLM unsupported error from internal_error to
not_implemented (501) since it's not a server failure
Signed-off-by: Chang Su <chang.s.su@oracle.com>
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 (1)
model_gateway/src/routers/grpc/regular/stages/embedding/request_building.rs (1)
60-99:⚠️ Potential issue | 🟡 MinorRestore the
Embedding/Classifyguard in this stage.This block now always emits
ProtoRequest::Embedat Line 99 once a client is present. If any unsupportedRequestTypeis ever routed here, the_ => format!("embed-...")fallback turns a wrong-pipeline invariant violation into a silent embed RPC dispatch instead of failing fast.Suggested fix
let request_id = match &ctx.input.request_type { RequestType::Embedding(_) => format!("embed-{}", Uuid::now_v7()), RequestType::Classify(_) => format!("classify-{}", Uuid::now_v7()), - _ => format!("embed-{}", Uuid::now_v7()), // fallback + _ => { + error!( + function = "EmbeddingRequestBuildingStage::execute", + "Unsupported request type for embedding request building" + ); + return Err(error::internal_error( + "wrong_pipeline", + "Embedding request building only supports embedding/classify requests", + )); + } };Based on learnings,
GrpcClient::embedintentionally keeps invariant-failure handling in the embedding path to catch client/request mismatches rather than silently coercing them.🤖 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/embedding/request_building.rs` around lines 60 - 99, The request-building stage currently coerces any RequestType into an embed RPC; restore the original guard by matching ctx.input.request_type (the RequestType enum) and only proceed when it's RequestType::Embedding(_) or RequestType::Classify(_), generating request_id with the appropriate prefix for those variants; for any other variant return a fast failure (e.g., from EmbeddingRequestBuildingStage::execute) with an appropriate not_implemented/invalid_request error instead of using the `_ => format!("embed-...")` fallback so we never set ctx.state.proto_request = Some(ProtoRequest::Embed(...)) for unsupported request types.
🤖 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/regular/stages/embedding/request_building.rs`:
- Around line 60-99: The request-building stage currently coerces any
RequestType into an embed RPC; restore the original guard by matching
ctx.input.request_type (the RequestType enum) and only proceed when it's
RequestType::Embedding(_) or RequestType::Classify(_), generating request_id
with the appropriate prefix for those variants; for any other variant return a
fast failure (e.g., from EmbeddingRequestBuildingStage::execute) with an
appropriate not_implemented/invalid_request error instead of using the `_ =>
format!("embed-...")` fallback so we never set ctx.state.proto_request =
Some(ProtoRequest::Embed(...)) for unsupported request types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15531094-7528-48aa-a6df-8404d135b13b
📒 Files selected for processing (2)
grpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/regular/stages/embedding/request_building.rs
…assify protocols log_metrics is a SGLang Prometheus metrics gate that leaked into the shared protocols crate. It has no meaning for vLLM or TensorRT-LLM backends and blocks clean multi-backend embedding support. Removed from EmbeddingRequest, ClassifyRequest, the gRPC embed proto (field 6 reserved), the SGLang client builder, and the EmbeddingRequestBuildingStage. GenerateRequest left as-is for now. Signed-off-by: Chang Su <chang.s.su@oracle.com>
- Add embed() and build_embed_request() to VllmEngineClient - Add Vllm variants to ProtoEmbedRequest, ProtoEmbedResponse, ProtoEmbedComplete - Add vLLM match arm in GrpcClient::embed() - Replace hardcoded sglang dispatch in EmbeddingRequestBuildingStage with backend-aware dispatch (SGLang, vLLM, TensorRT-LLM returns unsupported error) - Remove dead ProtoEmbedError enum and ProtoEmbedResponseVariant — all backends use tonic::Status for errors, not proto error types - Simplify request_execution embed path to use into_complete() Signed-off-by: Chang Su <chang.s.su@oracle.com>
Replace the UNIMPLEMENTED stub with a working implementation that calls vLLM's engine.encode() API with PoolingParams, collects the PoolingRequestOutput, and returns the embedding vector in EmbedResponse. Signed-off-by: Chang Su <chang.s.su@oracle.com>
- Flatten SGLang EmbedResponse: remove oneof, EmbedComplete, EmbedError, Embedding — put embedding/prompt_tokens/embedding_dim directly on EmbedResponse to match vLLM's flat proto shape - Remove cached_tokens from EmbedResponse (field 3 reserved) — SGLang hardcoded it to 0, vLLM doesn't have it, OpenAI embed API doesn't return it, gateway never reads it - Remove ProtoEmbedResponse wrapper — client.embed() now returns ProtoEmbedComplete directly, no oneof unwrapping needed - Simplify ProtoEmbedComplete to use EmbedResponse for both backends Signed-off-by: Chang Su <chang.s.su@oracle.com>
Add "vllm" to @pytest.mark.engine on both test_basic.py and test_correctness.py so the embedding test suite runs against vLLM gRPC backend in addition to SGLang. Signed-off-by: Chang Su <chang.s.su@oracle.com>
- Add HasField("tokenized") guard in vLLM Embed servicer to return
INVALID_ARGUMENT instead of silently using empty submessage
- Change TensorRT-LLM unsupported error from internal_error to
not_implemented (501) since it's not a server failure
Signed-off-by: Chang Su <chang.s.su@oracle.com>
…compat Old EmbedResponse used field 1 (request_id), 2 (oneof complete), 3 (oneof error). Reusing those numbers with different wire types would corrupt data during rolling deploys. Reserve 1-3, assign embedding=5, prompt_tokens=6, embedding_dim=4 (unchanged). Signed-off-by: Chang Su <chang.s.su@oracle.com>
6a253e3 to
86b7ae8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc_servicer/smg_grpc_servicer/sglang/servicer.py`:
- Around line 281-286: The Embed method assumes result contains "embedding" and
may KeyError when the scheduler returns an error payload; update the handler
(the Embed code path that currently builds sglang_scheduler_pb2.EmbedResponse)
to first check if "error" in result, and if so call
abort_code_from_output(result) and abort the RPC with that status and the
scheduler error message (use result["error"] and any meta_info/finish_reason),
otherwise extract embedding and return sglang_scheduler_pb2.EmbedResponse with
prompt_tokens and embedding_dim as before; follow the same error-check pattern
used by Generate to preserve the scheduler's original status and message.
In `@model_gateway/src/routers/grpc/common/stages/request_execution.rs`:
- Around line 200-208: The embed RPC path (client.embed(proto_request).await in
execute_single_embed) never records circuit-breaker outcomes, so embed-only
failures won't be seen by breaker logic; update the error path (inside the
map_err closure) to call workers.record_outcome(...) with the failing worker id
and a failure Outcome before returning the http error, and after a successful
embed (before returning Ok(ExecutionResult::Embedding { ... })) call
workers.record_outcome(...) with the worker id and a success Outcome; ensure you
use the same worker identity and Outcome enum used elsewhere in this module so
breaker/selection logic is updated consistently.
🪄 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: f8a86320-a197-4b1f-b0a7-97a73c8a67c9
📒 Files selected for processing (14)
crates/grpc_client/proto/sglang_scheduler.protocrates/grpc_client/src/sglang_scheduler.rscrates/grpc_client/src/vllm_engine.rscrates/protocols/src/classify.rscrates/protocols/src/embedding.rse2e_test/embeddings/test_basic.pye2e_test/embeddings/test_correctness.pygrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/client.rsmodel_gateway/src/routers/grpc/common/stages/request_execution.rsmodel_gateway/src/routers/grpc/proto_wrapper.rsmodel_gateway/src/routers/grpc/regular/stages/embedding/request_building.rsmodel_gateway/tests/spec/embedding.rs
💤 Files with no reviewable changes (4)
- model_gateway/tests/spec/embedding.rs
- crates/protocols/src/classify.rs
- crates/protocols/src/embedding.rs
- crates/grpc_client/src/sglang_scheduler.rs
…handling, circuit breaker - Fix EmbedResponse warmup check (no longer has error field, check embedding_dim) - Add return after await context.abort() in vLLM embed handler (defensive) - Check for error in sglang Embed result before accessing embedding - Record circuit-breaker outcomes for embed RPCs in request_execution Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 208-214: The abort branches should use the servicer's normal
logging and control flow: replace the inline context.abort branch for missing
tokenized (the request.HasField("tokenized") check) by raising ValueError for
the missing-tokenized case instead of calling context.abort directly, remove the
unreachable trailing return, and for the unfinished-encode case log an error via
the servicer's logger before calling
context.abort(grpc.StatusCode.INTERNAL,...); apply the same pattern to the
similar branch around the request/unfinished-encode check later (the block
referenced at 231-236) so server-side logs are emitted before aborting and
control stops via context.abort.
In `@model_gateway/src/routers/grpc/common/stages/request_execution.rs`:
- Line 136: The grpc span currently named "grpc_generate" is being used for
ProtoRequest::Embed (via execute_single_embed), which mislabels embed traffic;
update the tracing/span creation in the request_execution stage to distinguish
embed vs generate—either create separate spans (e.g., "grpc_embed" for
execute_single_embed and keep "grpc_generate" for generate paths) or add a
request_type tag/field to the existing span, ensuring you reference the
ProtoRequest::Embed match arm and the execute_single_embed call when making the
change.
🪄 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: 3621caef-98e3-419f-aba0-287d5e9cb915
📒 Files selected for processing (4)
grpc_servicer/smg_grpc_servicer/sglang/server.pygrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pymodel_gateway/src/routers/grpc/common/stages/request_execution.rs
…ort paths - Rename grpc_generate span to grpc_execute with request_type field - Use ValueError for missing tokenized input (goes through normal handler) - Log error before INTERNAL abort on unfinished embed Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
- Add vllm to engine marks on embedding test classes - Matrix e2e-1gpu-embeddings across sglang and vllm Signed-off-by: Chang Su <chang.s.su@oracle.com>
PoolingRequestOutput.outputs.data is a 2D tensor (1, dim). Squeeze it to 1D before tolist() so the repeated float proto field gets a flat list. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…t conversion PoolingOutput.data is a torch.Tensor that needs proper conversion via EmbeddingOutput.from_base() which validates dimensionality and calls .tolist() correctly. Fixes TypeError: must be real number, not list. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Raw TokensPrompt dicts (without type="token") trigger vLLM's deprecated input path which doesn't set the pooling task correctly, causing the token-wise ALL pooler to run instead of sequence-wise LAST. This produces 2D tensors that fail proto serialization. Use token_inputs() from vllm.inputs which sets type="token" and goes through the proper input processor. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
vLLM v0.18.1 returns shape [num_tokens, 4096] from the token-wise ALL pooler. Extract last token embedding to match seq_pooling_type=LAST. Also log input for further diagnosis. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Without task set, vLLM defaults to the token-wise ALL pooler returning per-token embeddings (2D tensor). Setting task='embed' routes to the sequence-wise LAST pooler which returns a single 1D embedding vector. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Was looking up 'embedding' key which doesn't exist. Use the actual model ID 'intfloat/e5-mistral-7b-instruct'. Signed-off-by: Chang Su <chang.s.su@oracle.com>
The correctness tests have a fundamental mismatch between sglang/vllm gateway embeddings and sentence-transformers CPU reference (similarity ~0.33 vs expected >0.98). Keep them skipped until investigated. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Log input text, first 5 embedding values, L2 norm, and dimension for both gateway and HF reference to diagnose mismatch. Signed-off-by: Chang Su <chang.s.su@oracle.com>
… reference sentence-transformers applies task-specific instruction prefixes for e5-mistral (e.g. "Instruct: ...\nQuery: ...") which the gateway doesn't add. Pass prompt_name=None to get raw embeddings matching gateway output. Remove debug logging. Signed-off-by: Chang Su <chang.s.su@oracle.com>
sglang produces embeddings that diverge from HF reference. vllm passes. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
…ject#1001) Signed-off-by: Chang Su <chang.s.su@oracle.com>
Description
Problem
The gRPC embedding pipeline was hardcoded to SGLang only. vLLM embedding requests via gRPC would panic. Additionally, the embedding proto and protocol types had accumulated SGLang-specific leaky abstractions (
log_metrics,EmbedError,EmbedCompleteoneof) that blocked clean multi-backend support.Solution
Add vLLM embedding support end-to-end (Rust gateway + Python servicer) and clean up the proto/protocol layers to be backend-agnostic.
Changes
log_metricsfrom embedding/classify protocols — SGLang-specific Prometheus metrics gate that leaked into sharedprotocolscrate. vLLM/TensorRT-LLM don't use it.embed()+build_embed_request()onVllmEngineClient,Vllmvariants onProtoEmbedRequest/ProtoEmbedComplete, backend-aware dispatch inEmbeddingRequestBuildingStage(TensorRT-LLM returns unsupported error).engine.encode()withPoolingParams, returns flatEmbedResponse.EmbedResponseproto — removeoneof { EmbedComplete, EmbedError },Embedding,EmbedErrormessage types. Errors usetonic::Status(same as chat pipeline). Both backends now share the same flatEmbedResponseshape.cached_tokensfrom embed proto — SGLang hardcoded it to 0, vLLM doesn't have it, OpenAI/v1/embeddingsspec doesn't return it, gateway never reads it.ProtoEmbedResponsewrapper —client.embed()returnsProtoEmbedCompletedirectly, no oneof unwrapping."vllm"to@pytest.mark.engineontest_basic.pyandtest_correctness.py.Test Plan
cargo checkpassescargo clippy --all-targets --all-features -- -D warningspasses (via pre-commit)cargo +nightly fmtpasses (via pre-commit)ruffpasses on Python changes (via pre-commit)pytest e2e_test/embeddings/test_basic.py -v -k "vllm and grpc"with embedding modelpytest e2e_test/embeddings/test_correctness.py -v -k "vllm and grpc"with embedding modelSummary by CodeRabbit
New Features
Changes
Tests