fix: grpc_servicer add check empty input and centralize tokenized validation#979
fix: grpc_servicer add check empty input and centralize tokenized validation#979gongwei-130 wants to merge 10 commits intomainfrom
Conversation
Signed-off-by: gongwei-130 <weigong28@gmail.com>
gongwei-130
requested review from
CatherineSue,
XinyueZhang369,
key4ng and
slin1237
as code owners
|
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:
📝 WalkthroughWalkthroughExtracted tokenized-input validation into a shared helper used by generate/embed conversion functions, added unit tests for that helper, made Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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: c66bdf7b47
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 a centralized validation helper, validate_tokenized_input, for SGLang gRPC request payloads. It refactors the _convert_generate_request and _convert_embed_request methods in the servicer to use this helper, which now also ensures that input_ids are not empty. Additionally, a new test suite has been added to verify the validation logic. I have no feedback to provide.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
.github/workflows/pr-test-rust.yml (1)
892-892:⚠️ Potential issue | 🟠 MajorAdd
grpc-servicer-unit-teststo thefinishjob'sneedsarray.The new job is not included in the
finishjob's dependency list, so CI failures ingrpc-servicer-unit-testswon't be reflected in the overall PR status.🐛 Proposed fix
finish: - needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, unit-tests, benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-4gpu-gateway, e2e-vendor, go-unit-tests, go-bindings-e2e] + needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, grpc-servicer-unit-tests, unit-tests, benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-4gpu-gateway, e2e-vendor, go-unit-tests, go-bindings-e2e]And add the corresponding failure check in the "Check CI result" step:
if [[ "${{ needs.pre-commit.result }}" == "failure" || \ "${{ needs.python-lint.result }}" == "failure" || \ "${{ needs.grpc-proto-build-check.result }}" == "failure" || \ "${{ needs.build-wheel.result }}" == "failure" || \ "${{ needs.python-unit-tests.result }}" == "failure" || \ + "${{ needs.grpc-servicer-unit-tests.result }}" == "failure" || \ "${{ needs.unit-tests.result }}" == "failure" || \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-test-rust.yml at line 892, The finish job's needs array is missing the new grpc-servicer-unit-tests dependency; update the finish job (the needs: [...] list) to include "grpc-servicer-unit-tests" so that the finish job waits on that job, and also add the corresponding check for grpc-servicer-unit-tests in the "Check CI result" step so its failure is reflected in the overall PR status.
🤖 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/pr-test-rust.yml:
- Around line 247-251: The CI job fails because the test imports
smg_grpc_servicer.sglang.validation (validate_tokenized_input) but the package
root isn't on PYTHONPATH; update the "Run grpc_servicer unit tests" step so the
package is importable by either exporting PYTHONPATH=grpc_servicer before
running pytest (so tests/test_sglang_validation.py can import
smg_grpc_servicer.sglang.validation) or install the package into the environment
(pip install -e ./grpc_servicer) prior to running pytest.
---
Outside diff comments:
In @.github/workflows/pr-test-rust.yml:
- Line 892: The finish job's needs array is missing the new
grpc-servicer-unit-tests dependency; update the finish job (the needs: [...]
list) to include "grpc-servicer-unit-tests" so that the finish job waits on that
job, and also add the corresponding check for grpc-servicer-unit-tests in the
"Check CI result" step so its failure is reflected in the overall PR status.
🪄 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: 75202845-4cfe-4d05-83a9-c2a02ebad111
📒 Files selected for processing (4)
.github/workflows/pr-test-rust.ymlgrpc_servicer/smg_grpc_servicer/sglang/servicer.pygrpc_servicer/smg_grpc_servicer/sglang/validation.pygrpc_servicer/tests/test_sglang_validation.py
Signed-off-by: gongwei-130 <weigong28@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a5eb77fd4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@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/pr-test-rust.yml:
- Around line 235-251: The new job grpc-servicer-unit-tests must be wired into
the finish job so its failure blocks merges: add "grpc-servicer-unit-tests" to
the finish job's needs list and include a check for its result in the finish
job's failure-checking script (the block that iterates job results / checks for
non-success between lines ~898-915); specifically update the arrays/conditions
that count failures to consider the grpc-servicer-unit-tests result and ensure
the final gate fails if that job returned failure or cancelled.
In `@grpc_servicer/tests/test_sglang_validation.py`:
- Around line 46-51: Add a symmetric success-path unit test for request_kind
"generate" in grpc_servicer/tests/test_sglang_validation.py: create a
_GrpcReqStub(tokenized=_Tokenized("world", [3, 4, 5])) and call
validate_tokenized_input(req, "generate"), then assert the returned input_text
equals "world" and input_ids equals [3, 4, 5]; mirror the existing
test_validate_tokenized_input_returns_text_and_ids structure and name it e.g.
test_validate_tokenized_input_returns_text_and_ids_generate to ensure coverage
for validate_tokenized_input across both "embed" and "generate" request kinds.
🪄 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: 25573af4-5735-42b7-8439-631ac2b7cc71
📒 Files selected for processing (3)
.github/workflows/pr-test-rust.ymlgrpc_servicer/smg_grpc_servicer/sglang/__init__.pygrpc_servicer/tests/test_sglang_validation.py
|
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.
♻️ Duplicate comments (1)
.github/workflows/pr-test-rust.yml (1)
235-251:⚠️ Potential issue | 🟠 MajorWire
grpc-servicer-unit-testsinto thefinishgate.Line 235 adds a new CI job, but it is not included in the
finishjob’sneeds(Line 891) or failure condition block (Line 898-Line 915). As-is, this job can fail without blocking merge.🐛 Proposed fix
- finish: - needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, unit-tests, benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-4gpu-gateway, e2e-vendor, go-unit-tests, go-bindings-e2e] + finish: + needs: [pre-commit, python-lint, grpc-proto-build-check, build-wheel, python-unit-tests, grpc-servicer-unit-tests, unit-tests, benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-chat, e2e-2gpu-responses, e2e-2gpu-pd, e2e-4gpu-chat, e2e-4gpu-gateway, e2e-vendor, go-unit-tests, go-bindings-e2e] @@ if [[ "${{ needs.pre-commit.result }}" == "failure" || \ "${{ needs.python-lint.result }}" == "failure" || \ "${{ needs.grpc-proto-build-check.result }}" == "failure" || \ "${{ needs.build-wheel.result }}" == "failure" || \ "${{ needs.python-unit-tests.result }}" == "failure" || \ + "${{ needs.grpc-servicer-unit-tests.result }}" == "failure" || \ "${{ needs.unit-tests.result }}" == "failure" || \ "${{ needs.benchmarks.result }}" == "failure" || \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pr-test-rust.yml around lines 235 - 251, The new CI job "grpc-servicer-unit-tests" is not wired into the final gating job ("finish"), so its failures won't block merges; update the "finish" job to include "grpc-servicer-unit-tests" in its needs list (the dependency array for the finish job) and add the same job name into the finish job’s failure condition block so a failing grpc-servicer-unit-tests run will mark the finish gate as failed. Locate the "grpc-servicer-unit-tests" job declaration and the "finish" job (referenced as finish and its needs/failure handling) and add the job name consistently to both places.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/pr-test-rust.yml:
- Around line 235-251: The new CI job "grpc-servicer-unit-tests" is not wired
into the final gating job ("finish"), so its failures won't block merges; update
the "finish" job to include "grpc-servicer-unit-tests" in its needs list (the
dependency array for the finish job) and add the same job name into the finish
job’s failure condition block so a failing grpc-servicer-unit-tests run will
mark the finish gate as failed. Locate the "grpc-servicer-unit-tests" job
declaration and the "finish" job (referenced as finish and its needs/failure
handling) and add the job name consistently to both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6a42308b-dc5f-4295-91ba-fa8a7eac86d0
📒 Files selected for processing (1)
.github/workflows/pr-test-rust.yml
Signed-off-by: gongwei-130 <weigong28@gmail.com>
|
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. |
Signed-off-by: gongwei-130 <weigong28@gmail.com>
|
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
🤖 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/tests/test_sglang_validation.py`:
- Around line 34-44: Consolidate the two duplicate tests into a single
parametrized test: replace the separate
test_validate_tokenized_input_rejects_empty_generate and
test_validate_tokenized_input_rejects_empty_embed with one test function (e.g.,
test_validate_tokenized_input_rejects_empty_for_kinds) decorated with
pytest.mark.parametrize over request_kind values ("generate", "embed"); inside
the test create the same _GrpcReqStub(tokenized=_Tokenized("hello", [])) and
call validate_tokenized_input(req, request_kind) asserting it raises ValueError
with the appropriate message (use match that can accept either "generate" or
"embed" dynamically, e.g., format or f-string), keeping references to
validate_tokenized_input, _GrpcReqStub, and _Tokenized so the intent and
assertions remain identical.
🪄 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: f0d67f18-19b5-4fd4-93e2-e8a3b5168776
📒 Files selected for processing (1)
grpc_servicer/tests/test_sglang_validation.py
| def test_validate_tokenized_input_rejects_empty_generate(self): | ||
| """Reject generate requests with empty input_ids.""" | ||
| req = _GrpcReqStub(tokenized=_Tokenized("hello", [])) | ||
| with pytest.raises(ValueError, match="input_ids cannot be empty for generate requests"): | ||
| validate_tokenized_input(req, "generate") | ||
|
|
||
| def test_validate_tokenized_input_rejects_empty_embed(self): | ||
| """Reject embed requests with empty input_ids.""" | ||
| req = _GrpcReqStub(tokenized=_Tokenized("hello", [])) | ||
| with pytest.raises(ValueError, match="input_ids cannot be empty for embed requests"): | ||
| validate_tokenized_input(req, "embed") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Parametrize duplicated empty-input_ids tests to reduce maintenance overhead.
Line 34-44 repeats the same assertion shape with only request_kind changed. Consider consolidating with pytest.mark.parametrize.
♻️ Suggested refactor
class TestSGLangValidation:
@@
- def test_validate_tokenized_input_rejects_empty_generate(self):
- """Reject generate requests with empty input_ids."""
- req = _GrpcReqStub(tokenized=_Tokenized("hello", []))
- with pytest.raises(ValueError, match="input_ids cannot be empty for generate requests"):
- validate_tokenized_input(req, "generate")
-
- def test_validate_tokenized_input_rejects_empty_embed(self):
- """Reject embed requests with empty input_ids."""
- req = _GrpcReqStub(tokenized=_Tokenized("hello", []))
- with pytest.raises(ValueError, match="input_ids cannot be empty for embed requests"):
- validate_tokenized_input(req, "embed")
+ `@pytest.mark.parametrize`("request_kind", ["generate", "embed"])
+ def test_validate_tokenized_input_rejects_empty_input_ids(self, request_kind: str):
+ """Reject requests with empty input_ids."""
+ req = _GrpcReqStub(tokenized=_Tokenized("hello", []))
+ with pytest.raises(
+ ValueError,
+ match=rf"input_ids cannot be empty for {request_kind} requests",
+ ):
+ validate_tokenized_input(req, request_kind)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@grpc_servicer/tests/test_sglang_validation.py` around lines 34 - 44,
Consolidate the two duplicate tests into a single parametrized test: replace the
separate test_validate_tokenized_input_rejects_empty_generate and
test_validate_tokenized_input_rejects_empty_embed with one test function (e.g.,
test_validate_tokenized_input_rejects_empty_for_kinds) decorated with
pytest.mark.parametrize over request_kind values ("generate", "embed"); inside
the test create the same _GrpcReqStub(tokenized=_Tokenized("hello", [])) and
call validate_tokenized_input(req, request_kind) asserting it raises ValueError
with the appropriate message (use match that can accept either "generate" or
"embed" dynamically, e.g., format or f-string), keeping references to
validate_tokenized_input, _GrpcReqStub, and _Tokenized so the intent and
assertions remain identical.
Signed-off-by: gongwei-130 <weigong28@gmail.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def __getattr__(name: str): | ||
| """Lazily expose `SGLangSchedulerServicer` to avoid eager heavy imports.""" | ||
| if name == "SGLangSchedulerServicer": | ||
| from smg_grpc_servicer.sglang.servicer import SGLangSchedulerServicer | ||
|
|
||
| return SGLangSchedulerServicer | ||
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
There was a problem hiding this comment.
🟡 Nit: __getattr__ is called on every attribute access that misses, not just the first. Without caching the result in globals(), repeated access (e.g. sglang.SGLangSchedulerServicer in a loop, or multiple from sglang import SGLangSchedulerServicer statements across modules) re-enters the import machinery each time. The overhead is small (Python caches modules in sys.modules), but the standard pattern caches the binding:
| def __getattr__(name: str): | |
| """Lazily expose `SGLangSchedulerServicer` to avoid eager heavy imports.""" | |
| if name == "SGLangSchedulerServicer": | |
| from smg_grpc_servicer.sglang.servicer import SGLangSchedulerServicer | |
| return SGLangSchedulerServicer | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") | |
| def __getattr__(name: str): | |
| """Lazily expose `SGLangSchedulerServicer` to avoid eager heavy imports.""" | |
| if name == "SGLangSchedulerServicer": | |
| from smg_grpc_servicer.sglang.servicer import SGLangSchedulerServicer | |
| globals()[name] = SGLangSchedulerServicer | |
| return SGLangSchedulerServicer | |
| raise AttributeError(f"module {__name__!r} has no attribute {name!r}") |
This is the pattern CPython's stdlib uses (e.g. importlib).
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: gongwei-130 <weigong28@gmail.com>
gongwei-130
changed the title
gongwei-130
changed the title
1 participant
Summary
smg_grpc_servicer/sglang/validation.pyvalidate_tokenized_inputfor both Generate and Embed requestsgrpc_servicer/tests/test_sglang_validation.pygrpc-servicer-unit-testsin.github/workflows/pr-test-rust.ymlWhat Changed
tokenizedfield must be presentinput_idsmust be non-empty (request-kind-specific error)servicer.pyis removedTest Plan
python3 -m py_compile grpc_servicer/smg_grpc_servicer/sglang/servicer.pypython3 -m py_compile grpc_servicer/smg_grpc_servicer/sglang/validation.pypython3 -m py_compile grpc_servicer/tests/test_sglang_validation.pygrpc-servicer-unit-testsrunspytest -q grpc_servicer/tests/test_sglang_validation.pyNotes
servicer.pyruntime dependenciesSummary by CodeRabbit
Tests
Chores
Refactor