feat: add smg-grpc-servicer package - #638
Conversation
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
github-actions
Bot
added
documentation
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 refactors the gRPC serving layer for vLLM by extracting it into a dedicated, independently releasable Python package, Highlights
Changelog
Ignored Files
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
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 a new smg-grpc-servicer package with a vLLM-based async gRPC server and VllmEngineServicer (streaming Generate and other RPCs), packaging and docs for the servicer, CI scripts to install local gRPC packages, and a unified GitHub Actions workflow to build and publish proto and servicer packages to PyPI. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC_Server as gRPC Server
participant Servicer as VllmEngineServicer
participant Engine as AsyncLLM
Client->>GRPC_Server: Stream GenerateRequest
GRPC_Server->>Servicer: route to Generate()
Servicer->>Servicer: validate request, build inputs & SamplingParams
Servicer->>Engine: request generation (async)
Engine-->>Servicer: stream RequestOutput chunks
loop per chunk
Servicer->>Servicer: assemble tokens, logprobs, metadata
Servicer-->>Client: yield GenerateResponse (chunk)
end
Engine-->>Servicer: final output
Servicer-->>Client: yield final GenerateResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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: 31e4aedf03
ℹ️ 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".
Signed-off-by: Chang Su <chang.s.su@oracle.com>
![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 successfully extracts the gRPC servicer logic into a new standalone Python package, smg-grpc-servicer, which is a good architectural improvement for decoupling release cycles. The new package structure, dependencies, and documentation are well-defined, and the core servicer logic handles streaming, non-streaming, and multimodal generation cases. However, several security vulnerabilities were identified, including a critical Denial of Service risk due to unrestricted gRPC message sizes, a high-severity SSRF vulnerability via unvalidated KV transfer parameters, and medium-severity issues related to missing authorization in the Abort RPC and insecure communication by default. Addressing these issues is essential before deploying this servicer in a production environment. Additionally, for general improvements, the last_receive_timestamp metric in GetServerInfo within servicer.py is not being tracked correctly, and it's recommended to use uv pip instead of pip in ci_install_vllm.sh for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-grpc-servicer.yml:
- Around line 4-13: The push trigger can fire when both
grpc_servicer/pyproject.toml and grpc_client/python/pyproject.toml are changed,
bypassing the intended workflow_run ordering; update the push trigger to ignore
changes to the client pyproject by adding a paths-ignore entry for
"grpc_client/python/pyproject.toml" (so push only fires for servicer-only
changes), and apply the same paths-ignore change to the other similar push block
referenced in the file; locate the push block and the workflow_run block
(symbols: push, paths, workflow_run) and add the paths-ignore entry accordingly.
In `@grpc_servicer/pyproject.toml`:
- Around line 10-15: Add uvloop as an explicit dependency in pyproject.toml to
match its direct import in smg_grpc_servicer/vllm/server.py; update the
dependencies list to include "uvloop>=0.17.0" (or a compatible minimum) so the
package no longer relies on a transitive dependency from vllm, and ensure the
project build/publish includes this new declared dependency.
In `@grpc_servicer/smg_grpc_servicer/vllm/server.py`:
- Around line 79-111: The startup and serve lifecycle isn't protected by an
outer try/finally so if an exception happens during startup the cleanup (notably
async_llm.shutdown()) may be skipped; wrap the sequence that creates/starts the
server and awaits stop_event (including await server.start(),
loop.add_signal_handler setup, and the await stop_event.wait()) in a single
try/finally block and call async_llm.shutdown() and await server.stop(grace=5.0)
in the finally to guarantee cleanup; also remove/restore any signal handlers
registered via loop.add_signal_handler after shutdown to avoid leaking handlers.
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 202-204: The except block in the Generate handler currently sends
raw exception text back to the client via await
context.abort(grpc.StatusCode.INTERNAL, str(e)) which can leak internal details;
instead keep the logger.exception(...) call for detailed server-side logging (it
already logs request_id and traceback) but change the context.abort call in the
except block to send a generic message (e.g., "Internal server error" or "An
internal error occurred") rather than str(e); update the code around the
Generate handler’s exception handling (references: logger.exception,
context.abort, request_id, exception variable e) accordingly.
- Around line 371-406: The code trusts multimodal metadata (flat, hf_dict
entries, and placeholder ranges) before indexing; add explicit validation to
return INVALID_ARGUMENT for malformed inputs: ensure each key used (flat[key]
and hf_dict[...] in the block building sizes and fields_config) exists in flat
and hf_dict before accessing, verify sizes.flatten() is valid tensor type before
to(torch.int64), and when iterating mm_proto.mm_placeholders validate each
PlaceholderRange p has non-negative offset and length and that p.offset +
p.length does not exceed len(prompt_token_ids) before slicing; also check
mm_proto.HasField("im_token_id") and that im_token_id is a valid token value
before comparing, and when any validation fails raise a clear INVALID_ARGUMENT
error (rather than letting indexing or torch ops raise INTERNAL). Use the
existing symbols (flat, hf_dict, sizes, MultiModalFieldConfig.flat_from_sizes,
mm_proto, prompt_token_ids, PlaceholderRange) to locate where to insert these
guards.
- Around line 321-323: The health/status response is setting
last_receive_timestamp to time.time() at read time, which is wrong; instead
track and return the actual last request arrival time: add a persistent
attribute (e.g., self.last_receive_timestamp initialized in the servicer class
constructor near self.start_time), update self.last_receive_timestamp in the
request ingress handler where new requests arrive, and in the status response
use that stored self.last_receive_timestamp (leave uptime_seconds = time.time()
- self.start_time as-is). Ensure the identifier last_receive_timestamp in the
response maps to the stored self.last_receive_timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 715b876e-7429-4c27-bf6f-051fb835b219
📒 Files selected for processing (9)
.github/workflows/release-grpc-servicer.ymlgrpc_servicer/DEVELOPMENT.mdgrpc_servicer/README.mdgrpc_servicer/pyproject.tomlgrpc_servicer/smg_grpc_servicer/__init__.pygrpc_servicer/smg_grpc_servicer/vllm/__init__.pygrpc_servicer/smg_grpc_servicer/vllm/server.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pyscripts/ci_install_vllm.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30941ac367
ℹ️ 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
♻️ Duplicate comments (3)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py (3)
202-204:⚠️ Potential issue | 🟠 MajorAvoid leaking internal exception details in gRPC INTERNAL errors.
context.abort(..., str(e))can expose internal state and implementation details to clients. Return a generic message and keep details in server logs.🔒 Suggested fix
- except Exception as e: + except Exception: logger.exception("Error in Generate for request %s", request_id) - await context.abort(grpc.StatusCode.INTERNAL, str(e)) + await context.abort(grpc.StatusCode.INTERNAL, "Internal server error")🤖 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 202 - 204, The catch block in Generate is currently calling await context.abort(grpc.StatusCode.INTERNAL, str(e)), which leaks internal exception details to clients; change it to send a generic error message (e.g., "Internal server error") while keeping the full exception in server logs (logger.exception("Error in Generate for request %s", request_id) already present) so remove str(e) from the abort and replace with the generic message in the except block of Generate in servicer.py.
321-323:⚠️ Potential issue | 🟡 MinorTrack and return actual last request ingress time.
last_receive_timestamp=time.time()reports “now” at read time, not the last request arrival. Persist it on ingress and return the stored value.🕒 Suggested fix
@@ def __init__(self, async_llm: AsyncLLM, start_time: float): @@ self.async_llm = async_llm self.start_time = start_time + self.last_receive_timestamp = start_time logger.info("VllmEngineServicer initialized") @@ async def Generate( @@ request_id = request.request_id + self.last_receive_timestamp = time.time() @@ return vllm_engine_pb2.GetServerInfoResponse( @@ - last_receive_timestamp=time.time(), # TODO looks wrong? + last_receive_timestamp=self.last_receive_timestamp, uptime_seconds=time.time() - self.start_time,🤖 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 321 - 323, The code is returning time.time() for last_receive_timestamp instead of the actual last ingress time; persist and update a field (e.g., self.last_receive_timestamp) when requests arrive in the request-ingress handler (the method that receives/queues requests) and return that stored value here instead of calling time.time(); keep uptime_seconds computed as time.time() - self.start_time and ensure self.last_receive_timestamp is initialized (e.g., None or start_time) in the servicer constructor where self.start_time is set.
371-406:⚠️ Potential issue | 🟠 MajorValidate multimodal metadata before indexing/slicing.
flat_keysand placeholder ranges are currently trusted. Malformed metadata can trigger runtime failures or incorrect masking behavior instead of a cleanINVALID_ARGUMENT.🧪 Suggested fix
for key in hf_dict: on_cpu = key in cpu_keys if key in batched: fields_config[key] = MultiModalFieldConfig.batched("image", keep_on_cpu=on_cpu) elif key in flat: - sizes = hf_dict[flat[key]].flatten().to(torch.int64) + size_key = flat[key] + if size_key not in hf_dict: + raise ValueError( + f"Invalid mm_inputs.flat_keys mapping: {key!r} -> {size_key!r} (missing tensor)" + ) + sizes = hf_dict[size_key].flatten().to(torch.int64) fields_config[key] = MultiModalFieldConfig.flat_from_sizes( "image", sizes, keep_on_cpu=on_cpu ) @@ placeholders = [] for p in mm_proto.mm_placeholders: + if p.offset < 0 or p.length < 0 or p.offset + p.length > len(prompt_token_ids): + raise ValueError( + "Invalid mm placeholder range: " + f"offset={p.offset}, length={p.length}, prompt_len={len(prompt_token_ids)}" + ) is_embed = None if im_token_id is not None: token_slice = prompt_token_ids[p.offset : p.offset + p.length]🤖 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 371 - 406, Validate multimodal metadata before using it: check that keys referenced in flat (used as flat[key] and hf_dict[flat[key]]) actually exist and that resulting tensors/sizes have expected dims/lengths before calling flatten()/to(); validate mm_proto.mm_hashes types/lengths if present; for each PlaceholderRange p ensure p.offset and p.length are non-negative and that p.offset + p.length does not exceed len(prompt_token_ids) before slicing prompt_token_ids[p.offset : p.offset + p.length]; also ensure im_token_id presence is correctly checked via HasField and that mask length matches slice length; on any invalid metadata raise a clear INVALID_ARGUMENT error (or propagate an appropriate grpc status) rather than proceeding.
🤖 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 127-133: The current check only rejects port 0; update the
validation around request.kv_transfer_params (remote_host and remote_port) in
servicer.py to ensure remote_port is an integer within the valid TCP/UDP port
range (1–65535) and remote_host is non-empty, and call await
context.abort(grpc.StatusCode.INVALID_ARGUMENT, ...) if remote_port is outside
that range or not provided; keep the existing error message or improve it to
mention the allowed port range so sampling args are never built with an invalid
port.
---
Duplicate comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 202-204: The catch block in Generate is currently calling await
context.abort(grpc.StatusCode.INTERNAL, str(e)), which leaks internal exception
details to clients; change it to send a generic error message (e.g., "Internal
server error") while keeping the full exception in server logs
(logger.exception("Error in Generate for request %s", request_id) already
present) so remove str(e) from the abort and replace with the generic message in
the except block of Generate in servicer.py.
- Around line 321-323: The code is returning time.time() for
last_receive_timestamp instead of the actual last ingress time; persist and
update a field (e.g., self.last_receive_timestamp) when requests arrive in the
request-ingress handler (the method that receives/queues requests) and return
that stored value here instead of calling time.time(); keep uptime_seconds
computed as time.time() - self.start_time and ensure self.last_receive_timestamp
is initialized (e.g., None or start_time) in the servicer constructor where
self.start_time is set.
- Around line 371-406: Validate multimodal metadata before using it: check that
keys referenced in flat (used as flat[key] and hf_dict[flat[key]]) actually
exist and that resulting tensors/sizes have expected dims/lengths before calling
flatten()/to(); validate mm_proto.mm_hashes types/lengths if present; for each
PlaceholderRange p ensure p.offset and p.length are non-negative and that
p.offset + p.length does not exceed len(prompt_token_ids) before slicing
prompt_token_ids[p.offset : p.offset + p.length]; also ensure im_token_id
presence is correctly checked via HasField and that mask length matches slice
length; on any invalid metadata raise a clear INVALID_ARGUMENT error (or
propagate an appropriate grpc status) rather than proceeding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 25cc250e-49fc-4f52-9698-005f755b4060
📒 Files selected for processing (1)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py
- use uv pip in CI script for consistency - wrap server startup in try/finally for guaranteed cleanup - guard islice against negative logprobs values - validate kv_transfer_params port range [1, 65535] Signed-off-by: Chang Su <chang.s.su@oracle.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
grpc_servicer/smg_grpc_servicer/vllm/servicer.py (2)
202-204:⚠️ Potential issue | 🟠 MajorAvoid leaking raw exception details in INTERNAL responses.
Returning
str(e)to clients can expose internal state (stack frames, file paths, internal identifiers). Keep details in server logs and send a generic message to clients.🔒 Suggested fix
- except Exception as e: - logger.exception("Error in Generate for request %s", request_id) - await context.abort(grpc.StatusCode.INTERNAL, str(e)) + except Exception: + logger.exception("Error in Generate for request %s", request_id) + await context.abort(grpc.StatusCode.INTERNAL, "Internal server error")🤖 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 202 - 204, In the Generate handler's exception block (the except Exception in servicer.py around the Generate method), stop sending str(e) to the client via await context.abort and instead send a generic message (e.g., "Internal server error") while keeping the full exception details in the server log via logger.exception; replace await context.abort(grpc.StatusCode.INTERNAL, str(e)) with await context.abort(grpc.StatusCode.INTERNAL, "<generic message>") and ensure logger.exception(...) still records e and request_id for debugging.
367-408:⚠️ Potential issue | 🟠 MajorValidate multimodal metadata before indexing.
flat_keysmappings and placeholder ranges are trusted without validation. Malformed inputs will raiseKeyErroror out-of-bounds errors that surface asINTERNALrather thanINVALID_ARGUMENT.🛡️ Suggested validation
for key in hf_dict: on_cpu = key in cpu_keys if key in batched: fields_config[key] = MultiModalFieldConfig.batched("image", keep_on_cpu=on_cpu) elif key in flat: + size_key = flat[key] + if size_key not in hf_dict: + raise ValueError( + f"Invalid mm_inputs.flat_keys: {key!r} -> {size_key!r} (tensor not found)" + ) - sizes = hf_dict[flat[key]].flatten().to(torch.int64) + sizes = hf_dict[size_key].flatten().to(torch.int64) fields_config[key] = MultiModalFieldConfig.flat_from_sizes( "image", sizes, keep_on_cpu=on_cpu ) @@ for p in mm_proto.mm_placeholders: + if p.offset < 0 or p.length < 0: + raise ValueError(f"Invalid placeholder: offset={p.offset}, length={p.length}") + if p.offset + p.length > len(prompt_token_ids): + raise ValueError( + f"Placeholder out of bounds: offset={p.offset}, length={p.length}, " + f"prompt_len={len(prompt_token_ids)}" + ) is_embed = None🤖 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 367 - 408, The code assumes multimodal metadata (keys in flat/batched, indices in flat mapping, and placeholder ranges) are valid and can raise KeyError or index errors; update validation in the vllm servicer before using hf_dict/flat/batched and mm_proto.mm_placeholders: verify that every flat[key] exists in hf_dict and that sizes tensors are non-empty and convertible to int64 before calling MultiModalFieldConfig.flat_from_sizes, and check placeholder offsets/lengths against prompt_token_ids length and that im_token_id handling uses HasField correctly before creating PlaceholderRange; on invalid inputs, raise/return an INVALID_ARGUMENT style error (or convert to a well-defined gRPC status) rather than letting KeyError/IndexError propagate.grpc_servicer/smg_grpc_servicer/vllm/server.py (1)
46-77: 🧹 Nitpick | 🔵 TrivialConsider wrapping more setup in try/finally for complete cleanup.
If an exception occurs between
AsyncLLM.from_vllm_config()(line 46) and thetryblock (line 79)—e.g., duringserver.add_insecure_port()with an invalid address—async_llm.shutdown()would not be called.While the risk is low since most operations here don't fail, for robustness you could either:
- Move the try block earlier to wrap servicer/server creation
- Use a context manager pattern for AsyncLLM lifecycle
🛠️ Example restructure
# Create AsyncLLM async_llm = AsyncLLM.from_vllm_config( vllm_config=vllm_config, usage_context=UsageContext.OPENAI_API_SERVER, enable_log_requests=args.enable_log_requests, disable_log_stats=args.disable_log_stats, ) + try: # Create servicer servicer = VllmEngineServicer(async_llm, start_time) # Create gRPC server server = grpc.aio.server(...) ... - - try: # Start server await server.start() ... finally: ... async_llm.shutdown()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@grpc_servicer/smg_grpc_servicer/vllm/server.py` around lines 46 - 77, The setup between AsyncLLM.from_vllm_config(...) and server.add_insecure_port(...) can raise and leak resources because async_llm.shutdown() is only called inside the later try block; wrap the creation of AsyncLLM, VllmEngineServicer, and the gRPC server (i.e. the calls to AsyncLLM.from_vllm_config, VllmEngineServicer(...), grpc.aio.server(...), adding the servicer, enabling reflection, and server.add_insecure_port(...)) in a try/finally (or use a context manager) so that async_llm.shutdown() is always called in the finally block (and server cleanup is performed) if any of these steps fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@grpc_servicer/smg_grpc_servicer/vllm/server.py`:
- Around line 46-77: The setup between AsyncLLM.from_vllm_config(...) and
server.add_insecure_port(...) can raise and leak resources because
async_llm.shutdown() is only called inside the later try block; wrap the
creation of AsyncLLM, VllmEngineServicer, and the gRPC server (i.e. the calls to
AsyncLLM.from_vllm_config, VllmEngineServicer(...), grpc.aio.server(...), adding
the servicer, enabling reflection, and server.add_insecure_port(...)) in a
try/finally (or use a context manager) so that async_llm.shutdown() is always
called in the finally block (and server cleanup is performed) if any of these
steps fail.
In `@grpc_servicer/smg_grpc_servicer/vllm/servicer.py`:
- Around line 202-204: In the Generate handler's exception block (the except
Exception in servicer.py around the Generate method), stop sending str(e) to the
client via await context.abort and instead send a generic message (e.g.,
"Internal server error") while keeping the full exception details in the server
log via logger.exception; replace await context.abort(grpc.StatusCode.INTERNAL,
str(e)) with await context.abort(grpc.StatusCode.INTERNAL, "<generic message>")
and ensure logger.exception(...) still records e and request_id for debugging.
- Around line 367-408: The code assumes multimodal metadata (keys in
flat/batched, indices in flat mapping, and placeholder ranges) are valid and can
raise KeyError or index errors; update validation in the vllm servicer before
using hf_dict/flat/batched and mm_proto.mm_placeholders: verify that every
flat[key] exists in hf_dict and that sizes tensors are non-empty and convertible
to int64 before calling MultiModalFieldConfig.flat_from_sizes, and check
placeholder offsets/lengths against prompt_token_ids length and that im_token_id
handling uses HasField correctly before creating PlaceholderRange; on invalid
inputs, raise/return an INVALID_ARGUMENT style error (or convert to a
well-defined gRPC status) rather than letting KeyError/IndexError propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2f55268-4f86-4dc7-b2a8-89d608d82c64
📒 Files selected for processing (3)
grpc_servicer/smg_grpc_servicer/vllm/server.pygrpc_servicer/smg_grpc_servicer/vllm/servicer.pyscripts/ci_install_vllm.sh
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 383899940a
ℹ️ 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".
Replace separate release-grpc-proto.yml and release-grpc-servicer.yml with a single workflow that enforces proto-first ordering via job dependencies. Fixes race when both pyproject.toml files change in one merge. Signed-off-by: Chang Su <chang.s.su@oracle.com>
context.abort() raises grpc.aio.AbortError which was caught by the broad except Exception handler, remapping INVALID_ARGUMENT to INTERNAL. 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: 231531a903
ℹ️ 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: 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 @.github/workflows/release-grpc.yml:
- Around line 35-37: The current package-change detection uses a single-commit
diff (git diff HEAD~1) which misses multi-commit pushes; update the steps that
set the proto and servicer flags (the proto and servicer variable assignments)
to use the full push commit range by diffing from ${{ github.event.before }} to
${{ github.sha }} instead of HEAD~1, and ensure the workflow checkout step
increases fetch-depth to 0 so both commits are available locally; keep the
output writing to GITHUB_OUTPUT as-is.
- Around line 102-108: The job currently only declares needs: [detect,
upload-proto] and checks needs.upload-proto.result != 'failure', which lets
build-servicer run when build-proto was skipped/failed; add build-proto to the
needs list and tighten the if condition to require a successful proto build.
Specifically, update needs to include build-proto (needs: [detect, build-proto,
upload-proto]) and change the if expression to include needs.build-proto.result
== 'success' (e.g., always() && needs.detect.outputs.servicer == 'true' &&
needs.build-proto.result == 'success' && needs.upload-proto.result != 'failure')
so the servicer job only proceeds when the proto build succeeded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04665434-67f8-4c4f-932c-1ac665042bfe
📒 Files selected for processing (3)
.github/workflows/release-grpc-proto.yml.github/workflows/release-grpc.ymlgrpc_servicer/smg_grpc_servicer/vllm/servicer.py
💤 Files with no reviewable changes (1)
- .github/workflows/release-grpc-proto.yml
- Use github.event.before..github.sha instead of HEAD~1 to detect changes across multi-commit pushes - Use fetch-depth: 0 to ensure full history is available - Add build-proto to servicer needs and tighten condition to block servicer release when proto build fails (skipped != failure) 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 @.github/workflows/release-grpc.yml:
- Around line 35-37: Detect when github.event.before (the variable assigned to
base) is the 40-zero SHA and handle that case before running git diff; if base
equals "0000000000000000000000000000000000000000" then populate changed using an
alternative command such as listing files in the head commit (e.g., use git
ls-tree -r --name-only "$head" or git diff --name-only "$head"^ "$head" when
appropriate) instead of running git diff "$base" "$head", otherwise continue
using changed="$(git diff --name-only "$base" "$head")". Ensure you reference
the base, head and changed variables in the conditional so the workflow remains
robust for initial/edge pushes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2afbaf05-50ef-4965-8541-6cd5a1402bb0
📒 Files selected for processing (1)
.github/workflows/release-grpc.yml
Move context.abort() call outside the try/except Exception scope so it can't be caught by the broad handler. This removes the need for the grpc.aio.AbortError re-raise workaround. 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: fb35905d0e
ℹ️ 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".
1 participant
Description
Problem
The gRPC servicer code (VllmEngineServicer + server launcher) currently lives inside the vLLM repo, tightly coupled to their release cycle. We need to control our own release cadence for the gRPC layer independently.
Solution
Extract the servicer into a standalone PyPI package
smg-grpc-servicerthat depends onvllmandsmg-grpc-proto. vLLM gains a--grpcflag (vllm serve --grpc) that lazy-imports from this package. No circular dependency — vLLM optionally depends on smg-grpc-servicer at runtime.Changes
grpc_servicer/— New Python package (smg-grpc-servicerv0.4.2)smg_grpc_servicer/vllm/servicer.py— VllmEngineServicer (copied from vLLM, updated imports to usesmg_grpc_proto)smg_grpc_servicer/vllm/server.py—serve_grpc()launcher + standalonemain()pyproject.toml— Dependencies:smg-grpc-proto>=0.4.2,vllm>=0.16.0,grpcio>=1.78.0.github/workflows/release-grpc-servicer.yml— PyPI release workflow withworkflow_runsequencing (waits for proto release if both change)scripts/ci_install_vllm.sh— Install grpc packages from source for CI testinggrpc_servicer/README.mdandDEVELOPMENT.md— Usage and release workflow docsCompanion vLLM PR: adds
--grpcflag, removes oldvllm/grpc/andvllm/entrypoints/grpc_server.py.Test Plan
Manually tested
vllm serve --grpcwith gpt-oss-20b on 4xGPU. Screenshots attached below.n>1curl request
Checklist
ruffpassesruff formatpassescodespellpassesSummary by CodeRabbit
New Features
Documentation
Chores