Repository navigation
refactor(grpc): use EngineClient interface instead of AsyncLLM in vLLM servicer - #949
Conversation
📝 WalkthroughWalkthroughVllm servicer replaced stored Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Servicer as VllmEngineServicer
participant Renderer as Renderer
participant Engine as EngineClient
participant Model as vLLM_Model
Client->>Servicer: Send Generate request
Servicer->>Servicer: capture arrival_time
Servicer->>Renderer: process_for_engine(inputs, arrival_time)
Renderer-->>Servicer: preprocessed inputs
Servicer->>Engine: generate(preprocessed inputs)
Engine->>Model: request generation
Model-->>Engine: tokens/chunks
Engine-->>Servicer: streamed responses
Servicer-->>Client: stream responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![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 migrates the VllmEngineServicer to use EngineClient instead of AsyncLLM, involving the renaming of the internal engine instance and updating the Generate method to utilize the engine's renderer for prompt processing. Additionally, several fields were removed from the GetServerInfo response. The review feedback recommends renaming the init parameter for consistency with the new type and refactoring the Generate method to capture a single timestamp for request arrival to ensure consistency and reduce redundant calls.
![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: f0d6d90b5f
ℹ️ 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".
![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 (1)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py (1)
295-320: 🧹 Nitpick | 🔵 TrivialConsider populating
uptime_secondsandserver_typefor consistency.Per PR objectives, SMG strips certain fields during worker discovery, so omitting them is acceptable. However,
uptime_secondsis trivially computable fromself.start_time, andserver_typeis a static string. Populating these would maintain parity with the SGLang servicer (seesglang/servicer.py:490-498) and support any non-SMG consumers of this RPC.♻️ Optional: populate additional fields
return vllm_engine_pb2.GetServerInfoResponse( kv_connector=kv_connector, kv_role=kv_role, + uptime_seconds=time.time() - self.start_time, + server_type="vllm-grpc", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py` around lines 295 - 320, GetServerInfo currently returns only kv_connector/kv_role; compute uptime_seconds as int(time.time() - self.start_time) and set server_type to the static string used elsewhere (e.g., "vllm") before returning the vllm_engine_pb2.GetServerInfoResponse. Update the GetServerInfo method to import/use time, reference self.start_time to calculate uptime_seconds, and include server_type and uptime_seconds in the returned GetServerInfoResponse alongside kv_connector and kv_role.
🤖 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 `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 295-320: GetServerInfo currently returns only
kv_connector/kv_role; compute uptime_seconds as int(time.time() -
self.start_time) and set server_type to the static string used elsewhere (e.g.,
"vllm") before returning the vllm_engine_pb2.GetServerInfoResponse. Update the
GetServerInfo method to import/use time, reference self.start_time to calculate
uptime_seconds, and include server_type and uptime_seconds in the returned
GetServerInfoResponse alongside kv_connector and kv_role.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 962eafd2-77ab-48fc-a787-35e2844c5e73
📒 Files selected for processing (1)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py
Replace the concrete AsyncLLM dependency with the EngineClient protocol interface, as suggested in vllm-project/vllm#36169. Changes: - Constructor takes EngineClient instead of AsyncLLM - Use self.engine.renderer directly instead of reaching through input_processor.input_preprocessor.renderer - Text prompt path now uses renderer.process_for_engine() which handles tokenization internally - Remove output_processor.get_num_unfinished_requests() from GetServerInfo — SMG strips active_requests, is_paused, last_receive_timestamp, uptime_seconds, and server_type during worker metadata discovery (discover_metadata.rs:268-276), so these fields were never consumed - Only kv_connector and kv_role are returned (used for PD routing) Signed-off-by: Chang Su <chang.s.su@oracle.com>
f0d6d90 to
9bd34be
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bd34be3a5
ℹ️ 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".
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 `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 121-124: The code incorrectly passes raw text via
self.engine.renderer.process_for_engine({"prompt": request.text}, ...) which
expects a tokenized TokensPrompt; instead, call the engine's generate flow with
raw text and tokenization parameters: use self.engine.generate(...) passing
request.text (or a PromptType containing the raw text) plus the prepared
tokenization_kwargs (the variable at line ~134) so the engine handles
tokenization; keep the tokenized path unchanged (which builds TokensPrompt and
calls process_for_engine), but for the text path remove the process_for_engine
call and invoke engine.generate with request.text and tokenization_kwargs to
match EngineClient's expected usage.
🪄 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: 6bcb7f9f-7239-4749-8b31-c06bdf5644f2
📒 Files selected for processing (2)
grpc_servicer/pyproject.tomlgrpc_servicer/smg_grpc_servicer/vllm/servicer.py
Address review feedback from #949: - Reject text prompts with a clear ValueError instead of passing them to process_for_engine which expects tokenized input. SMG always sends tokenized input via gRPC, so this path was dead code. The ValueError is caught and mapped to INVALID_ARGUMENT. - Consolidate time.time() into a single arrival_time variable reused across all branches. - Bump vllm dependency to >= 0.17.0 (process_for_engine added in v0.17.0, not v0.15.0 as previously stated). Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff4d5ed7e6
ℹ️ 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".
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 `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 309-319: Add a brief in-code comment above the return of
vllm_engine_pb2.GetServerInfoResponse explaining that active_requests,
is_paused, last_receive_timestamp, uptime_seconds, and server_type are
intentionally omitted and left at their proto defaults because SMG strips these
fields during worker metadata discovery (so the servicer only returns
kv_connector/kv_role derived from self.engine.vllm_config.kv_transfer_config);
reference the existing local symbols (GetServerInfoResponse, kv_connector,
kv_role, and self.engine.vllm_config.kv_transfer_config) so future maintainers
understand this is deliberate and not an oversight.
🪄 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: 22e41623-bb61-4d9c-b776-30d5c7a2c35f
📒 Files selected for processing (1)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py
1 participant
Description
Problem
The vLLM gRPC servicer directly depends on
AsyncLLM, a concrete implementation, rather than theEngineClientprotocol interface. This was flagged in vllm-project/vllm#36169 by @njhill.The servicer also accessed
output_processor.get_num_unfinished_requests()which is not onEngineClient, and reached throughinput_processor.input_preprocessor.rendererinstead of usingEngineClient.rendererdirectly.Solution
AsyncLLMtype withEngineClientprotocolself.engine.renderer.process_for_engine()directly (available onEngineClientsince vLLM v0.18.0)output_processor.get_num_unfinished_requests()fromGetServerInfo— SMG stripsactive_requests,is_paused,last_receive_timestamp,uptime_seconds, andserver_typeduring worker metadata discovery (discover_metadata.rs:268-276), so these fields were never consumed. Onlykv_connectorandkv_roleare returned (used for PD routing).async_llmas the constructor param name to avoid breaking the caller in vLLM'sgrpc_server.pyChanges
grpc_servicer/smg_grpc_servicer/vllm/servicer.py:AsyncLLM→EngineClientfrom vllm.v1.engine.async_llm import AsyncLLMself.engine.input_processor.input_preprocessor.renderer→self.engine.rendererrenderer.process_for_engine()instead ofinput_preprocessor.preprocess()GetServerInfo: return onlykv_connectorandkv_roleTest Plan
vllm serve --grpcwith tokenized input (normal SMG flow) → works as beforevllm serve --grpcwith text input → renderer handles tokenizationGetServerInfo→ returnskv_connector/kv_role, other fields default to zeroChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit