Repository navigation
[TRTLLM-17271][refactor] Declare trtllm-serve config precedence and server-only options once - #21992
Draft
JunyiXu-nv wants to merge 13 commits into
Draft
JunyiXu-nv wants to merge 13 commits into
JunyiXu-nv wants to merge 13 commits into
Conversation
…ller-owned config dicts On the trtllm-serve config path, launchers and config helpers wrote into mappings their callers still own: the telemetry opt-out helper, the KV cache pool provisioning, the HTTP/encoder/embedding launchers and both gRPC launchers edited the caller's llm_args in place, the shared merge function popped a key out of the caller's nested telemetry_config mapping, extract_disagg_cluster_config replaced a sub-mapping inside the user's YAML, and create_router wrote into the RouterConfig's args dict. A function that needs a changed mapping now copies first; one whose caller needs the change returns (or yields) the new mapping and the caller rebinds. _pop_bool_config_option is the one declared exception: removing the key from the caller's mapping is the effect both callers need. _init_multi_frontend_mode no longer pops num_serve_frontends when disabled; launch_server, its only enabled=False caller, drops the key from its own copy instead. The launcher calls in the serve, mm_embedding_serve and embeddings subcommands now bind every argument by keyword, so independent edits to a call site and a signature can no longer be auto-merged into a positionally misaligned call. Tests that read those calls by position read the keywords instead. tests/unittest/llmapi/test_serve_cli_no_mutation.py pins, per rewritten site, that the caller's mapping is unchanged and that the value handed downstream is the one handed down before; registered in l0_cpu.yml. No behavior change. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…ig-mutation Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…th the formatters and CODEOWNERS Create the package tensorrt_llm/commands/serve_cli/, which the trtllm-serve command line moves into. Its first module, _net.py, receives code from tensorrt_llm/commands/serve.py, so it is listed with the files the legacy formatters (yapf, isort, autoflake) keep formatting and the derived lint configs are regenerated. The package belongs to the runtime team in CODEOWNERS, like commands/serve.py. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…to commands/serve_cli/_net.py Move _publish_bound_address, _diagnose_port_in_use, _bind_failure and _listener_address_family from tensorrt_llm/commands/serve.py into tensorrt_llm/commands/serve_cli/_net.py unchanged. serve.py imports the four names back because its listeners still call them; nothing re-exports them (all four are private). The report-addr test imports the two it uses from the new module. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
… helper with its policy kept _listener_address_family takes a policy parameter with the three policies the listeners use today: auto (IPv6 only if the host resolves to IPv6 addresses only; the OpenAI HTTP server and the VisualGen server), ipv4 (always IPv4 without a lookup; the disaggregated server, its coordinator and its fleet workers, whose reason -- a link-local AAAA record resolves to fe80:: with scope id 0, which the kernel rejects -- now lives in the function's docstring) and first (the family of the first getaddrinfo() result; OpenEngine's pre-start trial bind). _bind_listener creates the socket, sets SO_REUSEADDR (and SO_REUSEPORT when several processes serve one port), binds it, and reports a failure through _bind_failure, closing the socket. The OpenAI HTTP server, the VisualGen server, the single disaggregated server and the coordinator bind through it; the fleet worker keeps its own bind (routing it through the helper would add a port-holder diagnosis and an error log and change its message) and OpenEngine keeps socket.create_server (it sets IPV6_V6ONLY and listens, so a trial bind through the helper would detect conflicts differently). Both take their family from the policy function. The report-addr test patches the port-holder diagnosis where _bind_failure looks it up, in the new module. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…d the wiring of the LLM subcommands test_serve_cli_net.py checks the three address-family policies and the bind helper with fake sockets, and, for each of the eight listener cases (the OpenAI HTTP server with one frontend, as the launcher of two and as an attached frontend; the VisualGen server; the single disaggregated server; the coordinator; a fleet worker; OpenEngine's trial bind), that the listener asks for the policy it has today and makes the SO_REUSEPORT choice it makes today. test_serve_cli_wiring.py drives trtllm-serve serve, mm_embedding_serve and embeddings through their click entry points with a distinct value for every server-side option and checks, by parameter name, what reaches the launcher, so an option wired to the wrong parameter or a shifted positional argument fails by name. Both files run in the CPU stage. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…rt-lock order of the HTTP listener Three listener tests: a port-0 bind of the OpenAI HTTP server and of the single disaggregated server publishes the port read back from the bound socket, not the requested 0; the launcher's port lock is reserved and entered before the listener bind and left after it, also when the bind fails. Each pins a line this change moved or re-arranged. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
… usage context out of later tests The disaggregated CLI callback sets the level of the process-wide TRT-LLM logger and the context of a live usage session before it reaches its listener. The two tests that run it left the logger at "info" and a live session's context at "disaggregated" for every later test in the process. The shared restored_environment fixture now stubs both calls, Logger.set_level on the class so the undo leaves no attribute on the logger instance, and set_usage_context in the module that holds the callback; the tests still exercise only the listener behavior. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…not part of the reviewable change) Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…erver-only options once The precedence of the --config YAML, the CLI flags and --set was written once per LLM subcommand, and which keys configure the server rather than the LLM was written four times: the keys popped out of the YAML, the reserved --set paths, the options the gRPC server rejects, and the schema generator's server keys and live objects. Declare every server-only option once in tensorrt_llm/commands/serve_cli/_serve_options.py (SERVE_OPTIONS) and put the precedence in tensorrt_llm/commands/serve_cli/_config_sources.py (ConfigSources.resolve()). serve, mm_embedding_serve and embeddings resolve their configuration through it; _RESERVED_PATH_REASONS, the gRPC rejection check and the schema generator's server keys and live objects derive from the declarations; the helpers that lost every caller are deleted. No behavior change: the derived lists are pinned to the hand-written ones by goldens, and a completeness test ties every Click option of the three subcommands to an LlmArgs field or a declaration. The generated schemas are unchanged. Stacked on the PRs that create commands/serve_cli (NVIDIA#21967) and stop the launchers from mutating caller-owned config dicts (NVIDIA#21394). Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…all site takes out of the YAML Review follow-up for the config-source declaration. The unit tests pin ConfigSources.resolve() per subcommand and the wiring tests pin the launcher arguments, but nothing tied a call site to the subcommand it resolves for: a wrong command= on the mm_embedding_serve or embeddings site would silently swallow a YAML key the LLM should reject. Two CLI-level cases now pass a server key through --config and check that it reaches the launcher's arguments (mm_embedding_serve passes internal_request_auth_key on and takes only allow_request_chat_template out; embeddings passes allow_request_chat_template on). Also assert that a top-level --set logs no "Explicit CLI flag(s) ... override" warning, whether or not the flag was typed: the --set root leaves the explicit set before the merge, which only the final value could not show. The module docstring describes the merge function without narrating what the change left alone. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
This branch has not been deployed
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
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.
@coderabbitai summary
Description
Stacked on #21967 ([TRTLLM-17271][refactor] Create commands/serve_cli with one bind helper for every trtllm-serve listener) and #21394 ([TRTLLM-17271][refactor] Stop trtllm-serve launchers from mutating caller-owned config dicts): the first commits below belong to those PRs and disappear from this diff when they merge; this PR's own change is the last 2 commits.
trtllm-serveresolves its LLM configuration from three sources: the--configYAML, the CLI flags and--set. Their precedence was written three times, once per LLM subcommand (serve,mm_embedding_serve,embeddings), and the knowledge of which keys configure the server rather than the LLM was written four times: the keys popped out of the YAML before the merge, the reserved--setpaths incommands/_config_overrides.py, the options the gRPC server rejects, and the server keys and live objects of the config-schema generator. Those copies had already drifted (mm_embedding_servehandles one of the three YAML keys,embeddingsnone;mpi_sessionin one list, its alias_mpi_sessionin another), and an option missed in one copy is silently ignored. This PR declares each server-only option once, intensorrt_llm/commands/serve_cli/_serve_options.py(SERVE_OPTIONS, 30 declarations: the subcommands that take it, its YAML key and validator, why--setrejects the key, and what the gRPC server does with it), puts the precedence in one place,tensorrt_llm/commands/serve_cli/_config_sources.py(ConfigSources(...).resolve()), and derives the four lists from the declarations. The three subcommands callConfigSources;_RESERVED_PATH_REASONS, the gRPC rejection check and the schema generator's server keys and live objects are derived; the helpers that lost every caller (_pop_bool_config_option,_apply_config_overrides) are deleted. A completeness test ties every Click option of the three subcommands to anLlmArgsfield or to a declaration, and goldens pin the four derived lists to what the hand-written copies said.Part of the trtllm-serve refactor (epic TRTLLM-17056): CLI and config (TRTLLM-17271). Builds on #21394 (the launchers no longer write into caller-owned config dicts, so a parameter object cannot change behavior silently) and #21967 (which creates
tensorrt_llm/commands/serve_cli/). Neither has merged yet; the move of the remaining serve commands intoserve_cli/has not been opened, so this PR editstensorrt_llm/commands/serve.pyandtensorrt_llm/commands/_config_overrides.pywhere the code lives today. It is not to be in review at the same time as that move.In flight on the same lines: #21486 "[None][fix] Cherry-pick optional native RankInfo authentication" removes the
internal_request_auth_keyentry from the gRPC rejection dict this PR replaces (SMG then accepts the key and forwards it) and adds_set_rank_info_auth_key, which writes into thellm_argsmapping it receives. Whichever lands second takes a small follow-up: theinternal_request_auth_keydeclaration becomesokfor both protocols, the three goldens that pin today's SMG rejection follow, and the helper returns the new mapping instead of writing in place (the rule #21394 introduces). The other open PRs ontensorrt_llm/commands/serve.py(#21944, #21405) and on the schema generator (#21950, #21778, #21734, #20887, #20877, #20802, #14159) do not overlap with these lines; the generator PRs edit field-level fragments, while this PR moves only the server-only keys and the live-object fragments into declarations.Behavior
No behavior change. In particular:
_config_sources.py): a YAML key replaces a CLI-derived value unless the flag was typed; inside a nested config an explicit convenience flag keeps its field (--free_gpu_memory_fraction,--kv_cache_dtype,--video_pruning_rate,--telemetry/--no-telemetry), the YAML's siblings stay and the YAML'stelemetry_config.usage_contextis dropped,hf_revisionis the YAML spelling ofrevision;--setbeats both (applied to the raw YAML first, its top-level roots stop protecting the explicit flag of the same name, applied again to the merged result);--no-telemetryand the YAML opt-out are handled at the call site as before; the server-only YAML keys leave the YAML before the merge (allow_request_chat_templateis flag OR YAML key,internal_request_auth_keyis YAML-only,disagg_clustercombines with--disagg_cluster_uriinextract_disagg_cluster_configexactly as before, including the YAMLcluster_uriwinning over the flag);--setrejects the reserved paths;--enable_sleep_modeaddsSleepConfig()only whensleep_configis unset.disagg_clusternow leaves the YAML before the merge instead of being popped from the merged dict after it. The value is the same: the merge copies unknown keys as they are,--setcannot target it, no CLI flag is nameddisagg_cluster, and nothing between the merge and the old pop (sleep_config, the frontend-count default, telemetry, startup capture) reads it.--grpcsilently ignores today (--media_io_kwargs,--num_input_processor_workers,--num_media_load_workers) stay ignored; they are declaredignoredso that the gap is visible. Turning them into errors is a follow-up behavior change. The gRPC check reports the same first option, in the same order, with the same message.mainreplaced mypy with Pyrefly ([None][infra] Introduce Pyrefly to do type check #21325), so the two new modules join the project through Pyrefly'stensorrt_llminclude instead of a mypyfileslist;scripts/run_pyrefly.sh --strictreports no diagnostic on them.The four lists, before and after
tests/unittest/llmapi/test_serve_cli_options.py)serve:allow_request_chat_template,internal_request_auth_keybefore the merge,disagg_clusterafter it;mm_embedding_serve:allow_request_chat_template;embeddings: noneyaml_options(command): the same keys per subcommand, all before the merge--setpaths (_RESERVED_PATH_REASONS)reserved_set_paths(): the same 13 paths and reasons,speculative_config.drafterbeforespeculative_config.resource_managertool_parser,middleware,chat_template,allow_request_chat_template,internal_request_auth_key(SMG only),metadata_server_config_file,server_role,disagg_cluster_configfirst_unsupported_grpc_option(protocol, values)over the same 8 in the same order;grpc_ignored()names the 3 ignored onesscripts/generate_trtllm_serve_schemas.pyyaml_options("serve")andLIVE_OBJECT_PATHS;python3 scripts/generate_trtllm_serve_schemas.py --checkpasses andtensorrt_llm/schemas/*.jsonare unchangedKnown exception recorded in the completeness test:
--agent_percentageand--agent_typesare mapped to nothing (they reachllm_argsas top-level keys although they are fields ofAgentTreeConfig); whether they work at run time is not verified, and fixing them is a separate PR.Moves
None.
CI bookkeeping
/tensorrt_llm/commands/serve_cliis owned by runtime-devs since [TRTLLM-17271][refactor] Create commands/serve_cli with one bind helper for every trtllm-serve listener #21967)ruff-legacy-baseline.json: nonetests/unittest/llmapi/test_serve_cli_config_sources.pyandtests/unittest/llmapi/test_serve_cli_options.pyregistered intests/integration/test_lists/test-db/l0_cpu.yml(next to the othertest_serve_cli_*entries)For
tensorrt_llm/serve/AGENTS.md(created by #21406, not yet onmain), the line to add under "Where code goes":Test Coverage
New (both
cpu_only, registered inl0_cpu.yml):tests/unittest/llmapi/test_serve_cli_config_sources.py(29 tests): table-driven, literal inputs and literal expected outputs for each precedence rule, the server-only keys per subcommand, the validation errors and their order,llm_yaml, inputs untouched andresolve()repeatable; that a--seton a key logs no "Explicit CLI flag(s) ... override" warning whether or not the flag was typed; and two CLI-level cases that tie each call site to its subcommand (mm_embedding_serve --configwithinternal_request_auth_keypasses the key on to the encoder arguments and takes onlyallow_request_chat_templateout;embeddings --configwithallow_request_chat_templatepasses it on to the LLM arguments).tests/unittest/llmapi/test_serve_cli_options.py(25 tests): every Click option ofserve,mm_embedding_serveandembeddingsis anLlmArgsfield or a declaration; every declaration names a real option; the schema's server-only keys equal the declared YAML keys; goldens of the four derived lists; the gRPC branch ofservepasses every declared rejected or ignored value to the check; the declaration rules (aserveoption must declare its gRPC behavior, a YAML key needs a--setreason and a schema).Updated:
tests/unittest/llmapi/test_trtllm_serve_config_schemas.py(the source guard no longer looks for the deleted helper;test_serving_yaml_fields_have_schemastill findssleep_configandnum_serve_frontendsinserve; the three server keys are now checked bytest_yaml_server_keys_match_the_schema),tests/unittest/grpc/test_grpc_optional.py(the merge no longer happens incommands/serve.py, so the stub ofupdate_llm_args_with_extra_dicton that module is dropped).Run locally on a CPU-only host, from the worktree, one pytest process per file:
*
test_protobuf_warning_in_fresh_process[python|upb]spawn a fresh interpreter whoseimport tensorrt_llmfails on the test host's compiled artifacts (operator trtllm::swiglu_nvfp4_quantize does not exist); the same two tests fail identically on the base commit with the same environment. Not related to this change; CI runs them with a matching build.Differential check while developing (not committed): the three hand-written code paths of the base commit, copied verbatim, against
ConfigSources.resolve()over 3000 random YAML / CLI /--setinputs per subcommand (23 fields, 3 server keys, nested configs, invalid values, conflicting--setpaths): 0 mismatches, including the cases that end in an error on both sides (362 forserve). A Hypothesis run of the same oracle plus the rules stated in words (scalar precedence, server keys out, inputs untouched,llm_yaml, the gRPC check against the old call-site normalizations) found no counter-example in 150 generated inputs per property, and a mutation gate of 16 planted bugs in the two new modules was fully detected; three further planted bugs from the review (a wrong subcommand on themm_embedding_serveorembeddingscall site, and a--setroot left in the explicit set) are caught by the two registered test files alone.GPU end-to-end, left to CI:
examples/serve/test_serve.py::test_config_override_precedence(YAML < CLI <--seton a GPU).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.🤖 Generated with Claude Code