Skip to content
Closed
20 changes: 19 additions & 1 deletion .github/workflows/pr-test-rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,23 @@ jobs:
python3 -m pip install pytest pytest-cov pytest-xdist
pytest -q tests --cov=smg --cov-config=.coveragerc --cov-report=term-missing --cov-fail-under=80

grpc-servicer-unit-tests:
Comment thread
gongwei-130 marked this conversation as resolved.
runs-on: k8s-runner-cpu
permissions:
contents: read
steps:
- uses: actions/checkout@v6

- name: Set up Python
uses: actions/setup-python@v6
with:
python-version: "3.13"

- name: Run grpc_servicer unit tests
run: |
python3 -m pip install pytest
PYTHONPATH=grpc_servicer pytest -q grpc_servicer/tests/

Comment thread
coderabbitai[bot] marked this conversation as resolved.
unit-tests:
runs-on: k8s-runner-cpu
permissions:
Expand Down Expand Up @@ -886,7 +903,7 @@ jobs:
path: benchmark_go_bindings/

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]
if: always()
runs-on: k8s-runner-cpu
permissions: {}
Expand All @@ -898,6 +915,7 @@ jobs:
"${{ 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" || \
"${{ needs.e2e-1gpu-chat.result }}" == "failure" || \
Expand Down
13 changes: 10 additions & 3 deletions grpc_servicer/smg_grpc_servicer/sglang/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
"""SGLang gRPC servicer — implements SglangScheduler proto service."""

from smg_grpc_servicer.sglang.servicer import SGLangSchedulerServicer
"""SGLang gRPC servicer package."""

__all__ = ["SGLangSchedulerServicer"]


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}")
Comment on lines +6 to +12

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

Suggested change
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).

17 changes: 5 additions & 12 deletions grpc_servicer/smg_grpc_servicer/sglang/servicer.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from smg_grpc_servicer.sglang.health_servicer import SGLangHealthServicer
from smg_grpc_servicer.sglang.request_manager import GrpcRequestManager
from smg_grpc_servicer.sglang.utils import abort_code_from_output
from smg_grpc_servicer.sglang.validation import validate_tokenized_input

logger = logging.getLogger(__name__)
HEALTH_CHECK_TIMEOUT = int(os.getenv("SGLANG_HEALTH_CHECK_TIMEOUT", 20))
Expand Down Expand Up @@ -699,12 +700,8 @@ def _convert_generate_request(
) -> TokenizedGenerateReqInput:
"""Convert gRPC GenerateRequest to internal format."""

# Extract tokenized input
if not grpc_req.HasField("tokenized"):
raise ValueError("Tokenized input must be provided")

input_text = grpc_req.tokenized.original_text
input_ids = list(grpc_req.tokenized.input_ids)
# Extract and validate tokenized input
input_text, input_ids = validate_tokenized_input(grpc_req, "generate")

# Convert sampling params
sampling_params = self._convert_sampling_params(grpc_req.sampling_params)
Expand Down Expand Up @@ -805,12 +802,8 @@ def _convert_embed_request(
) -> TokenizedEmbeddingReqInput:
"""Convert gRPC EmbedRequest to internal format."""

# Extract tokenized input
if not grpc_req.HasField("tokenized"):
raise ValueError("Tokenized input must be provided")

input_text = grpc_req.tokenized.original_text
input_ids = list(grpc_req.tokenized.input_ids)
# Extract and validate tokenized input
input_text, input_ids = validate_tokenized_input(grpc_req, "embed")

# Convert sampling params
sampling_params = self._convert_sampling_params(grpc_req.sampling_params)
Expand Down
16 changes: 16 additions & 0 deletions grpc_servicer/smg_grpc_servicer/sglang/validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
"""Validation helpers for SGLang gRPC request payloads."""

from typing import Any


def validate_tokenized_input(grpc_req: Any, request_kind: str) -> tuple[str, list[int]]:
"""Validate tokenized field and return (original_text, input_ids)."""
if not grpc_req.HasField("tokenized"):
raise ValueError("Tokenized input must be provided")

input_text = grpc_req.tokenized.original_text
input_ids = list(grpc_req.tokenized.input_ids)
if not input_ids:
raise ValueError(f"input_ids cannot be empty for {request_kind} requests")

return input_text, input_ids
61 changes: 61 additions & 0 deletions grpc_servicer/tests/test_sglang_validation.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
"""
Unit tests for SGLang request validation helpers.

These tests focus on validating tokenized request input in isolation.
"""

import pytest
from smg_grpc_servicer.sglang.validation import validate_tokenized_input
Comment thread
gongwei-130 marked this conversation as resolved.


class _Tokenized:
def __init__(self, original_text: str, input_ids: list[int]):
"""Store tokenized payload fields used by validation tests."""
self.original_text = original_text
self.input_ids = input_ids


class _GrpcReqStub:
def __init__(self, tokenized: _Tokenized | None):
"""Mimic a protobuf request object with an optional `tokenized` field."""
self.tokenized = tokenized

def HasField(self, field_name: str) -> bool: # noqa: N802
"""Match protobuf HasField behavior for the `tokenized` presence check."""
return field_name == "tokenized" and self.tokenized is not None


class TestSGLangValidation:
"""Test tokenized input validation behavior."""

def test_validate_tokenized_input_rejects_missing_tokenized(self):
"""Reject requests without tokenized payload."""
req = _GrpcReqStub(tokenized=None)
with pytest.raises(ValueError, match="Tokenized input must be provided"):
validate_tokenized_input(req, "generate")

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")
Comment on lines +37 to +47

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


def test_validate_tokenized_input_returns_text_and_ids(self):
"""Return original text and ids for valid tokenized payload."""
req = _GrpcReqStub(tokenized=_Tokenized("hello", [1, 2]))
input_text, input_ids = validate_tokenized_input(req, "embed")
assert input_text == "hello"
assert input_ids == [1, 2]
Comment thread
coderabbitai[bot] marked this conversation as resolved.

def test_validate_tokenized_input_returns_text_and_ids_generate(self):
"""Return original text and ids for valid generate tokenized payload."""
req = _GrpcReqStub(tokenized=_Tokenized("world", [3, 4, 5]))
input_text, input_ids = validate_tokenized_input(req, "generate")
assert input_text == "world"
assert input_ids == [3, 4, 5]
Loading