Skip to content

[TRTLLM-15264][doc] Kimi K3 disagg: stop recommending a UCX_TLS pin by default - #17479

Merged
nv-guomingz merged 1 commit into
NVIDIA:mainfrom
brnguyen2:k3/15264-readme-ucx-tls
Aug 11, 2026
Merged

[TRTLLM-15264][doc] Kimi K3 disagg: stop recommending a UCX_TLS pin by default#17479
nv-guomingz merged 1 commit into
NVIDIA:mainfrom
brnguyen2:k3/15264-readme-ucx-tls

Conversation

@brnguyen2

@brnguyen2 brnguyen2 commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Description

Follow-up to review feedback on #17334 (#17334 (comment)).

The K3 disagg README exported UCX_TLS=tcp,self,sm,cuda_copy,cuda_ipc as a launch prerequisite, and the benchmark config hard-coded the same pin in worker_env_var. That transport list excludes RDMA/verbs, so carrying it to clusters whose inter-node communication depends on verbs can fail initialization or silently force traffic onto TCP.

This change makes no pin the default:

  • The launch-sequence prerequisite is now "leave UCX_TLS unset" (UCX selects transports itself, RDMA/verbs included), with a pointer to the troubleshooting caveats.
  • benchmark_kimi_k3_dep16.yaml no longer carries TRTLLM_WORKER_UCX_TLS in worker_env_var; the harness's start_worker.sh already clears any container-provided UCX_TLS (a container-default UCX_TLS=tcp breaks V2 NIXL VRAM registration), so workers default to UCX's own selection.
  • The transport override is documented in the troubleshooting caveats only, scoped to clusters whose verbs transports cannot initialize, with an explicit warning that the list excludes RDMA/verbs.

Test Coverage

Documentation/example-config change only; no code paths affected.

PR Checklist

  • PR title follows the [TRTLLM-15264][doc] convention
  • pre-commit run on the touched files

Dev Engineer Review

  • Updated Kimi K3 disaggregated deployment guidance to leave UCX_TLS unset by default.
  • Removed TRTLLM_WORKER_UCX_TLS from the example worker environment.
  • Documented the fallback override for clusters where verbs transports cannot initialize.
  • Configuration changes are scoped to documentation and an example YAML file.
  • No code, API, performance, or error-handling changes were made.
  • No test-list changes were made.

QA Engineer Review

No test changes.

…y default

The K3 disagg README exported UCX_TLS=tcp,self,sm,cuda_copy,cuda_ipc as a
launch prerequisite and the benchmark config hard-coded the same pin in
worker_env_var. That transport list excludes RDMA/verbs, so carrying it to
clusters whose inter-node traffic depends on verbs can fail initialization
or silently force traffic onto TCP.

Make no pin the default: leave UCX_TLS unset so UCX selects transports
itself (the harness already clears any container-provided UCX_TLS=tcp,
which breaks V2 NIXL VRAM registration), and move the transport override
to the troubleshooting caveats, scoped to clusters where verbs transports
cannot initialize.

Addresses review feedback on NVIDIA#17334.

Signed-off-by: Brian Nguyen <brnguyen@nvidia.com>
@brnguyen2
brnguyen2 requested a review from a team as a code owner August 10, 2026 22:35
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 192090cb-4336-455d-9d52-bd77ca1034cf

📥 Commits

Reviewing files that changed from the base of the PR and between c67879c and b5fac8a.

📒 Files selected for processing (2)
  • examples/kimi_k3/disagg/README.md
  • examples/kimi_k3/disagg/benchmark_kimi_k3_dep16.yaml

Walkthrough

The Kimi K3 disaggregated deployment guidance now leaves UCX transport selection automatic by default. It documents clearing container-provided settings and using TRTLLM_WORKER_UCX_TLS only when verbs initialization fails.

Changes

UCX transport guidance

Layer / File(s) Summary
Transport selection and fallback policy
examples/kimi_k3/disagg/README.md
Manual and cluster guidance now leaves UCX_TLS unset by default, rejects container-provided UCX_TLS=tcp for V2 NIXL VRAM registration, and documents the TCP/CUDA fallback for verbs initialization failures.
Worker override wiring
examples/kimi_k3/disagg/README.md, examples/kimi_k3/disagg/benchmark_kimi_k3_dep16.yaml
SLURM startup clears container-provided UCX_TLS and reapplies an optional TRTLLM_WORKER_UCX_TLS value. The benchmark configuration no longer sets the fallback override by default.

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

Suggested reviewers: bowenfu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format and clearly summarizes the main documentation change.
Description check ✅ Passed The description includes the required sections and clearly explains the change and test coverage.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@brnguyen2

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65139 [ run ] triggered by Bot. Commit: b5fac8a Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65139 [ run ] completed with state FAILURE. Commit: b5fac8a
/LLM/main/L0_MergeRequest_PR pipeline #52935 completed with status: 'FAILURE'

CI Report

⚠️ Action Required:

  • Please check the failed tests and fix your PR
  • If you cannot view the failures, ask the CI triggerer to share details
  • Once fixed, request an NVIDIA team member to trigger CI again

CI Agent Failure Analysis

Link to invocation

@brnguyen2

Copy link
Copy Markdown
Collaborator Author

/bot run

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65190 [ run ] triggered by Bot. Commit: b5fac8a Link to invocation

@tensorrt-cicd

Copy link
Copy Markdown
Collaborator

PR_Github #65190 [ run ] completed with state SUCCESS. Commit: b5fac8a
/LLM/main/L0_MergeRequest_PR pipeline #52979 completed with status: 'SUCCESS'

CI Report

Link to invocation

@nv-guomingz nv-guomingz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nv-guomingz
nv-guomingz merged commit fdbe8d5 into NVIDIA:main Aug 11, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants