[#17016][feat] Add selectable OpenEngine gRPC server - #17084
Conversation
|
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:
WalkthroughChangesOpenEngine dependencies and packaging
SMG gRPC server lifecycle
OpenEngine gRPC stub lifecycle
Serving lifecycle and CLI wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/commands/serve.py (1)
517-534: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete annotations for the modified callables.
tensorrt_llm/commands/serve.py#L517-L534: annotatelaunch_serverwith-> None.tensorrt_llm/commands/serve.py#L640-L655: annotateserve_http_and_openenginewith-> None.tensorrt_llm/commands/serve.py#L1317-L1341: annotateservewith-> None.tests/unittest/openengine/test_server.py#L38-L40: annotate the parametrized inputs using precise stub/request factory types.As per coding guidelines, “Annotate every function.”
🤖 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/commands/serve.py` around lines 517 - 534, Complete the callable annotations: add -> None to launch_server, serve_http_and_openengine, and serve in tensorrt_llm/commands/serve.py. In tests/unittest/openengine/test_server.py lines 38-40, annotate the parametrized inputs with the precise stub and request-factory types used by the test.Source: Coding guidelines
🤖 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 `@requirements-openengine.txt`:
- Line 1: Add the required NVIDIA SPDX copyright and license header at the
beginning of requirements-openengine.txt, placing it before the existing
--extra-index-url directive and preserving the pip configuration unchanged.
In `@tensorrt_llm/openengine/server.py`:
- Line 32: Update the address construction in OpenEngineServer to bracket IPv6
literal hosts before appending the port, producing targets such as [::1]:50051
while preserving existing formatting for IPv4 addresses and hostnames.
In `@tensorrt_llm/serve/openai_server.py`:
- Line 270: Document the new lifecycle parameters: in
tensorrt_llm/serve/openai_server.py at lines 270-270, update the public function
docstring to explain that shutdown_generator=False delegates LLM shutdown to the
sibling-server owner; in tensorrt_llm/openengine/server.py at lines 43-46, add a
Google-style Args entry describing grace and its effect on gRPC termination.
---
Outside diff comments:
In `@tensorrt_llm/commands/serve.py`:
- Around line 517-534: Complete the callable annotations: add -> None to
launch_server, serve_http_and_openengine, and serve in
tensorrt_llm/commands/serve.py. In tests/unittest/openengine/test_server.py
lines 38-40, annotate the parametrized inputs with the precise stub and
request-factory types used by the test.
🪄 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: 7b583cd8-ee03-4596-b2b2-2906e5d7728f
📒 Files selected for processing (10)
docker/Dockerfile.multirequirements-openengine.txtsetup.pytensorrt_llm/commands/serve.pytensorrt_llm/openengine/README.mdtensorrt_llm/openengine/__init__.pytensorrt_llm/openengine/server.pytensorrt_llm/serve/openai_server.pytests/unittest/api_stability/references/trtllm_serve_cli.yamltests/unittest/openengine/test_server.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unittest/grpc/openengine/test_server.py (1)
40-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover the startup-failure path before merging.
await server.start()runs beforetry, so a startup exception bypasses thefinallyblock andserver.stop(grace=0). Moveawait server.start()insidetry. Add a regression case that forces startup to fail after the constructor binds the port.grpc.aio.Server.stop()is documented as callable at any time. (grpc.github.io)Test coverage summary
- Added:
test_all_openengine_rpcs_are_unimplemented,test_format_bind_address.- Removed: none.
- CI list:
tests/integration/test_lists/test-db/l0_cpu_x86.ymlincludesunittest/grpc/openengine/test_server.py.- Manual QA list: not provided, so membership is not verifiable.
- Coverage verdict: needs follow-up. RPC status and address formatting are covered. Startup-failure cleanup and bind-failure behavior are not covered.
As per path instructions, this summary reports changed test functions, test-list membership, and a coverage verdict.
🤖 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 `@tests/unittest/grpc/openengine/test_server.py` around lines 40 - 59, Move await server.start() inside the existing try/finally in test_all_openengine_rpcs_are_unimplemented so server.stop(grace=0) always runs, including startup failures. Add a regression test that forces startup to fail after OpenEngineServer construction has bound the port, and verify cleanup remains safe when stop is called after the failed start.Source: Path instructions
🤖 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/openengine/lifecycle.py`:
- Around line 29-34: Update the lifecycle startup flow to initialize
openengine_server as None, then call stop(grace=0) after a constructed
OpenEngineServer fails during start() and before on_startup_failure() re-raises
the exception. Guard the cleanup so stop is called only when construction
succeeded, including the existing cancellation and exception paths.
In `@tensorrt_llm/grpc/openengine/README.md`:
- Around line 50-51: Update the documentation sentence near the Buf link to
hyphenate the compound modifier, changing “Python generated SDK guide” to
“Python-generated SDK guide” without altering the link or surrounding wording.
---
Nitpick comments:
In `@tests/unittest/grpc/openengine/test_server.py`:
- Around line 40-59: Move await server.start() inside the existing try/finally
in test_all_openengine_rpcs_are_unimplemented so server.stop(grace=0) always
runs, including startup failures. Add a regression test that forces startup to
fail after OpenEngineServer construction has bound the port, and verify cleanup
remains safe when stop is called after the failed start.
🪄 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: 7f7b6218-7b8c-4897-9ab1-199a6995afbc
📒 Files selected for processing (10)
.github/CODEOWNERSjenkins/L0_Test.groovyrequirements-openengine.txttensorrt_llm/commands/serve.pytensorrt_llm/grpc/openengine/README.mdtensorrt_llm/grpc/openengine/__init__.pytensorrt_llm/grpc/openengine/lifecycle.pytensorrt_llm/grpc/openengine/server.pytests/integration/test_lists/test-db/l0_cpu_x86.ymltests/unittest/grpc/openengine/test_server.py
🚧 Files skipped from review as they are similar to previous changes (2)
- requirements-openengine.txt
- tensorrt_llm/commands/serve.py
a5e629b to
0a455c3
Compare
connorcarpenter15
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/commands/serve.py (1)
1321-1346: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject multiple frontends in gRPC mode.
num_serve_frontendsis not inunsupported_args. Neithertensorrt_llm.grpc.smg.server.launch_servernortensorrt_llm.grpc.openengine.server.launch_servercreates attached frontend processes. Rejectnum_serve_frontends != 1here, or implement the required gRPC multi-frontend lifecycle.🤖 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/commands/serve.py` around lines 1321 - 1346, The gRPC validation in the serve command does not reject unsupported multiple frontends. Update the grpc branch near unsupported_args to validate num_serve_frontends and raise a ValueError unless it equals 1, before launching either gRPC server path; preserve the existing unsupported-argument checks.
🧹 Nitpick comments (1)
tensorrt_llm/grpc/openengine/server.py (1)
67-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the public parameters.
launch_serveris exported, but its docstring does not documenthostorport. Add anArgs:section.As per coding guidelines, “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 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/openengine/server.py` around lines 67 - 68, Update the public launch_server docstring to include a Google-style Args section documenting the host and port parameters, including their expected values or purpose.Source: Coding guidelines
🤖 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/__init__.py`:
- Around line 84-95: Sort the __all__ entries in the smg package according to
Ruff’s configured ordering, including PROTOS_AVAILABLE, GrpcRequestManager, and
the remaining exported symbols, without changing which names are exported.
In `@tensorrt_llm/grpc/smg/server.py`:
- Around line 97-100: Update the server startup flow around add_insecure_port
and the startup logger to capture its returned bound port, then log the resolved
host and port; this must report the actual dynamically assigned port when the
configured port is zero while preserving the configured-port behavior otherwise.
---
Outside diff comments:
In `@tensorrt_llm/commands/serve.py`:
- Around line 1321-1346: The gRPC validation in the serve command does not
reject unsupported multiple frontends. Update the grpc branch near
unsupported_args to validate num_serve_frontends and raise a ValueError unless
it equals 1, before launching either gRPC server path; preserve the existing
unsupported-argument checks.
---
Nitpick comments:
In `@tensorrt_llm/grpc/openengine/server.py`:
- Around line 67-68: Update the public launch_server docstring to include a
Google-style Args section documenting the host and port parameters, including
their expected values or purpose.
🪄 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: f329d5d5-77b8-4132-89ac-f05e8cee07a4
📒 Files selected for processing (15)
.github/CODEOWNERSdocker/Dockerfile.multijenkins/L0_Test.groovyrequirements-openengine.txtsetup.pytensorrt_llm/commands/serve.pytensorrt_llm/grpc/__init__.pytensorrt_llm/grpc/openengine/README.mdtensorrt_llm/grpc/openengine/__init__.pytensorrt_llm/grpc/openengine/server.pytensorrt_llm/grpc/smg/__init__.pytensorrt_llm/grpc/smg/request_manager.pytensorrt_llm/grpc/smg/server.pytensorrt_llm/grpc/smg/servicer.pytests/integration/test_lists/test-db/l0_cpu_x86.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- jenkins/L0_Test.groovy
- tests/integration/test_lists/test-db/l0_cpu_x86.yml
- .github/CODEOWNERS
- setup.py
- requirements-openengine.txt
- docker/Dockerfile.multi
- tensorrt_llm/grpc/openengine/README.md
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The SMG extraction reads cleanly and the default-preserving --grpc-protocol smg is the right call. Three things before merge:
The OpenEngine path silently ignores the model and all LLM args. launch_server(host, port) is called with no llm_args, so trtllm-serve <model> --grpc --grpc-protocol openengine parses a full LLM config, builds llm_args, then throws it away and starts a socket that answers UNIMPLEMENTED. That's defensible for a stub, but it should be loud: log a WARNING (not INFO) at startup stating no engine is loaded and all RPCs return UNIMPLEMENTED, and ideally reject or warn on the LLM-affecting options the user passed. Right now the failure mode for someone trying it is "server started, everything 501s."
No automated test for the new path. The description reports a manual check that all 11 RPCs return UNIMPLEMENTED — that check is worth ~30 lines as a real test (start OpenEngineServer on port 0, one unary + one streaming call, assert StatusCode.UNIMPLEMENTED), guarded by pytest.importorskip("openengine"). Without it nothing catches a BSR package rename or a servicer registration typo.
Description/diff mismatch. The description says "Updates CLI stability metadata for OpenEngine host and port options" and mentions --openengine-host / --openengine-port, but neither flag exists in the diff and the YAML reference only adds grpc_protocol. Please reconcile.
Also worth confirming: does tests/unittest/grpc/smg/test_server.py still get collected? Check for references to the old tests/unittest/llmapi/test_grpc.py path in tests/integration/test_lists/ and any CI stage definitions — a moved test that nothing runs looks green forever.
|
/bot run |
|
PR_Github #68914 [ run ] triggered by Bot. Commit: |
|
PR_Github #68914 [ run ] completed with state
|
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #69141 [ run ] triggered by Bot. Commit: |
|
PR_Github #69141 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #69470 [ run ] triggered by Bot. Commit: |
|
PR_Github #69470 [ run ] completed with state
|
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
This reverts commit 6b8daf2. Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Yanchao Lu <yanchaol@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #69586 [ run ] triggered by Bot. Commit: |
|
PR_Github #69586 [ run ] completed with state
|
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/bot run --stage-list "Build-Docker-Images" |
|
PR_Github #69749 [ run ] triggered by Bot. Commit: |
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
|
/bot run --disable-fail-fast |
|
PR_Github #69781 [ run ] triggered by Bot. Commit: |
|
PR_Github #69749 [ run ] completed with state |
|
PR_Github #69781 [ run ] completed with state
|
|
/bot run |
|
PR_Github #69919 [ run ] triggered by Bot. Commit: |
|
PR_Github #69919 [ run ] completed with state |
11 participants
Description
Implement the first OpenEngine integration proposed in #17016 on top of the gRPC protocol structure merged in #17179.
tensorrt_llm/grpc/smg/merged in [None][refactor] Organize SMG gRPC adapter by protocol #17179; this PR adds no SMG implementation changes.--grpc-protocol {smg,openengine}selection totrtllm-serve. Existing--grpcinvocations continue to select SMG by default.tensorrt_llm.grpc.openengine.OpenEngineServeras a dedicated gRPC server selected with--grpc --grpc-protocol openengine.InferenceServicerandControlServicerbases directly, so all 11 v0.1.0 RPCs intentionally returnUNIMPLEMENTEDand no request reaches the TensorRT-LLM engine.openenginepip extra. The default install gains no new dependency.--grpc-protocol openenginewithout--grpcand reject gRPC mode for VisualGen. The normal OpenAI HTTP path is unchanged.The OpenEngine bindings come from the public OpenEngine repository release
v0.1.0and the Buf Schema Registry module at immutable commit768a93c7b44e40f28c692ad0b471a8f2. Package versions, SHA-256 provenance, installation instructions, and the adapter maintenance boundary are documented intensorrt_llm/grpc/openengine/README.md.This PR deliberately does not copy proto files or add request conversion, engine hooks, LoRA behavior, KV integration, or request tracking.
Maintainer action: please apply the
api-compatiblelabel for the prototype CLI metadata change. GitHub does not grant external contributors permission to add repository labels.Test Coverage
grpc.StatusCode.UNIMPLEMENTED.git diff --checkfor the changed implementation.--grpc-protocol.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.