[#17016][feat] Add selectable OpenEngine gRPC server - #17084
Open
connorcarpenter15 wants to merge 4 commits into
Open
[#17016][feat] Add selectable OpenEngine gRPC server#17084connorcarpenter15 wants to merge 4 commits into
connorcarpenter15 wants to merge 4 commits into
Conversation
connorcarpenter15
marked this pull request as ready for review
July 30, 2026 22:18
connorcarpenter15
requested review from
QiJune,
allisonlim-nv,
brnguyen2,
dpitman-nvda and
yiqingy0
July 30, 2026 22:18
Contributor
|
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:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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}
Contributor
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
tburt-nv
reviewed
Jul 31, 2026
Comment on lines
+2
to
+4
| openengine-openengine-grpc-python==1.67.1.2.20260730172104+768a93c7b44e | ||
| openengine-openengine-protocolbuffers-python==31.1.0.2.20260730172104+768a93c7b44e | ||
| openengine-openengine-protocolbuffers-pyi==31.1.0.2.20260730172104+768a93c7b44e |
Collaborator
There was a problem hiding this comment.
The provenance of these packages is not documented well in the package metadata, so I'll need to do some digging before we can approve this.
Author
There was a problem hiding this comment.
Hi @tburt-nv, for some additional context:
OpenEngine is a gRPC proto being developed by the Dynamo team for interactions between Dynamo and inference engines. The schema source lives in ai-dynamo/openengine, and releases are published to Buf Build openengine/openengine. The packages above are automatically generated by Buf (which is why the names are a bit ugly and documentation is sparse) and published for use.
- Schema source:
ai-dynamo/openenginereleasev0.1.0, pointing to Git commitb5f2bd93721f7b888d3e2440679e0ae7012939d1. The repository is Apache-2.0 licensed. - Buf source:
buf.build/openengine/openengineat immutable BSR commit768a93c7b44e40f28c692ad0b471a8f2.
Contributor
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
1 task
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
connorcarpenter15
force-pushed
the
feat/openengine-server-stub
branch
from
August 3, 2026 23:02
a5e629b to
0a455c3
Compare
connorcarpenter15
changed the title
Contributor
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
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
Signed-off-by: Connor Carpenter <connorc@nvidia.com>
StanleySun639
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dev Engineer Review
smgandopenenginegRPC protocols.--grpcusage.OpenEngineServer.tensorrt_llm.grpcprotocol-neutral.openengineoptional dependency extra and Docker packaging support.QA Engineer Review
tests/unittest/api_stability/references/trtllm_serve_cli.yaml.--openengine-hostand--openengine-port.Description
Implement the gRPC protocol structure and first OpenEngine integration proposed in #17016.
tensorrt_llm/grpc/smg/and keeptensorrt_llm.grpcprotocol-neutral.--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.