[None][refactor] Organize SMG gRPC adapter by protocol - #17179
Conversation
|
/bot run |
|
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:
WalkthroughThe gRPC serve command now uses the SMG gRPC server launcher. The change adds optional SMG bindings, backend initialization, graceful shutdown, dependency validation, adapter import updates, and focused tests. ChangesSMG gRPC integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCommand
participant launch_smg_server
participant LLMEngine
participant RequestManager
participant SMGServicer
participant grpcServer
ServeCommand->>launch_smg_server: host, port, LLM arguments, served model name
launch_smg_server->>LLMEngine: initialize 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: termination signal
launch_smg_server->>grpcServer: stop with grace period
launch_smg_server->>LLMEngine: shut down engine
Suggested reviewers: 🚥 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: |
|
The server move appears behavior-preserving: message limits, keepalive settings, signal handling, and shutdown ordering are unchanged. Could you address three follow-ups before merge?
I’d wait for a clean pipeline before approval. |
|
PR_Github #63353 [ run ] completed with state
|
|
/bot run |
|
PR_Github #63365 [ run ] triggered by Bot. Commit: |
|
PR_Github #63365 [ run ] completed with state
|
|
The current red runs look unrelated: build 51350 reports 27,211 passing tests and only stage-level failures. Several PRs on the same base also fail Check Test List on synthetic cache-transceiver precheck names in Could you rerun once the base validator issue is fixed? My remaining code concerns are CI registration for |
|
/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
|
PR_Github #63659 [ run ] triggered by Bot. Commit: |
|
The title could also mention optional packaging, since this is more than a directory refactor. I’d hold until the import boundary is narrowed and CI completes. |
|
/bot run |
|
PR_Github #64556 [ run ] triggered by Bot. Commit: |
|
PR_Github #64556 [ run ] completed with state
|
|
/bot run |
|
PR_Github #64840 [ run ] triggered by Bot. Commit: |
|
PR_Github #64840 [ run ] completed with state
|
Move the smg-grpc-proto pin from requirements-dev.txt into setup.py as the single source of truth. Add a user-facing grpc-smg extra for SMG gateway deployments; the devel extra aggregates it so developers and CI still get the adapter. Update install hints and tests accordingly. Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
The CI test environment is provisioned at image granularity: the release image installs the wheel, and test stages only add requirements-dev.txt, which no longer carries smg-grpc-proto. Bake the grpc-smg extra into the wheel install (alongside mx) so unittest/grpc/smg keeps running in CI instead of being silently skipped by importorskip once images rebuild. Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
|
/bot run --stage-list "A10-PyTorch-1" |
|
/bot run --stage-list "A10-PyTorch-1" |
|
PR_Github #65008 [ run ] triggered by Bot. Commit: |
|
PR_Github #65008 [ run ] completed with state
|
…nts file Give each gRPC gateway a dedicated requirements-<gateway>.txt as the single source of truth for its dependency pins, starting with requirements-grpc-smg.txt; setup.py parses it into the grpc-smg extra, and the file is owned by OSS compliance like the other requirements files. A requirements file may carry gateway-specific options such as an --extra-index-url without touching the default dependency graph, which a setup.py literal cannot express, so the OpenEngine adapter can declare its BSR-resolved pins the same way. CI test environments install exactly zero or one gateway file per stage, routed by whether the gateway's pins co-resolve with the default environment. SMG co-resolves, so the shared K8s and SLURM provisioning paths install its file next to requirements-dev.txt; this restores the package that unittest/grpc/smg needs after its move out of requirements.txt (without it the module is skipped at collection and pytest exit code 5 fails the stage, pipeline 52819). A gateway whose pins conflict with the default environment installs its file behind a dedicated stage guard instead, as documented at the provisioning site. Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
|
/bot run --stage-list "A10-PyTorch-1" |
|
PR_Github #65041 [ run ] triggered by Bot. Commit: |
|
PR_Github #65041 [ run ] completed with state |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The reorganization is clean: no stale references to the old module or test paths remain (checked test-db, waives, and imports), the optional-dependency guards correctly match on ModuleNotFoundError.name, and the pin-file plumbing is consistent across setup.py, Docker, Jenkins, and slurm.
Two notes on the PR description:
- It's pasted CodeRabbit output, and several of its action items are stale against the actual diff — e.g. it claims the CLI text says
tensorrt_llm[devel], but the diff consistently usestensorrt_llm[grpc-smg], and the test-list registration it asks to confirm is already in l0_a10.yml. Please replace it with a short description of what the PR actually does. - The PR is titled
[refactor]but bundles two behavior changes worth naming: thevocab_sizeread-through to the inner HF tokenizer in[request_manager.py:219](https://github.com/NVIDIA/TensorRT-LLM/pull/17179/files#diff-44ca1c6095111b3cd2ce91762e77269ceb1bf665339a241cfc7065350a84c285R219)(the wrapper's property raisesNotImplementedError, soGetModelInfopreviously failed to report vocab size), and the module→class fixture scoping intest_smg.pyto keep the two test LLMs from coexisting on an A10. Both look correct; they just shouldn't be invisible in a refactor PR.
There's also a functional gap carried over from the old code, detailed inline: add_insecure_port failures are still silent, and the new bind-failure test mocks a failure mode gRPC doesn't actually have. Not a regression from this PR, so it's fine to handle in a follow-up rather than here.
| logger.info("gRPC reflection enabled") | ||
|
|
||
| address = f"{host}:{port}" | ||
| server.add_insecure_port(address) |
There was a problem hiding this comment.
Non-blocking, and pre-existing rather than introduced here — fine as a follow-up PR.
add_insecure_port never raises on failure — it returns the bound port, or 0 when binding fails (port already in use, bad address). The return value is ignored here, so on a real bind failure the server "starts" cleanly, logs Server is ready to accept requests, and blocks on stop_event while listening on nothing. Since this PR already adds the failure-cleanup path, checking the return is a natural next step:
if server.add_insecure_port(address) == 0:
raise RuntimeError(f"Failed to bind gRPC server to {address}")The finally block then handles teardown correctly.
| grpc_server.stop = AsyncMock() | ||
|
|
||
| if failure_point == "bind": | ||
| grpc_server.add_insecure_port.side_effect = RuntimeError("bind failed") |
There was a problem hiding this comment.
Follow-up along with the server.py bind check — no need to block this PR.
This simulates a bind failure with a raising mock, but real grpc.aio add_insecure_port doesn't raise — it returns 0 on failure. As written, the "bind" case validates cleanup for a failure mode that can't occur, while the actual bind-failure behavior (silent unbound server) goes untested. Once server.py checks the return value, change this case to grpc_server.add_insecure_port.return_value = 0 and assert on the resulting error, so the test exercises the real contract.
| # PreTrainedTokenizerBase's abstract property (raises | ||
| # NotImplementedError). Read from the inner tokenizer. | ||
| tokenizer = self.llm.tokenizer | ||
| inner_tokenizer = getattr(tokenizer, "tokenizer", tokenizer) |
There was a problem hiding this comment.
This is a behavior fix (GetModelInfo previously hit PreTrainedTokenizerBase's NotImplementedError through the wrapper and reported no vocab_size), not part of the package reorganization — worth a line in the PR description so it's findable later. Also, none of the tests in test_smg.py cover get_model_config() with a TransformersTokenizer-wrapped LLM; a small unit test asserting vocab_size comes from the inner tokenizer would keep this from regressing.
|
/bot run |
|
PR_Github #65249 [ run ] triggered by Bot. Commit: |
…image Install the release image with [mx] only; gateway users add pip install "tensorrt_llm[grpc-smg]" on top, same as wheel users. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: junq <22017000+QiJune@users.noreply.github.com>
|
/bot run |
|
@CatherineSue Heads-up on a packaging change to the SMG gRPC adapter, since you've been the main contributor on the SMG side. What changes. This PR moves pip install "tensorrt_llm[grpc-smg]"This applies to the NGC release container as well — the image no longer preinstalls the SMG dependency, so existing containerized Why. A second community gateway adapter (OpenEngine, #17084, from the Dynamo team) is being contributed alongside SMG, and the two bindings pin protobuf incompatibly (5.x vs 6.x). The principle we've aligned on is that gateway adapters are peer, community-maintained, opt-in extras: no gateway sits in the default install or the release image, and each owns a What stays the same. Runtime behavior of the adapter is unchanged — this PR is a relocation plus the packaging split. The SMG unit tests remain in pre-merge CI ( If SMG-side docs reference the old install assumption ("gRPC works out of the box with |
|
PR_Github #65257 [ run ] triggered by Bot. Commit: |
|
PR_Github #65249 [ run ] completed with state |
|
PR_Github #65257 [ run ] completed with state
|
6 participants
Dev Engineer Review
tensorrt_llm.grpc.smg.bindings.py,server.py, and package metadata.bindingspath.serve.pyto launchlaunch_smg_server.protobuf>=5.27.2to runtime requirements.smg-grpc-proto>=0.4.2to development requirements.tensorrt_llm[devel]. The required package name istensorrt_llm[grpc-smg]. Align the documentation and error text.trtllm-serve --grpcrequirespip install tensorrt_llm[grpc-smg].ImportErrorhandling only catches missingsmg_grpc_protoand does not mask unrelated import failures.tests/unittest/grpc/smg/test_smg.pyis registered in the required CI test list.QA Engineer Review
test_smg_bindings_missing_gives_actionable_errortest_smg_bindings_preserves_unrelated_import_errortest_smg_bindings_present_smoketest_smg_server_startup_failure_cleans_uptests/unittest/grpc/smg/test_smg.pyfor the new package path and optional dependency behavior.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.