[None][feat] serve: multi-process HTTP frontends on the classic IPC executor path#16523
Open
lancelly wants to merge 18 commits into
Open
[None][feat] serve: multi-process HTTP frontends on the classic IPC executor path#16523lancelly wants to merge 18 commits into
lancelly wants to merge 18 commits into
Conversation
…xecutor path At high concurrency a trtllm-serve worker is host-bound on its single serving process: one asyncio event loop (one GIL) performs every request json.loads+pydantic validate and every SSE chunk write, and queuing on that loop dominates first-token latency while the GPUs idle (measured on DSv4 disagg GEN: gen_preprocessing p50 54ms@c512 -> 1520ms@c2048). vLLM/SGLang address the same limit with multiple API-server processes. TLLM_SERVE_NUM_FRONTENDS=K (env-gated, default off) runs K HTTP frontend processes against ONE executor on the default (classic IPC) orchestrator: - Launcher/attach split: frontend 0 builds the LLM and launches workers as usual, then spawns K-1 children that re-exec the command line with TLLM_EXECUTOR_ATTACH_INFO set; their executor attaches via the new GenerationExecutorFrontendProxy (no MPI session, no worker launch, and shutdown never emits the worker's None engine-shutdown sentinel -- the launcher alone owns the engine lifecycle). - Deterministic ipc endpoints, pre-generated by the launcher (TLLM_MULTI_FRONTEND_IPC_DIR/_HMAC): the rank0 worker BINDS the request ingress (PULL) so every frontend PUSH-connects; each frontend binds its own result lane (PULL) that the worker (non-postproc) or every postprocess worker (one PUSH pipe per frontend) sends to. - client-id namespacing: the top 16 bits of the uint64 client id carry the frontend id; responses are routed by client_id>>48. Responses without a usable client id (e.g. attention-DP dummy requests carry client_id=None) route to the launcher, preserving today's silent discard semantics. Frontend 0 keeps ids bit-identical to today. - All frontends bind the public port with SO_REUSEPORT, so the kernel load-balances accepted connections across their independent processes (and GILs); clients and the disagg orchestrator still see one URL. Known limitations (documented): /metrics and /perf_metrics are served per-frontend (SO_REUSEPORT samples one frontend per request); enable_resource_governor is rejected in multi-frontend mode. Validation: 12 CPU-only unit tests (id namespacing, response-lane bucketing incl. the ADP-dummy None guard, attached-proxy submit/cancel/ never-sends-sentinel over real ipc sockets). E2E A/B on DSv4-Pro disagg (11xCTX-DEP4 + 1xGEN-DEP16, c3120, back-to-back on identical nodes): gen_preprocessing p50 1075ms -> 37.6ms (-96.5%), p95 3145ms -> 75.7ms; per-request SSE speed p50 +30%; 16 frontends, 75 min, zero tracebacks; engine-side phases (gen_queue, kv_transfer) byte-identical to baseline. Ported to main from the feat/deepseek_v4-based branch (PR NVIDIA#16413), with review cleanups folded in: attached-frontend child env now uses split_mpi_env() (strips SLURM_/UCX_/PMI_ etc., not just 3 prefixes); TLLM_SERVE_NUM_FRONTENDS parsing/validation unified in get_num_serve_frontends(); result-lane selection unified in frontend_lane_index(); stale rpc_common docstring reference fixed. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…e env gate Review follow-ups on the multi-frontend prototype: - The attach-info JSON (mkstemp, carries the executor HMAC keys in hex) and the shared ipc directory (mkdtemp, holds the request/result .sock files) were never removed: every multi-frontend run leaked a secret-bearing file plus a directory in /tmp. launch_server's finally now unlinks the attach file and rmtree's the ipc dir after the attached frontends have been terminated (established zmq connections are not disturbed by unlinking ipc paths; the engine can still drain its lanes at teardown). - launch_server read TLLM_SERVE_NUM_FRONTENDS from the global env unconditionally, but disaggregated ctx/gen MPI workers reach launch_server too (_launch_disaggregated_server) and inherit the submitter's env under Slurm export-all: an exported knob would mis-switch them into multi-frontend mode. launch_server gains multi_frontend_enabled (default True); the disagg worker path passes False and an ignored knob logs a warning. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Extract the env-gate block at the top of launch_server into _init_multi_frontend_mode(), returning a MultiFrontendMode NamedTuple whose .active / .is_launcher properties replace the repeated 'multi_frontend and not is_attached_frontend' expressions at the SO_REUSEPORT, spawn, and cleanup sites. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…sponse routing hot path Every response type that reaches _send_rsp and bucket_responses_by_frontend (tllm.Response, ErrorResponse, ResponseWrapper via __getattr__ delegation, PostprocWorker.Output) defines client_id, and the None-VALUE case (ADP dummy requests) is already handled by frontend_lane_index. The getattr(..., None) default was never exercised for a missing attribute; worse, it would silently route a hypothetical malformed response to lane 0 (where the launcher discards it, hanging the client) instead of failing loudly. Use direct attribute access: cheaper on the per-response loop and loud on real bugs. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…serve_frontends knob Promote the multi-frontend switch from TLLM_SERVE_NUM_FRONTENDS to a typed config knob, following the vLLM --api-server-count shape: - llm_args gains num_serve_frontends (int, default 1, ge=1 le=65536, status=prototype) next to the other serving knobs; exposed on trtllm-serve as --num_serve_frontends and via the config yaml. api_stability reference updated. - GenerationExecutorProxy now reads the count from llm_args and OWNS the shared ipc directory + HMAC key (generated in __init__, removed in shutdown): the TLLM_MULTI_FRONTEND_IPC_DIR / TLLM_MULTI_FRONTEND_HMAC env side-channel between serve.py and the proxy is deleted, and the ipc-dir cleanup moves from serve.py to the proxy that created it. - serve.py _init_multi_frontend_mode normalizes the knob into llm_args; TLLM_SERVE_NUM_FRONTENDS is kept as a fallback for when the knob is unset (existing deployments), translated to the knob at the CLI entry. The disabled entry points (disagg MPI workers) now also scrub the knob from llm_args so the executor cannot enter multi-frontend mode there. - executor.py validates the TLLM_EXECUTOR_FRONTEND_ID / TLLM_EXECUTOR_ATTACH_INFO pairing with a clear error instead of a raw KeyError. The attach-info env+file bootstrap remains: it crosses the child re-exec boundary, which a config knob cannot. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…op the env fallback 64 frontends already exceeds what one node's cores can usefully serve; the previous 1<<16 bound was the client-id encoding capacity, not a sane policy limit (the 16-bit wire format is unchanged). TLLM_SERVE_NUM_FRONTENDS is no longer honored: the knob is the only switch. A set env now logs a pointer to --num_serve_frontends instead of being silently ignored. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…ghten comments TLLM_SERVE_NUM_FRONTENDS never shipped in a release, so there is nothing to deprecate -- remove the warning shim entirely; the knob is the only switch. Also condense the multi-frontend comments/docstrings: the architecture is described once (MultiFrontendMode / GenerationExecutorFrontendProxy); other sites keep only the local constraint they enforce. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…tattr for the frontend count llm_args can legitimately be None (legacy TensorRT path), but any non-None llm_args has the num_serve_frontends field (BaseLlmArgs), so handle the None case explicitly like the adjacent lines and let a renamed field fail loud. The 'or 1' was dead: pydantic enforces ge=1. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
The 1<<16 upper bound (wire-format capacity) was dead code after the num_serve_frontends<=64 cap: the lane-count check already rejects anything >= len(result_addrs). Fold both into one range check with a message that names the valid ids. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
lancelly
added
the
api-compatible
Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
reasonsolo
reviewed
Jul 17, 2026
…ot a temp file Review feedback (reasonsolo): serialize the attach payload directly into TLLM_EXECUTOR_ATTACH_INFO instead of writing a 0600 temp file and pointing the env at it. This deletes the file lifecycle entirely (no mkstemp, no unlink, no key left on disk after a hard kill), and the child pops the env once consumed so the HMAC keys cannot leak into descendant processes. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Review feedback (reasonsolo): drop the single-use 'active' property (its one call site reads clearer as launcher-or-attached), fold its definition into is_launcher, and shorten the docstrings. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
…ally Review feedback (reasonsolo): no backward compatibility needed for the in-tree call chain -- worker_main always passes a list (a single lane in single-frontend mode), so PostprocWorker/postproc_worker_main drop the Union[tuple, List[tuple]] signatures and the isinstance normalization. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Review feedback (reasonsolo): fold the disabled-path get/warn/pop into one pop-with-default, drop the duplicated knob read, and shorten the docstring and error text. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Review feedback (reasonsolo) on the id encoding: - The launcher (lane 0) now applies the same namespace rule as attached frontends whenever multi-frontend is active, so a long-lived request counter can never bleed into the frontend-id bits and misroute responses (it was an unguarded invariant before; the re-encode is a numeric no-op until the counter wraps). Single-frontend mode keeps raw ids. - The frontend-id field width is now the single source of truth: FRONTEND_ID_BITS = 6 derives MAX_NUM_FRONTENDS (64), the shift and the counter mask. The CLI cap imports the constant; the llm_args bound cannot (import cycle) and is pinned by a unit test instead. - The field sits just below the sign bit (shift 57): bit 63 stays clear, so ids remain positive in signed-int64 contexts. Field order stays frontend-id-high: the launcher's ids keep their numeric values, and a stray un-namespaced id degrades to lane 0 (the launcher) instead of spraying across lanes. Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
reasonsolo
approved these changes
Jul 17, 2026
Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds ChangesMulti-Frontend Serving
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCLI
participant GenerationExecutorProxy
participant worker_main
participant PostprocWorker
participant FrontendProxy
ServeCLI->>GenerationExecutorProxy: configure frontend lanes
GenerationExecutorProxy->>worker_main: provide shared request and result IPC addresses
ServeCLI->>FrontendProxy: spawn with attachment metadata
FrontendProxy->>worker_main: submit namespaced request
worker_main->>PostprocWorker: route response to frontend lane
PostprocWorker-->>FrontendProxy: deliver response
FrontendProxy->>FrontendProxy: cancel own requests on shutdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Lance Liao <108499334+lancelly@users.noreply.github.com>
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #59954 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #59955 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #59954 [ run ] completed with state |
![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: 5
🧹 Nitpick comments (1)
tests/unittest/executor/test_multi_frontend_routing.py (1)
38-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd annotations to every new function and test method.
Include parameter types and
-> Noneon test methods; annotate helpers such as_response,_fake_worker, and_make_proxy_and_fake_workerprecisely.As per coding guidelines, “Annotate every function.”
Also applies to: 79-162, 192-221
🤖 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/executor/test_multi_frontend_routing.py` around lines 38 - 72, Add complete type annotations to every newly introduced function and test method in this file, including the methods shown and the additional ranges noted in the review. Annotate all parameters and return types, use -> None for test methods, and provide precise annotations for helpers such as _response, _fake_worker, and _make_proxy_and_fake_worker.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/executor/base_worker.py`:
- Line 1106: Remove the unnecessary f-string prefix from the logger_debug
message in the await_response helper creation code, leaving the message text and
logging behavior unchanged.
In `@tensorrt_llm/executor/proxy.py`:
- Around line 1024-1079: Initialize the inherited proxy state in
GenerationExecutorFrontendProxy.__init__ after the deliberate
GenerationExecutorProxy.__init__ skip, including _engine_dead and
_worker_process_monitor with the values expected by inherited submit() and
check_health(). Preserve the existing attached-frontend initialization and avoid
invoking the parent constructor’s MPI, worker-launch, or shutdown-hook setup.
In `@tensorrt_llm/llmapi/llm_args.py`:
- Around line 4124-4135: Regenerate the LLM arguments golden manifest after
adding BaseLlmArgs.num_serve_frontends using python3
scripts/generate_llm_args_golden_manifest.py, and commit the updated
llm_args_golden_manifest.json. Obtain and include the required telemetry/privacy
CODEOWNER approval for this new field.
In `@tests/unittest/executor/test_multi_frontend_routing.py`:
- Around line 192-219: Make the affected proxy/IPC tests exception-safe by
wrapping each test body in try/finally, ensuring proxy.shutdown() runs even when
assertions fail. In the finally blocks, close every worker queue created by
_make_proxy_and_fake_worker after shutdown, including the queues used by the
test spanning the referenced second range, so ZeroMQ sockets and dispatcher
threads are cleaned up.
- Around line 66-69: Add a regression assertion to
test_lane_index_clamps_to_launcher covering a negative client ID, such as
frontend_lane_index(-1, 4) returning launcher lane 0. Update frontend_lane_index
so its bound validation requires frontend_id to be at least 0 and less than
num_lanes, preserving valid routing and clamping all invalid IDs to lane 0.
---
Nitpick comments:
In `@tests/unittest/executor/test_multi_frontend_routing.py`:
- Around line 38-72: Add complete type annotations to every newly introduced
function and test method in this file, including the methods shown and the
additional ranges noted in the review. Annotate all parameters and return types,
use -> None for test methods, and provide precise annotations for helpers such
as _response, _fake_worker, and _make_proxy_and_fake_worker.
🪄 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: eccfe12a-176a-4251-a0cf-8bff17f2075a
📒 Files selected for processing (12)
tensorrt_llm/commands/serve.pytensorrt_llm/executor/base_worker.pytensorrt_llm/executor/executor.pytensorrt_llm/executor/postproc_worker.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytensorrt_llm/llmapi/llm.pytensorrt_llm/llmapi/llm_args.pytests/unittest/api_stability/references/llm.yamltests/unittest/executor/test_multi_frontend_routing.pytests/unittest/llmapi/test_executor.py
| or self.worker.postproc_queues is not None | ||
| or self.worker.frontend_result_queues is not None) | ||
| if not has_ipc_queues: | ||
| logger_debug(f"creating await_response helper for Worker\n", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the extraneous f prefix (Ruff F541).
This f-string has no placeholders; since F rules are enabled, this fails linting on the changed line.
🧹 Proposed fix
- logger_debug(f"creating await_response helper for Worker\n",
+ logger_debug("creating await_response helper for Worker\n",
color="yellow")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| logger_debug(f"creating await_response helper for Worker\n", | |
| logger_debug("creating await_response helper for Worker\n", | |
| color="yellow") |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 1106-1106: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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/executor/base_worker.py` at line 1106, Remove the unnecessary
f-string prefix from the logger_debug message in the await_response helper
creation code, leaving the message text and logging behavior unchanged.
Source: Linters/SAST tools
Comment on lines
+1024
to
+1079
| def __init__( | ||
| self, | ||
| attach_info: dict, | ||
| *, | ||
| frontend_id: int, | ||
| postproc_worker_config: Optional[PostprocWorkerConfig] = None, | ||
| is_llm_executor: Optional[bool] = None, | ||
| ) -> None: | ||
| num_lanes = len(attach_info["result_addrs"]) | ||
| if not 0 < frontend_id < num_lanes: | ||
| raise ValueError( | ||
| f"frontend_id {frontend_id} out of range: attached frontends " | ||
| f"use ids 1..{num_lanes - 1} (id 0 is the launcher)") | ||
| postproc_worker_config = postproc_worker_config or PostprocWorkerConfig( | ||
| ) | ||
| # Deliberately skip GenerationExecutorProxy.__init__: it creates an | ||
| # MPI session, launches workers, and registers the pre_shutdown | ||
| # atexit hook that emits the engine shutdown sentinel. | ||
| GenerationExecutor.__init__( | ||
| self, | ||
| num_postprocess_workers=postproc_worker_config. | ||
| num_postprocess_workers, | ||
| postprocess_tokenizer_dir=postproc_worker_config. | ||
| postprocess_tokenizer_dir, | ||
| is_llm_executor=is_llm_executor) | ||
|
|
||
| self._frontend_id = frontend_id | ||
| self._num_frontends = num_lanes | ||
| self._results: Dict[int, GenerationResult] = {} | ||
| self.garbage_collection_gen0_threshold = None | ||
| self.workers_started = False | ||
| self.dispatch_result_thread: Optional[ManagedThread] = None | ||
| # Must be None: OpenAIServer reads the resource_governor_queue | ||
| # property at init; the governor lives with the launcher only. | ||
| self._resource_governor_queue = None | ||
|
|
||
| hmac_key = bytes.fromhex(attach_info["hmac_key"]) | ||
| self.request_queue = IpcQueue( | ||
| (attach_info["request_addr"], hmac_key), | ||
| is_server=False, | ||
| socket_type=zmq.PUSH, | ||
| name=f"frontend_{frontend_id}_request_queue") | ||
| self.result_queue = FusedIpcQueue( | ||
| (attach_info["result_addrs"][frontend_id], hmac_key), | ||
| is_server=True, | ||
| fuse_message=False, | ||
| socket_type=zmq.PULL, | ||
| name=f"frontend_{frontend_id}_result_queue") | ||
|
|
||
| # Stats / KV events / disagg params share the rank0 worker's stats | ||
| # RPC server with the launcher (ROUTER socket, multi-client). | ||
| self.rpc_client: Optional[RPCClient] = None | ||
| if attach_info.get("rpc_addr"): | ||
| self.rpc_client = RPCClient(attach_info["rpc_addr"], | ||
| hmac_key=bytes.fromhex( | ||
| attach_info["rpc_hmac_key"])) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List attributes set in GenerationExecutorProxy.__init__ vs GenerationExecutorFrontendProxy.__init__,
# and where inherited methods read _engine_dead / _worker_process_monitor.
rg -nP 'self\.(_engine_dead|_worker_process_monitor)\b' tensorrt_llm/executor/proxy.py
echo '--- FrontendProxy self.* assignments ---'
ast-grep --pattern 'class GenerationExecutorFrontendProxy($$$) {
$$$
}' --lang python tensorrt_llm/executor/proxy.py || \
sed -n '/class GenerationExecutorFrontendProxy/,/^class /p' tensorrt_llm/executor/proxy.py | rg -nP 'self\.\w+\s*='Repository: NVIDIA/TensorRT-LLM
Length of output: 1518
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=tensorrt_llm/executor/proxy.py
echo '--- class definitions / inheritance ---'
rg -n '^class (GenerationExecutor|GenerationExecutorProxy|GenerationExecutorFrontendProxy)\b' "$file"
echo
echo '--- relevant method blocks ---'
sed -n '140,360p' "$file"
echo
echo '--- frontend proxy block ---'
sed -n '1000,1105p' "$file"Repository: NVIDIA/TensorRT-LLM
Length of output: 14659
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=tensorrt_llm/executor/proxy.py
echo '--- submit/check_health definitions ---'
rg -n '^( def (submit|check_health|_check_mpi_workers)\b|class )' "$file"
echo
echo '--- assignments to _engine_dead / _worker_process_monitor and guards ---'
rg -n 'self\.(?:_engine_dead|_worker_process_monitor)\b|hasattr\(self, "_worker_process_monitor"|hasattr\(self, "_engine_dead"' "$file"Repository: NVIDIA/TensorRT-LLM
Length of output: 1067
Initialize inherited proxy state here
GenerationExecutorFrontendProxy inherits submit() and check_health() from GenerationExecutorProxy, but this constructor skips the parent __init__ that sets _engine_dead and _worker_process_monitor. Initialize those members here or override the inherited methods so attached frontends don’t hit AttributeError on submit or health checks.
🤖 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/executor/proxy.py` around lines 1024 - 1079, Initialize the
inherited proxy state in GenerationExecutorFrontendProxy.__init__ after the
deliberate GenerationExecutorProxy.__init__ skip, including _engine_dead and
_worker_process_monitor with the values expected by inherited submit() and
check_health(). Preserve the existing attached-frontend initialization and avoid
invoking the parent constructor’s MPI, worker-launch, or shutdown-hook setup.
Comment on lines
+4124
to
+4135
| num_serve_frontends: int = Field( | ||
| default=1, | ||
| ge=1, | ||
| # = executor.utils.MAX_NUM_FRONTENDS (cannot be imported here); | ||
| # test_multi_frontend_routing pins the two together. | ||
| le=64, | ||
| description= | ||
| "The number of HTTP frontend processes serving one executor. Used by " | ||
| "trtllm-serve: values > 1 run additional attached frontend processes " | ||
| "that share the serving port via SO_REUSEPORT (classic IPC executor " | ||
| "path only).", | ||
| status="prototype") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Regenerate the LLM args golden manifest and obtain CODEOWNER approval for this new field.
Adding num_serve_frontends to BaseLlmArgs requires updating the committed manifest and a privacy/telemetry sign-off, neither of which appears in this change set.
As per coding guidelines: "When changing LLM arguments or nested configuration, regenerate the golden manifest with python3 scripts/generate_llm_args_golden_manifest.py and commit tensorrt_llm/usage/llm_args_golden_manifest.json. New fields require telemetry/privacy CODEOWNER approval."
🤖 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/llmapi/llm_args.py` around lines 4124 - 4135, Regenerate the LLM
arguments golden manifest after adding BaseLlmArgs.num_serve_frontends using
python3 scripts/generate_llm_args_golden_manifest.py, and commit the updated
llm_args_golden_manifest.json. Obtain and include the required telemetry/privacy
CODEOWNER approval for this new field.
Source: Coding guidelines
Comment on lines
+66
to
+69
| def test_lane_index_clamps_to_launcher(self): | ||
| assert frontend_lane_index(namespace_client_id(2, 7), 3) == 2 | ||
| assert frontend_lane_index(namespace_client_id(7, 1), 2) == 0 | ||
| assert frontend_lane_index(None, 4) == 0 |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover negative client IDs to expose cross-frontend misrouting.
Coverage is insufficient for signed invalid IDs: frontend_lane_index(-1, 4) currently returns -1, causing Python indexing to route the response to the last frontend rather than launcher lane 0.
Proposed regression case
def test_lane_index_clamps_to_launcher(self):
assert frontend_lane_index(namespace_client_id(2, 7), 3) == 2
assert frontend_lane_index(namespace_client_id(7, 1), 2) == 0
assert frontend_lane_index(None, 4) == 0
+ assert frontend_lane_index(-1, 4) == 0The corresponding production bound should require 0 <= frontend_id < num_lanes.
As per path instructions, provide a concrete coverage assessment for tests/unittest/executor/test_multi_frontend_routing.py; this case is currently insufficiently covered.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_lane_index_clamps_to_launcher(self): | |
| assert frontend_lane_index(namespace_client_id(2, 7), 3) == 2 | |
| assert frontend_lane_index(namespace_client_id(7, 1), 2) == 0 | |
| assert frontend_lane_index(None, 4) == 0 | |
| def test_lane_index_clamps_to_launcher(self): | |
| assert frontend_lane_index(namespace_client_id(2, 7), 3) == 2 | |
| assert frontend_lane_index(namespace_client_id(7, 1), 2) == 0 | |
| assert frontend_lane_index(None, 4) == 0 | |
| assert frontend_lane_index(-1, 4) == 0 |
🤖 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/executor/test_multi_frontend_routing.py` around lines 66 - 69,
Add a regression assertion to test_lane_index_clamps_to_launcher covering a
negative client ID, such as frontend_lane_index(-1, 4) returning launcher lane
0. Update frontend_lane_index so its bound validation requires frontend_id to be
at least 0 and less than num_lanes, preserving valid routing and clamping all
invalid IDs to lane 0.
Source: Path instructions
Comment on lines
+192
to
+219
| def test_submit_namespaces_and_shutdown_never_sends_sentinel(self): | ||
| from tensorrt_llm.executor.request import CancellingRequest, GenerationRequest | ||
| from tensorrt_llm.sampling_params import SamplingParams | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmpdir: | ||
| proxy, worker_ingress, _, _ = self._make_proxy_and_fake_worker(tmpdir) | ||
|
|
||
| # Attributes read by OpenAIServer at init must exist (a missing | ||
| # _resource_governor_queue crashed all siblings in the first e2e). | ||
| assert proxy.resource_governor_queue is None | ||
|
|
||
| result = proxy.submit(GenerationRequest([1, 2, 3], SamplingParams())) | ||
| assert get_frontend_id(result.request_id) == 1 | ||
| assert worker_ingress.poll(5) | ||
| received = worker_ingress.get() | ||
| assert isinstance(received, GenerationRequest) | ||
| assert received.id == result.request_id | ||
|
|
||
| # Frontend shutdown aborts its in-flight requests (cancel) but | ||
| # must NOT emit the None engine-shutdown sentinel. | ||
| proxy.shutdown() | ||
| assert worker_ingress.poll(5) | ||
| cancel = worker_ingress.get() | ||
| assert isinstance(cancel, CancellingRequest) | ||
| assert cancel.id == result.request_id | ||
| assert not worker_ingress.poll(1), ( | ||
| "an attached frontend must never send the engine-shutdown sentinel" | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make proxy and IPC cleanup exception-safe.
An assertion failure can bypass proxy.shutdown(), and the second test never shuts down its proxy or closes its worker queues. This can leak ZeroMQ sockets and dispatcher threads into subsequent tests. Wrap both tests in try/finally and close every created queue after shutting down the proxy.
Also applies to: 221-249
🤖 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/executor/test_multi_frontend_routing.py` around lines 192 -
219, Make the affected proxy/IPC tests exception-safe by wrapping each test body
in try/finally, ensuring proxy.shutdown() runs even when assertions fail. In the
finally blocks, close every worker queue created by _make_proxy_and_fake_worker
after shutdown, including the queues used by the test spanning the referenced
second range, so ZeroMQ sockets and dispatcher threads are cleaned up.
Collaborator
|
PR_Github #59955 [ run ] completed with state
|
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #59990 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #59990 [ run ] completed with state
|
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #60078 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #60078 [ run ] completed with state
|
Collaborator
Author
|
/bot run --disable-fail-fast |
Collaborator
|
PR_Github #60084 [ run ] triggered by Bot. Commit: |
Collaborator
|
PR_Github #60084 [ run ] completed with state
|
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.
Description
Port of #16413 (base
feat/deepseek_v4) tomain, with review cleanups folded in.At high concurrency a
trtllm-serveworker is host-bound on its single serving process: one asyncio event loop (one GIL) performs every requestjson.loads+pydantic validate and every SSE chunk write. Queuing on that loop — not GPU time — then dominates first-token latency (measured on DSv4 disagg GEN:gen_preprocessingp50 54ms@c512 → 109ms@c1024 → 1520ms@c2048, an M/M/1-style blow-up while decode step time stayed flat).This PR adds a prototype multi-frontend mode on the default (classic IPC) executor path:
num_serve_frontends: K(llm-args knob /--num_serve_frontends, default 1 = off, max 64) runs K HTTP frontend processes against one executor.Design
TLLM_EXECUTOR_ATTACH_INFOset; their executor attaches via the newGenerationExecutorFrontendProxy(no MPI session, no worker launch, and shutdown never emits the worker'sNoneengine-shutdown sentinel — the launcher alone owns the engine lifecycle).TLLM_MULTI_FRONTEND_IPC_DIR/_HMAC): the rank0 worker binds the request ingress (PULL) so every frontend PUSH-connects; each frontend binds its own result lane (PULL) that the worker (non-postproc) or every postprocess worker (one PUSH pipe per frontend) sends to.client_id>>48. Responses without a usable client id (e.g. attention-DP dummy requests carryclient_id=None) route to the launcher, preserving today's silent-discard semantics. Frontend 0 / feature-off keeps ids bit-identical to today.Changes vs #16413 (review cleanups)
split_mpi_env()(stripsSLURM_/UCX_/HYDRA_/… identity vars, not justOMPI_/PMIX_/PMI_).num_serve_frontendsknob only (no env gate; validated1..64by Pydantic and the CLI).None/out-of-range ids) unified infrontend_lane_index(), used by_send_rspandbucket_responses_by_frontend.rpc_common.namespace_client_id→utils.namespace_client_id); unit tests extended for the new helpers.TLLM_EXECUTOR_ATTACH_INFOenv value — nothing is written to disk — and the child pops the env once consumed; the shared ipc directory is owned and removed by the launcher's executor proxy.trtllm-serveentry point: disaggregated ctx/gen MPI workers (which also reachlaunch_serverand inherit the submitter's env under Slurm export-all) passmulti_frontend_enabled=Falseand log a warning if the knob is set.--api-server-countshape (status="prototype"):GenerationExecutorProxyreads it fromllm_argsand owns the shared ipc dir + HMAC key end-to-end (generated in__init__, removed inshutdown) — no env side-channels. The attach-info env bootstrap remains only where it must — across the child re-exec boundary — and theTLLM_EXECUTOR_FRONTEND_IDpairing is validated with a clear error.Scope / limitations (prototype)
orchestrator_type: rpc/rayare rejected with a clear error.num_serve_frontendsllm-args knob (prototype status), capped at 64./metrics,/perf_metrics,/healthare served per-frontend (SO_REUSEPORT samples one frontend per request); cross-frontend aggregation is follow-up work.enable_resource_governoris rejected in multi-frontend mode (the governor signal only reaches the launcher).Summary by CodeRabbit
num_serve_frontendsconfiguration option, supporting 1–64 frontends.