[None][refactor] Organize SMG gRPC adapter by protocol - #17179
Conversation
Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
|
/bot run |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe gRPC serve path now uses the SMG server launcher. The change adds optional SMG bindings, server lifecycle handling, adapter import updates, dependency metadata, and coverage for installed and missing SMG dependencies. ChangesSMG gRPC integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ServeCommand
participant launch_smg_server
participant LLMEngine
participant RequestManager
participant SMGServicer
participant grpcServer
ServeCommand->>launch_smg_server: Pass host, port, LLM arguments, and model name
launch_smg_server->>LLMEngine: Construct selected backend
launch_smg_server->>RequestManager: Create request manager
launch_smg_server->>SMGServicer: Register SMG servicer
launch_smg_server->>grpcServer: Configure, bind, and start
grpcServer-->>launch_smg_server: Signal or interruption
launch_smg_server->>grpcServer: Stop gracefully
launch_smg_server->>LLMEngine: Shut down engine
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
tensorrt_llm/grpc/smg/server.py (1)
117-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
except KeyboardInterruptbranch.
loop.add_signal_handler(signal.SIGINT, signal_handler)at Line 115 replaces the defaultSIGINThandling for this event loop. Once registered,SIGINTno longer raisesKeyboardInterruptinside the coroutine; it invokessignal_handler, which setsstop_event. Theexcept KeyboardInterruptat Line 119 therefore does not trigger during normal operation after Line 115 runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/grpc/smg/server.py` around lines 117 - 120, Remove the unreachable KeyboardInterrupt handler around stop_event.wait in the server startup flow. Keep loop.add_signal_handler(signal.SIGINT, signal_handler) and the existing stop_event-based shutdown behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/grpc/smg/server.py`:
- Around line 62-129: Widen the cleanup scope in the server lifecycle so the
existing shutdown logic covering llm.shutdown() also handles failures from
server.add_insecure_port(), server.start(), and subsequent startup or serving
operations after LLM construction. Ensure cleanup runs exactly once for any
post-construction exception while preserving the current graceful server.stop()
behavior.
---
Nitpick comments:
In `@tensorrt_llm/grpc/smg/server.py`:
- Around line 117-120: Remove the unreachable KeyboardInterrupt handler around
stop_event.wait in the server startup flow. Keep
loop.add_signal_handler(signal.SIGINT, signal_handler) and the existing
stop_event-based shutdown behavior unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 822a751f-5f58-42a0-9edd-1f2f2d73f128
📒 Files selected for processing (8)
tensorrt_llm/commands/serve.pytensorrt_llm/grpc/__init__.pytensorrt_llm/grpc/smg/__init__.pytensorrt_llm/grpc/smg/bindings.pytensorrt_llm/grpc/smg/request_manager.pytensorrt_llm/grpc/smg/server.pytensorrt_llm/grpc/smg/servicer.pytests/unittest/grpc/smg/test_smg.py
|
PR_Github #63353 [ run ] triggered by Bot. Commit: |
|
Reviewed the move itself: Three things worth settling before merge:
The |
|
PR_Github #63353 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63365 [ run ] triggered by Bot. Commit: |
|
PR_Github #63365 [ run ] completed with state
|
|
On the two red runs — neither looks like your refactor. Build 51350 (#63365) ran 27211 passing tests, 0 test-level failures; the only FAILED entries are That is also why I wouldn't read the earlier build 51341 as signal either — it only got 81 tests in before dying. Nothing new from me on the diff itself; my earlier notes stand (the |
Move the SMG gRPC adapter's smg-grpc-proto package out of the default install into an opt-in extra (tensorrt_llm[grpc-smg]), so a default TensorRT-LLM install no longer ships a gateway-specific protobuf package. - requirements.txt: drop smg-grpc-proto; add it to requirements-dev.txt so it stays present in every CI/dev environment (test coverage preserved). - setup.py: add the grpc-smg extra. - grpc/smg/bindings.py, commands/serve.py: guard the import and emit an actionable "pip install tensorrt_llm[grpc-smg]" hint instead of a bare ImportError. - tests: importorskip-guard test_smg.py; add test_grpc_optional.py covering the actionable-error and present-path smoke; register both under l0_a10. Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
|
/bot run |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/grpc/smg/bindings.py`:
- Around line 18-25: Update the import guard around trtllm_service_pb2 and
trtllm_service_pb2_grpc to catch ModuleNotFoundError only when e.name equals
"smg_grpc_proto"; preserve the existing optional-package error for that case,
and re-raise all other import failures unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bf8b014c-c62a-42f9-a3ed-9c392a0a2ff1
📒 Files selected for processing (8)
requirements-dev.txtrequirements.txtsetup.pytensorrt_llm/commands/serve.pytensorrt_llm/grpc/smg/bindings.pytests/integration/test_lists/test-db/l0_a10.ymltests/unittest/grpc/smg/test_smg.pytests/unittest/grpc/test_grpc_optional.py
💤 Files with no reviewable changes (1)
- requirements.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unittest/grpc/smg/test_smg.py
- tensorrt_llm/commands/serve.py
| try: | ||
| from smg_grpc_proto.generated import trtllm_service_pb2, trtllm_service_pb2_grpc | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "The SMG gRPC adapter requires the optional 'smg-grpc-proto' package, " | ||
| "which is not part of the default TensorRT-LLM installation. Install it " | ||
| "with: pip install tensorrt_llm[grpc-smg]" | ||
| ) from e |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
python -m pip download --no-deps --only-binary=:all: \
--dest "$tmpdir" 'smg-grpc-proto==0.4.2'
python - "$tmpdir" <<'PY'
from pathlib import Path
import sys
import zipfile
wheel = next(Path(sys.argv[1]).glob("*.whl"))
names = set(zipfile.ZipFile(wheel).namelist())
for name in (
"smg_grpc_proto/trtllm_service_pb2.py",
"smg_grpc_proto/trtllm_service_pb2_grpc.py",
"smg_grpc_proto/generated/trtllm_service_pb2.py",
"smg_grpc_proto/generated/trtllm_service_pb2_grpc.py",
):
print(f"{name}: {name in names}")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 194
🏁 Script executed:
set -euo pipefail
printf '%s\n' '--- binding file ---'
sed -n '1,80p' tensorrt_llm/grpc/smg/bindings.py
printf '%s\n' '--- SMG dependency declarations and references ---'
rg -n -C 3 'smg-grpc-proto|smg_grpc_proto|grpc-smg' --glob '!build/**' --glob '!dist/**' .Repository: NVIDIA/TensorRT-LLM
Length of output: 10306
🌐 Web query:
smg-grpc-proto 0.4.2 trtllm_service_pb2 package root generated import
💡 Result:
In the smg-grpc-proto package (which provides generated gRPC stubs for projects like TensorRT-LLM and SGLang) [1][2][3], the generated protobuf modules such as trtllm_service_pb2 are located within the smg_grpc_proto.generated subpackage [4][3]. When using the package, these modules are accessed via imports from the generated namespace [4][3]. For example, in TensorRT-LLM, the integration is handled as follows: from smg_grpc_proto.generated import trtllm_service_pb2, trtllm_service_pb2_grpc [4] The smg-grpc-proto package was introduced to centralize gRPC protobuf definitions, replacing the previous practice where downstream projects like TensorRT-LLM and SGLang maintained local.proto files and custom build-time compilation scripts [2][3]. Consequently, you should not expect to find these generated files in the root package directory or generated via local project build hooks; they are managed as part of the external smg-grpc-proto package installation [5][3]. Note that recent versions of smg-grpc-proto have implemented lazy-loading mechanisms in the package's init.py to optimize imports and avoid loading all backend stubs simultaneously [6].
Citations:
- 1: https://pypi.org/project/smg-grpc-proto/
- 2: refactor: replace local proto compilation with smg-grpc-proto package sgl-project/sglang#18682
- 3: [#11292][feat] use smg-grpc-proto package for gRPC proto definitions #11578
- 4: https://github.com/NVIDIA/TensorRT-LLM/blob/7a8bd87f/tensorrt_llm/grpc/__init__.py
- 5: https://github.com/lightseekorg/smg/blob/b1120b13/crates/grpc_client/python/pyproject.toml
- 6: fix(grpc-proto): lazy-load generated re-exports smg-project/smg#1769
🏁 Script executed:
set -euo pipefail
curl -fsSL https://pypi.org/pypi/smg-grpc-proto/0.4.2/json |
python3 -c 'import json,sys; d=json.load(sys.stdin); print(d["info"]["requires_python"]); print(d["urls"][0]["url"])'Repository: NVIDIA/TensorRT-LLM
Length of output: 309
🌐 Web query:
site:pypi.org/project/smg-grpc-proto/0.4.2 "from smg_grpc_proto"
💡 Result:
For smg-grpc-proto==0.4.2, the documented import is:
from smg_grpc_proto import (
sglang_scheduler_pb2,
sglang_scheduler_pb2_grpc,
)The package was released on March 4, 2026. (pypi.org)
Citations:
🏁 Script executed:
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
url='https://files.pythonhosted.org/packages/0b/a2/5417b23d0a46cfd2531e20ead5efb73e6800433c5ae82afcdeea5b355be9/smg_grpc_proto-0.4.2-py3-none-any.whl'
curl -fsSL "$url" -o "$tmpdir/pkg.whl"
unzip -Z1 "$tmpdir/pkg.whl" | grep -E '(^|/)(trtllm_service_pb2(_grpc)?\.py|__init__\.py)$' | sort
printf '%s\n' '--- root initializer ---'
unzip -p "$tmpdir/pkg.whl" smg_grpc_proto/__init__.py | sed -n '1,160p'
printf '%s\n' '--- generated initializer ---'
unzip -p "$tmpdir/pkg.whl" smg_grpc_proto/generated/__init__.py 2>/dev/null | sed -n '1,120p' || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 1401
🏁 Script executed:
set -euo pipefail
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
url='https://files.pythonhosted.org/packages/0b/a2/5417b23d0a46cfd2531e20ead5efb73e6800433c5ae82afcdeea5b355be9/smg_grpc_proto-0.4.2-py3-none-any.whl'
curl -fsSL "$url" -o "$tmpdir/pkg.whl"
for file in \
smg_grpc_proto/generated/trtllm_service_pb2.py \
smg_grpc_proto/generated/trtllm_service_pb2_grpc.py
do
printf '%s\n' "--- $file imports ---"
unzip -p "$tmpdir/pkg.whl" "$file" | sed -n '1,70p' | grep -E '^(from|import) '
doneRepository: NVIDIA/TensorRT-LLM
Length of output: 756
Narrow the optional-dependency guard.
Keep the smg_grpc_proto.generated import path. Catch ModuleNotFoundError only when e.name == "smg_grpc_proto"; re-raise other import failures so missing google.protobuf or grpc dependencies are not reported as a missing optional package.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/grpc/smg/bindings.py` around lines 18 - 25, Update the import
guard around trtllm_service_pb2 and trtllm_service_pb2_grpc to catch
ModuleNotFoundError only when e.name equals "smg_grpc_proto"; preserve the
existing optional-package error for that case, and re-raise all other import
failures unchanged.
Source: Coding guidelines
|
PR_Github #63659 [ run ] triggered by Bot. Commit: |
|
Looked at the new commit The opt-in change itself is safe for released users, but it is a default-install contract change on main. I checked: Both new Also a process note rather than a code one: the packaging change is a distinct concern from "organize the adapter by protocol". It's small enough that I wouldn't insist on splitting it, but the title no longer describes the PR. Nothing here is blocking on my side beyond the open thread on |
|
PR_Github #63659 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63696 [ run ] triggered by Bot. Commit: |
|
PR_Github #63696 [ run ] completed with state
|
3 participants
Dev Engineer Review
tensorrt_llm.grpc.smg.serve.pynow delegates gRPC startup tolaunch_smg_server.smg-grpc-protois optional and is available through thegrpc-smgextra.QA Engineer Review
test_smg_bindings_missing_gives_actionable_error.test_smg_bindings_present_smoke.tests/unittest/grpc/smg/test_smg.pyimports and optional-dependency handling.unittest/grpc/test_grpc_optional.pyandunittest/grpc/smg/test_smg.pytotests/integration/test_lists/test-db/l0_a10.yml.Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.