[spicysharkco]

Description

trtllm-serve --disagg_cluster_uri is documented to take precedence over disagg_cluster.cluster_uri in the config file (see the comment in tensorrt_llm/commands/serve.py above the extract_disagg_cluster_config call, and the docstring of that function). In practice the command line value is ignored whenever the config file also sets disagg_cluster.cluster_uri, and the uri from the file is used instead.

Root cause. extract_disagg_cluster_config() builds DisaggClusterConfig(cluster_uri or cluster_config_dict["cluster_uri"]), so the argument is used for the constructor. It then runs update_dataclass() over the same object with the whole cluster_config_dict. That loop sets every non-None key, including the dict's own cluster_uri, and overwrites the value that was just passed in. The override only worked when the file had no cluster_uri at all.

Repro (no GPU needed):

from tensorrt_llm.llmapi.disagg_utils import extract_disagg_cluster_config

cfg = extract_disagg_cluster_config(
    {"cluster_uri": "http://file-host:8000"}, "http://cli-host:2379")
print(cfg.cluster_uri)
# before: http://file-host:8000
# after:  http://cli-host:2379

Fix. Apply the override after the config file values have been copied onto the dataclass. When no override is given, nothing changes. A config with neither a file uri nor an argument still raises KeyError as before.

release/1.3 has the same code and the same bug. #20128 also edits the import list in test_disagg_utils.py; whichever merges second needs a trivial rebase.

This was found by reading the code path and confirmed with the unit test below. I did not launch a disaggregated deployment, so the trtllm-serve side of the behavior is covered only through extract_disagg_cluster_config, which is the single place serve.py takes the value from.

Test Coverage

tests/unittest/disaggregated/test_disagg_utils.py::test_extract_disagg_cluster_config_uri_argument has two cases: no argument keeps the uri from the config, and an argument replaces it (the other fields still come from the config). The second case fails before this change.

Run on CPU only with Python 3.12 and pytest, against the real disagg_utils.py. The compiled bindings are not built there, so mpi4py and tensorrt_llm._utils.global_mpi_rank/global_mpi_size were replaced by small stubs; extract_disagg_cluster_config uses neither. GPU CI and a real trtllm-serve launch were not run.

disagg_utils.py	test_disagg_utils.py
base (a857768f9)	1 failed (the argument case), 41 passed
with this fix	42 passed

Formatting hooks (yapf, isort, autoflake) pass on both touched files.

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-compatible or api-breaking. For api-breaking, include BREAKING in 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.

Dev Engineer Review

The inspected diff changes only two docstrings in tensorrt_llm/llmapi/disagg_utils.py. The current extract_disagg_cluster_config() implementation reapplies a truthy cluster_uri argument after copying dictionary values, so the argument takes precedence. The diff does not establish that this behavior was introduced by the changes reviewed.

QA Engineer Review

No test changes.

Per-File QA Perspective

• tensorrt_llm/llmapi/disagg_utils.py: The current function uses the argument URI when it is truthy and otherwise uses the config URI. Verify both paths and confirm that a missing URI still raises KeyError; the inspected diff does not change this behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: NVIDIA/TensorRT-LLM/.coderabbit.yaml
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 1eebc575-9858-444c-946d-79883c31eea5

📥 Commits

Reviewing files that changed from the base of the PR and between c226e2d and 074df1c.

📒 Files selected for processing: 2

• tensorrt_llm/llmapi/disagg_utils.py
• tests/unittest/disaggregated/test_disagg_utils.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

Walkthrough

extract_disagg_cluster_config now applies a truthy CLI cluster URI after copying configuration values. Tests cover the YAML URI when the CLI argument is None and the CLI URI when provided.

Changes

Cluster URI selection

Layer / File(s)	Summary
Apply and test URI precedence tensorrt_llm/llmapi/disagg_utils.py, tests/unittest/disaggregated/test_disagg_utils.py	A truthy CLI URI overwrites the URI copied from the configuration dictionary. Tests check both URI cases and confirm the configured cluster name.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: brnguyen2

Merge Risk: ⚪ Minimal · up to 074df

The requested cluster URI precedence behavior is implemented and covered for both CLI and configuration sources, with no merge-blocking risk identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title follows the required format and clearly states that the command-line disaggregated cluster URI overrides the configuration-file URI.
Description check		The description clearly explains the issue, root cause, fix, reproduction, test coverage, limitations, and checklist status. It provides sufficient detail for review.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}