-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[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
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 |
|---|---|---|
|
|
@@ -490,6 +490,46 @@ def _init_multi_frontend_mode(llm_args: dict, | |
| return mode | ||
|
|
||
|
|
||
| # Default for the plain `trtllm-serve` subcommand. At high concurrency one | ||
| # serving process is host-bound (a single asyncio loop parses every request | ||
| # and writes every streamed chunk), so several frontends per executor pay off | ||
| # by default. The LlmArgs field itself keeps default 1: a bare LLM() has no | ||
| # HTTP frontends to fan out to, and only trtllm-serve can spawn them. | ||
| 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]) -> None: | ||
| """Fall back to one frontend when the *default* count cannot be honored. | ||
|
|
||
| Multi-frontend mode only exists on the classic IPC executor path behind | ||
| the OpenAI HTTP server on a fixed port. When the user did not ask for a | ||
| specific count (neither on the CLI nor in the --config YAML) and the | ||
| configuration can only run a single frontend, run one instead of failing | ||
| at startup. An explicit request keeps the loud error from the guard that | ||
| owns the incompatibility (gRPC / port 0 / orchestrator_type / governor). | ||
| """ | ||
| num_frontends = llm_args.get("num_serve_frontends", 1) | ||
| if requested or num_frontends <= 1: | ||
| return | ||
| if grpc: | ||
| 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" | ||
| else: | ||
| return | ||
| logger.info( | ||
| f"num_serve_frontends defaults to {num_frontends}, but {reason} " | ||
| "supports a single serving frontend only; running 1 frontend. Pass " | ||
| "--num_serve_frontends explicitly to override.") | ||
| llm_args["num_serve_frontends"] = 1 | ||
|
|
||
|
|
||
| def _spawn_attached_frontends(llm, num_frontends: int) -> list: | ||
| """Spawn num_frontends - 1 attached serving frontend processes. | ||
|
|
||
|
|
@@ -653,7 +693,8 @@ 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, | ||
|
|
@@ -1103,10 +1144,14 @@ def launch_visual_gen_server( | |
| status="prototype") | ||
| @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 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). The " | ||
| "default falls back to 1 for configurations that support " | ||
| "a single frontend only (--grpc, --port 0/--report_addr, " | ||
| "orchestrator_type, enable_resource_governor); an explicit " | ||
| "value fails instead.", | ||
| status="prototype") | ||
| @stability_option("--num_input_processor_workers", | ||
| type=click.IntRange(min=1), | ||
|
|
@@ -1468,6 +1513,16 @@ def _serve_llm(): | |
| llm_args = update_llm_args_with_extra_dict( | ||
| llm_args, llm_args_extra_dict, explicit_cli_keys=explicit_cli_keys) | ||
|
|
||
| # The multi-frontend default only applies where it can run; an | ||
| # explicit CLI flag or YAML key is honored as-is (and may fail loudly). | ||
| _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) | ||
|
|
||
| _apply_effective_telemetry_config(llm_args, telemetry=telemetry) | ||
|
|
||
| metadata_server_cfg = parse_metadata_server_config_file( | ||
|
|
@@ -1504,7 +1559,7 @@ def _serve_llm(): | |
| media_io_kwargs=parsed_media_io_kwargs) | ||
|
|
||
| if grpc: | ||
| if num_serve_frontends != 1: | ||
| if llm_args.get("num_serve_frontends", 1) != 1: | ||
|
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. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '480,545p' tensorrt_llm/commands/serve.py
sed -n '1490,1575p' tensorrt_llm/commands/serve.py
sed -n '1,180p' tests/unittest/llmapi/test_serve_num_frontends_default.py
rg -n --glob '*.py' 'num_serve_frontends.*grpc|grpc.*num_serve_frontends|--grpc' tests/unittest tests/integration | head -200Repository: NVIDIA/TensorRT-LLM Length of output: 13742 🏁 Script executed: rg -n --glob '*.py' 'CliRunner|extra_llm_api_options|trtllm.?serve|serve\.callback|num_serve_frontends' tests/unittest tests/integration | head -300
printf '\n--- candidate CLI test ---\n'
sed -n '150,235p' tests/unittest/api_stability/test_serve_cli.py
printf '\n--- target test tail ---\n'
wc -l tests/unittest/llmapi/test_serve_num_frontends_default.py
sed -n '1,240p' tests/unittest/llmapi/test_serve_num_frontends_default.pyRepository: NVIDIA/TensorRT-LLM Length of output: 46627 🏁 Script executed: printf '%s\n' '--- gRPC references in tests ---'
rg -n -C 12 --glob '*.py' -- '--grpc' tests/unittest tests/integration
printf '%s\n' '--- num_serve_frontends references in tests ---'
rg -n -C 8 --glob '*.py' 'num_serve_frontends' tests/unittest tests/integration
printf '%s\n' '--- config database context ---'
sed -n '180,355p' tests/unittest/llmapi/test_config_database.py
printf '%s\n' '--- report address CLI context ---'
sed -n '140,240p' tests/unittest/llmapi/test_serve_report_addr.pyRepository: NVIDIA/TensorRT-LLM Length of output: 40097 Add a YAML-backed gRPC regression test. The current tests cover YAML merging and default fallback separately. They do not pass an explicit YAML value through the 🤖 Prompt for AI Agents |
||
| raise click.UsageError( | ||
| "--num_serve_frontends must be 1 when --grpc is enabled.") | ||
|
|
||
|
|
||
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.