Repository navigation
[None][feat] BREAKING: serve: default num_serve_frontends to 8 with single-frontend fallback #19379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1b2c96c
7b21070
de71e9e
609bfcd
0d9d714
77950f8
c9e69a1
cf7f800
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -488,6 +488,34 @@ def _diagnose_port_in_use(port: int) -> str: | |
| return "; ".join(details) | ||
|
|
||
|
|
||
| def _reserve_multi_frontend_port(host: str, port: int, | ||
| family: socket.AddressFamily) -> socket.socket: | ||
| """Refuse a port another server uses; return a lock to hold while serving. | ||
|
|
||
| SO_REUSEPORT would let a second server of the same user join the port. A | ||
| plain bind catches any listener (SO_REUSEADDR ignores TIME_WAIT), and an | ||
| abstract Unix socket named after the port catches a launcher that has not | ||
| started listening yet. | ||
| """ | ||
| lock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) | ||
| try: | ||
| lock.bind(f"\0trtllm-serve-{os.geteuid()}-{port}") | ||
| except OSError as e: | ||
| lock.close() | ||
| raise RuntimeError(f"Port {port} is already reserved by another " | ||
| f"multi-frontend trtllm-serve: {e}") from e | ||
| try: | ||
| with socket.socket(family, socket.SOCK_STREAM) as probe: | ||
| probe.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) | ||
| probe.bind((host, port)) | ||
| except OSError as e: | ||
| lock.close() | ||
| raise RuntimeError( | ||
| f"Failed to bind socket to {host}:{port}: {e}. " | ||
| f"Port holder(s): {_diagnose_port_in_use(port)}") from e | ||
| return lock | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def _provision_kv_cache_pool(llm_args: dict, | ||
| owns_engine: bool = True) -> Iterator[None]: | ||
|
|
@@ -558,6 +586,43 @@ def _share_prometheus_multiproc_dir(llm, ipc_dir: str) -> None: | |
| f"Shared PROMETHEUS_MULTIPROC_DIR for all frontends: {path}") | ||
|
|
||
|
|
||
| # One serving process is host-bound at high concurrency. LlmArgs keeps | ||
| # default 1: only trtllm-serve spawns frontends. | ||
| DEFAULT_NUM_SERVE_FRONTENDS = 8 | ||
|
|
||
|
|
||
| def _resolve_default_num_serve_frontends(llm_args: dict, *, requested: bool, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function has five branches of pure dict/flag logic and no test. Commit 80ee120 on this branch removed
Please restore a CPU unit test parametrized over the four fallback reasons plus the two passthrough cases that matter: |
||
| port: int, grpc: bool, | ||
| report_addr: Optional[str], | ||
| enable_sleep_mode: bool) -> None: | ||
| """Fall back to one frontend where the default count cannot run. | ||
|
|
||
| An explicit count (CLI, --config YAML or --set) is left alone, so the | ||
| guard that owns the conflict still rejects it. | ||
| """ | ||
| num_frontends = llm_args.get("num_serve_frontends", 1) | ||
| if requested or num_frontends <= 1: | ||
| return | ||
|
Comment on lines
+604
to
+605
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Reject explicit multi-frontend counts for registry-backed serving. When 🤖 Prompt for AI AgentsSource: Path instructions |
||
| if grpc: | ||
| # OpenEngine supports several frontends, but only when asked to. | ||
| reason = "--grpc" | ||
| elif port == 0 or report_addr: | ||
| reason = "--port 0 / --report_addr" | ||
| elif llm_args.get("orchestrator_type") is not None: | ||
| reason = f"orchestrator_type={llm_args['orchestrator_type']!r}" | ||
| elif llm_args.get("enable_resource_governor"): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback list is missing two configurations that are also single-frontend-only in practice: Every frontend (launcher and attached children) constructs its own Either add |
||
| reason = "enable_resource_governor" | ||
| elif enable_sleep_mode: | ||
| reason = "--enable_sleep_mode" | ||
| else: | ||
| return | ||
| logger.info( | ||
| f"num_serve_frontends defaults to {num_frontends} for the OpenAI HTTP " | ||
| f"server only; running 1 frontend because of {reason}. Pass " | ||
| "--num_serve_frontends explicitly to override.") | ||
| llm_args["num_serve_frontends"] = 1 | ||
|
|
||
|
|
||
| def launch_server( | ||
| host: str, | ||
| port: int, | ||
|
|
@@ -622,14 +687,17 @@ def launch_server( | |
| logger.warning( | ||
| "num_serve_frontends > 1: stateful Responses API storage " | ||
| "(store/previous_response_id) is disabled; the per-frontend " | ||
| "in-memory store cannot be shared across frontends.") | ||
| "in-memory store cannot be shared across frontends. Pass " | ||
| "--num_serve_frontends 1 to keep it.") | ||
| os.environ["TRTLLM_RESPONSES_API_DISABLE_STORE"] = "1" | ||
|
|
||
| addr_info = socket.getaddrinfo(host, port, socket.AF_UNSPEC, | ||
| socket.SOCK_STREAM) | ||
| address_family = socket.AF_INET6 if all( | ||
| [info[0] == socket.AF_INET6 for info in addr_info]) else socket.AF_INET | ||
| with socket.socket(address_family, socket.SOCK_STREAM) as s: | ||
| port_lock = (_reserve_multi_frontend_port(host, port, address_family) | ||
| if multi_frontend.is_launcher else contextlib.nullcontext()) | ||
| with port_lock, socket.socket(address_family, socket.SOCK_STREAM) as s: | ||
| # port == 0 lets the kernel pick the port; the caller then needs a way | ||
| # to learn it, either by service discovery or by report_addr. | ||
| assert port > 0 or disagg_cluster_config is not None or report_addr, ( | ||
|
|
@@ -1111,10 +1179,12 @@ def launch_visual_gen_server( | |
| @stability_option( | ||
| "--num_serve_frontends", | ||
| type=click.IntRange(min=1, max=MAX_NUM_FRONTENDS), | ||
| default=1, | ||
| default=DEFAULT_NUM_SERVE_FRONTENDS, | ||
| help="Number of HTTP or OpenEngine frontend processes serving one " | ||
| "executor; values > 1 share the serving port via " | ||
| "SO_REUSEPORT (classic IPC executor path only).", | ||
| "SO_REUSEPORT (classic IPC executor path only). Without an explicit " | ||
| "value, --grpc and configurations that support a single frontend only " | ||
| "use 1 and log why.", | ||
| status="prototype") | ||
| @stability_option("--num_input_processor_workers", | ||
| type=click.IntRange(min=1), | ||
|
|
@@ -1556,6 +1626,15 @@ def _serve_llm(): | |
| if enable_sleep_mode and llm_args.get("sleep_config") is None: | ||
| llm_args["sleep_config"] = SleepConfig() | ||
|
|
||
| _resolve_default_num_serve_frontends( | ||
| llm_args, | ||
| requested=("num_serve_frontends" in explicit_cli_keys | ||
| or "num_serve_frontends" in llm_args_extra_dict), | ||
| port=port, | ||
| grpc=grpc, | ||
| report_addr=report_addr, | ||
| enable_sleep_mode=enable_sleep_mode) | ||
|
|
||
| _apply_effective_telemetry_config(llm_args, telemetry=telemetry) | ||
| # Preserve requested settings for failures before entering the LLM constructor. | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| """CPU tests for trtllm-serve's multi-frontend default, its fallback to one, and its port guard.""" | ||
|
|
||
| import socket | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
|
|
||
| from tensorrt_llm._utils import get_free_port | ||
| from tensorrt_llm.commands import serve as serve_cmd | ||
| from tensorrt_llm.commands.serve import ( | ||
| DEFAULT_NUM_SERVE_FRONTENDS, | ||
| _reserve_multi_frontend_port, | ||
| _resolve_default_num_serve_frontends, | ||
| ) | ||
|
|
||
| pytestmark = pytest.mark.cpu_only | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "llm_args,conflict,reason", | ||
| [ | ||
| pytest.param({}, {"grpc": True}, "--grpc", id="grpc"), | ||
| pytest.param({}, {"port": 0}, "--port 0", id="port0"), | ||
| pytest.param({}, {"report_addr": "/tmp/bound.addr"}, "--report_addr", id="report_addr"), | ||
| pytest.param({"orchestrator_type": "rpc"}, {}, "orchestrator_type='rpc'", id="rpc"), | ||
| pytest.param({"enable_resource_governor": True}, {}, "enable_resource_governor", id="rg"), | ||
| pytest.param({}, {"enable_sleep_mode": True}, "--enable_sleep_mode", id="sleep_mode"), | ||
| ], | ||
| ) | ||
| def test_default_falls_back_to_one_frontend(monkeypatch, llm_args, conflict, reason) -> None: | ||
| messages = [] | ||
| monkeypatch.setattr(serve_cmd.logger, "info", lambda msg, *a, **k: messages.append(msg)) | ||
| llm_args = {"num_serve_frontends": DEFAULT_NUM_SERVE_FRONTENDS, **llm_args} | ||
| no_conflict = dict( | ||
| requested=False, | ||
| port=8000, | ||
| grpc=False, | ||
| report_addr=None, | ||
| enable_sleep_mode=False, | ||
| ) | ||
|
|
||
| _resolve_default_num_serve_frontends(llm_args, **{**no_conflict, **conflict}) | ||
|
|
||
| assert llm_args["num_serve_frontends"] == 1 | ||
| assert len(messages) == 1 and reason in messages[0] and "--num_serve_frontends" in messages[0] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "args,config,expected", | ||
| [ | ||
| pytest.param([], None, DEFAULT_NUM_SERVE_FRONTENDS, id="default"), | ||
| pytest.param([], "num_serve_frontends: 1", 1, id="yaml_opt_out"), | ||
| pytest.param(["--port", "0"], None, 1, id="port0"), | ||
| pytest.param(["--enable_sleep_mode"], None, 1, id="sleep_mode"), | ||
| # An explicit count is honored; launch_server's own guard rejects the conflict. | ||
| pytest.param(["--port", "0", "--num_serve_frontends", "8"], None, 8, id="explicit"), | ||
| ], | ||
| ) | ||
| def test_frontend_count_reaching_launch_server( | ||
| monkeypatch, tmp_path, args, config, expected | ||
| ) -> None: | ||
| monkeypatch.setenv("TRTLLM_RUNTIME_CONTROL_API_KEY", "secret") # for --enable_sleep_mode | ||
| if config is not None: | ||
| (tmp_path / "config.yaml").write_text(config) | ||
| args = [*args, "--config", str(tmp_path / "config.yaml")] | ||
| with ( | ||
| mock.patch.object(serve_cmd, "get_is_diffusion_only_model", return_value=False), | ||
| mock.patch.object(serve_cmd, "device_count", return_value=1), | ||
| mock.patch.object(serve_cmd, "launch_server") as launch, | ||
| ): | ||
| serve_cmd.main(args=[str(tmp_path), *args], standalone_mode=False) | ||
|
|
||
| assert launch.call_args.args[2].get("num_serve_frontends", 1) == expected | ||
|
|
||
|
|
||
| def test_port_used_by_another_server_is_refused() -> None: | ||
| # Without the check, SO_REUSEPORT would let this launcher join the port. | ||
| with socket.socket() as other: | ||
| other.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEPORT, 1) | ||
| other.bind(("127.0.0.1", 0)) | ||
| other.listen() | ||
| port = other.getsockname()[1] | ||
| with pytest.raises(RuntimeError, match="Failed to bind socket"): | ||
| _reserve_multi_frontend_port("127.0.0.1", port, socket.AF_INET) | ||
|
|
||
|
|
||
| def test_port_reserved_by_another_launcher_is_refused() -> None: | ||
| # A launcher binds its port long before it listens: only its lock shows. | ||
| port = get_free_port() | ||
| with _reserve_multi_frontend_port("127.0.0.1", port, socket.AF_INET): | ||
| with pytest.raises(RuntimeError, match="another multi-frontend trtllm-serve"): | ||
| _reserve_multi_frontend_port("127.0.0.1", port, socket.AF_INET) | ||
| # The lock goes with the launcher. | ||
| _reserve_multi_frontend_port("127.0.0.1", port, socket.AF_INET).close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the
LlmArgsfield at 1 so a bareLLM()never pays for the fan-out is exactly the right split, and the fallback resolution is careful.What gives me pause is the upgrade path for someone who never types the flag. Per the description, with 8 frontends
/metricsand/prometheus/metricsdescribe only the frontend that answered, so iteration-derived series see ~1/8 of traffic and counters look like they reset between scrapes — with aggregation left as follow-up. The Responses store also silently turns itself off, and the table shows healthy-after going 54 s to 154 s.Would it be worth holding the default at 1 until the aggregation work lands, so the flip ships with a correct
/metrics? The rest of this I'd take happily on its own.Required for this PR, I think, rather than a nit — it changes what every existing deployment reports.