[TRTLLM-12440][feat] Add GMS-only weight sharing support#13926
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #47456 [ run ] triggered by Bot. Commit: |
|
PR_Github #47456 [ run ] completed with state
|
03ddff2 to
e0fa384
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #47789 [ run ] triggered by Bot. Commit: |
|
PR_Github #47789 [ run ] completed with state
|
…managed deployment Append a new section to §06 distinguishing two failover models: - Shadow failover (existing): pre-warmed standby, sub-5s activation, requires §07 compile cache + new executor state machine. - Restart-after-death (new section): no standby, external supervisor restarts trtllm-serve, replacement zero-copy materializes from surviving GMS daemon via PR NVIDIA#13926's RO branch. Works on main today (no new TRT-LLM code). Document the supporting gaps: - TRT-LLM has no built-in worker restart machinery; restart-after-death requires an external supervisor (systemd / K8s / Dynamo). - GMS daemon must be a node-level service, not a worker subprocess. - Daemon commit-survival on writer disconnect is the load-bearing upstream assumption (GMS-7 open question). - Sleep/wake (NVIDIA#13918 + NVIDIA#14052) doesn't compose with GMS weights — three implementation tiers documented; Tier-A validator recommended for NVIDIA#13926. Add a self-managed deployment recipe: systemd unit for the GMS daemon, trtllm-serve unit with --gms-mode=auto for the worker, verification steps, and caveats (single-rank only, daemon lifetime discipline, no auto sleep/wake for weights, compile-cache cold-restart cost pre-§07). README TOC entry updated to point at the new section. Signed-off-by: Chien-Chun Hung <chienchunh@nvidia.com>
|
/bot run --disable-fail-fast |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
a9ba2d5 to
148c490
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #48230 [ run ] triggered by Bot. Commit: |
|
PR_Github #48230 [ run ] completed with state
|
Add §16 capturing the holistic fix for the conflated post_load_weights() semantics surfaced independently by PR NVIDIA#13926 (GMS RO ordering) and PR NVIDIA#14151 (MX publish-pre vs publish-post-transform). Documents: - The four-category breakdown of what post_load_weights() does today (alias wiring / data transforms / per-process state / derived Python state) and why each consumer needs a different subset. - Proposed staged-hook protocol: setup_aliases / transform_weights / cache_derived_state, with a default orchestrator for backward compat. - Per-path orchestration sketches for AUTO+HF, AUTO+MX (post-transform), GMS RW, GMS RO. - Migration inventory: 23 LLM-relevant files bucketed by category; calling-convention nuance for QuantMethod nested callbacks. - Tiny prep PR scope (~50-100 LOC, base class only) and family-PR migration sequence (~600-800 LOC total over 4-5 follow-ups). - 7 open questions with sensible defaults. Updates §7 of 05-challenges.md to forward-link the holistic fix and acknowledge the per-PR mitigation's residual divergence. Updates README.md TOC and Last-Updated date. Signed-off-by: Chien-Chun Hung <chienchunh@nvidia.com> Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…view Apply 5 review-driven edits to §16 to address residual concerns: Walk ordering (concern NVIDIA#8): change GMS RO and MX-receiver alias walks from per-module to top-level model.setup_aliases(). Matches the §7 mitigation contract from ai-dynamo/dynamo PR NVIDIA#7053 ("Call model.post_load_weights() (top-level only) before materialize_module_from_gms()"). transform_weights() and cache_derived_state() walks remain per-module since those bodies live on submodules. New "Why setup_aliases() is top-level-only" callout documents the asymmetry. Lifecycle of _weights_transformed (concern #4): new subsection specifying explicit set/reset/orthogonality rules. Includes a 2x2 truth table showing _weights_removed and _weights_transformed track different lifecycles and can take any combination. Reset is the orchestrator's responsibility (e.g., ModelLoader.reload() resets the flag before re-binding tensors); subclasses do not manage reset. Hard preconditions (concern #5): promote MX source-identity completeness from "open question" to "hard precondition P1." Lists transform-affecting parameters that MX identity must cover (attn_backend, quant backend list, FP8/NVFP4 fusion strategy, TP/EP layout, model revision). Specifies an in-tree backend-fingerprint fail-safe as the fallback if upstream MX cannot guarantee completeness. P2 documents that orchestrator owns _weights_transformed reset. Removes redundant open question NVIDIA#6 from the table. Cosmetic fixes (concerns #2, NVIDIA#6): "four stages" -> "three per-module stages plus orchestrator-managed per-process finalization." cache_derived_state description softened to "reserved for data-dependent state where it exists; many existing modules will have empty bodies." Scope clarifications (concerns #1, #3, NVIDIA#7): - Tiny PR scope reframed as "duck-typed helpers, not inheritance" with citations to existing model_loader.py walker pattern. Lists 4 walker helpers (_setup_aliases, _walk_transform, _walk_cache_state, _walk_full_post_load). - Migration callout: when migrating a subclass, the old post_load_weights() override MUST be removed; otherwise the new staged calls silently no-op. - Family PR #2 (Linear/Attention) gains a "quant-method callback decision" note with default = keep quant_method.post_load_weights callback name (no rename). No code changes. Drives Tiny prep PR scope and family-PR migration sequence. References: - TRT-LLM PR NVIDIA#13926 (GMS-only) - TRT-LLM PR NVIDIA#14151 (MX shim refactor) - ai-dynamo/dynamo PR NVIDIA#7053 (upstream GMS prototype) Signed-off-by: Chien-Chun Hung <chienchunh@nvidia.com> Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py (1)
1-15: QA list update not needed for this PR scope.These changes are confined to
tests/unittest/..., so QA integration/release test-list entries undertests/integration/test_lists/qa/are unnecessary here.As per coding guidelines "If the PR only touches unittest/ or narrow unit scope, say explicitly whether QA list updates are unnecessary or optional."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py` around lines 1 - 15, Add an explicit note inside the module docstring of MXCheckpointLoader tests (tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py) stating that QA integration/release test-list updates under tests/integration/test_lists/qa/ are unnecessary for this PR because changes are confined to unit tests; update the top-level docstring (the existing triple-quoted description) to include a one-sentence statement like "QA list updates under tests/integration/test_lists/qa/ are unnecessary for this PR scope." so reviewers and release engineers see it immediately.tests/unittest/_torch/pyexecutor/test_model_loader_gms.py (1)
1-3: QA list update is unnecessary for this test scope.These changes are confined to
tests/unittest/**; no integration test definition/list changes are required intests/integration/test_lists/qa/for this PR.As per coding guidelines, if PR scope is unittest-only, QA scheduled list updates are unnecessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unittest/_torch/pyexecutor/test_model_loader_gms.py` around lines 1 - 3, This PR only changes the unit test test_model_loader_gms.py and therefore the QA scheduled list updates included in the diff are unnecessary; remove or revert any modifications to the integration QA list files that were accidentally included in this PR, ensure the commit/PR only contains the unit test changes (keep changes in test_model_loader_gms.py), and update the commit message/PR description to reflect that this is a unittest-only change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_torch/memory/gpu_memory_backend.py`:
- Around line 205-206: When clearing the GPU memory backend client you must also
reset the _is_rw flag to None; update the connect failure branch where
self._client is set to None and the cleanup() method to set self._is_rw = None
as well so is_rw reports None after disconnect/cleanup. Locate the places
referencing self._client being cleared (the connect failure path and the
cleanup() method) and add resetting of the unique attribute _is_rw to None
immediately after clearing self._client.
- Around line 363-380: The loop in move_untracked_params currently skips
rebinding tensors that share a storage because it continues when
storage_base_ptr is seen in tracked_storage_ptrs; instead maintain a dict (e.g.,
storage_to_base_va or storage_to_replacement_info) keyed by storage_base_ptr so
you only call gms_client.create_mapping(...) once per storage but for every
tensor/view reuse the cached base_va (or cached replacement pointer/metadata) to
build or obtain the replacement and set tensor.data = replacement; update
references to tracked_storage_ptrs to store the mapping key and ensure the
create_mapping path runs only on cache misses while the tensor.data rebinding
runs for every tensor regardless of prior seen storages.
- Around line 190-206: The connect() block in GMSBackend currently swallows all
errors by using a broad `except Exception` around
get_or_create_gms_client_memory_manager(...) which can hide real regressions;
change this to catch only the expected client/socket-related exceptions (for
example OSError, ConnectionRefusedError, TimeoutError or the specific GMS client
exception type if one exists) and handle those by setting self._client = None
and returning False, while allowing any other unexpected exceptions to propagate
(or re-raise) so they don't get masked; update the except clause in
GMSBackend.connect (the call to get_or_create_gms_client_memory_manager with
mode_map[self._mode] and tag=self._tag) to list the specific exception types or
re-raise after logging when the caught exception is not one of the expected
ones.
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 879-881: Move engine teardown to run before the resource manager
shutdown and consolidate teardown logic: for each of self.model_engine and
self.draft_model_engine, if engine is not None then call engine.cleanup() if it
exists, otherwise call engine._release_cuda_graphs() as a fallback; perform this
before calling manager.shutdown() and remove the later post-shutdown engine
cleanup path so engines are torn down via a single ordered hook prior to
manager.shutdown().
In `@tensorrt_llm/llmapi/llm_args.py`:
- Around line 3718-3754: Change the GmsConfig.mode annotation from str to
Literal["auto","rw","ro"] so the schema/docs show the enum; add the import for
typing.Literal; update the model_validator validate_gms_config to stop
re-checking mode (remove the membership if-block) and only enforce that tag is
non-empty / not whitespace-only (raise ValueError for tag issues), or keep a
defensive assertion if you prefer but avoid duplicating the whitelist logic now
provided by the Literal typing; reference the GmsConfig class, the mode field,
and validate_gms_config when making these edits.
- Around line 4144-4145: convert_load_format currently treats bool as int
because bool is a subclass of int, causing True/False to be coerced into
LoadFormat; update convert_load_format to explicitly detect and reject bool
before the int branch (e.g., if isinstance(v, bool): raise TypeError or
ValueError with a clear message), then keep the existing int branch (if
isinstance(v, int): return LoadFormat(v)) so only true integers are converted to
LoadFormat.
In `@tests/unittest/_torch/pyexecutor/test_model_loader_gms.py`:
- Line 168: The test's assertion for checkpoint_loader.load_weights uses the old
signature; update the assertion in test_model_loader_gms to expect the new
named-argument call by asserting checkpoint_loader.load_weights was called once
with "/ckpt", mapping=loader.mapping, model=model (i.e., include model=model as
the third keyword argument) so it matches the actual call used by the loader.
---
Nitpick comments:
In `@tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py`:
- Around line 1-15: Add an explicit note inside the module docstring of
MXCheckpointLoader tests
(tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py)
stating that QA integration/release test-list updates under
tests/integration/test_lists/qa/ are unnecessary for this PR because changes are
confined to unit tests; update the top-level docstring (the existing
triple-quoted description) to include a one-sentence statement like "QA list
updates under tests/integration/test_lists/qa/ are unnecessary for this PR
scope." so reviewers and release engineers see it immediately.
In `@tests/unittest/_torch/pyexecutor/test_model_loader_gms.py`:
- Around line 1-3: This PR only changes the unit test test_model_loader_gms.py
and therefore the QA scheduled list updates included in the diff are
unnecessary; remove or revert any modifications to the integration QA list files
that were accidentally included in this PR, ensure the commit/PR only contains
the unit test changes (keep changes in test_model_loader_gms.py), and update the
commit message/PR description to reflect that this is a unittest-only change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae8ec7b8-a885-4e7f-9ded-a81dd3fb72e5
📒 Files selected for processing (12)
tensorrt_llm/_torch/memory/__init__.pytensorrt_llm/_torch/memory/gpu_memory_backend.pytensorrt_llm/_torch/models/checkpoints/mx/checkpoint_loader.pytensorrt_llm/_torch/pyexecutor/model_engine.pytensorrt_llm/_torch/pyexecutor/model_loader.pytensorrt_llm/_torch/pyexecutor/py_executor.pytensorrt_llm/llmapi/llm_args.pytests/unittest/_torch/memory/test_gms_backend.pytests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pytests/unittest/_torch/pyexecutor/test_model_loader_gms.pytests/unittest/api_stability/references/llm.yamltests/unittest/llmapi/test_gms_args.py
|
PR_Github #49798 [ run ] completed with state
|
…and fix lint issues Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #49972 [ run ] triggered by Bot. Commit: |
|
PR_Github #49972 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #50020 [ run ] triggered by Bot. Commit: |
|
PR_Github #50020 [ run ] completed with state |
|
There are a few CodeRabbit comments that seems to raise valid issues, can you address after the request cancellation work. |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #50856 [ run ] triggered by Bot. Commit: |
brb-nv
left a comment
There was a problem hiding this comment.
Changes under tensorrt_llm/_torch/models/checkpoints/mx/checkpoint_loader.py look good to me.
|
PR_Github #50856 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51092 [ run ] triggered by Bot. Commit: |
|
PR_Github #51092 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #51156 [ run ] triggered by Bot. Commit: |
|
PR_Github #51156 [ run ] completed with state |
Sync the in-tree design doc with the standalone version kept offline
(2026-05-31 last update). Substantive additions / corrections:
- Foundation references section: split into merged (TRTLLM-11851, -12440,
end-to-end prototype) and inflight (TRTLLM-13077 prep PR, MX-team
Delegate-to-ModelExpress refactor proposal) sub-sections.
- Audience and intent section: name the two intended readers (MX/GMS
upstream engineers + TRT-LLM code owners) and what each needs to take
away.
- P1 precondition reframed: source-identity matching is fundamentally a
TRT-LLM concern (only TRT-LLM knows which knobs affect layout), so the
API surface lives in TRT-LLM as `tllm.disagg.compute_source_identity()`
/ `is_source_compatible()` and is consumed by both MX and GMS. The
opaque-bytes design protects transport libraries from churn when
TRT-LLM adds new layout-affecting parameters.
- Wave 4 scope updated to land the TRT-LLM source-identity API (~80 LOC)
as a foundational sub-step, used by both MX and GMS receiver paths.
- New "Coordination with MX and GMS" section:
- Source-identity API directionality (TRT-LLM-owned, MX/GMS-consumed).
- Scope of MX checkpoint loading: discusses the wholesale vs.
transport-only delegation question raised by the MX-team refactor
proposal, with TRT-LLM advocating transport-only (fallback /
validation / telemetry concerns are TRT-LLM-internal and parallel
the NIXL/UCX division-of-labor model in the disagg KV-cache
transceiver).
- What this asks of MX vs. what this asks of GMS as separate bullets.
- Status / Created / Last-updated header brought to 2026-05-31.
All PR-number cross-references (NVIDIA#13531, NVIDIA#13926, NVIDIA#13045, NVIDIA#14770, NVIDIA#14151)
deliberately omitted to keep this docs-and-plans branch from triggering
back-references on the public NVIDIA/TensorRT-LLM PRs; cited via JIRA
tickets and descriptive titles instead. Only external cross-ref
retained is ai-dynamo/dynamo PR NVIDIA#7053 (different repo/org).
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Summary by CodeRabbit
Release Notes
New Features
LoadFormat.GMSconfiguration option for GPU memory backend integration.Improvements
is_weights_preloaded()method to check weight preload status during model initialization.Tests
Summary
Adds
LoadFormat.GMSso multiple TRT-LLM instances on the same node can zero-copy share model weights via the GPU Memory Service (GMS) pool. The first instance loads weights as the writer (RW); subsequent peers materialize them read-only (RO) without disk I/O or per-instance copies.Scope
In scope:
LoadFormat.GMSenum value and nestedGmsConfig(socket_path,mode,tag) onTorchLlmArgs.GMSBackendadapter undertensorrt_llm/_torch/memory/, lazily imported only when GMS is selected.ModelLoaderGMS branch with explicit RW / RO / unexpected-state handling, plus a guard that refuses to commit an unpopulated model to the pool.PyTorchModelEngine.__del__.Adjacent (sub-PR, ~15 lines): remove the redundant
MXCheckpointLoader.p2p_succeededproperty;is_weights_preloaded()(the abstract hook fromBaseCheckpointLoader) is now the single accessor. Tests updated accordingly.Out of scope:
LoadFormat.GMS).gpu_memory_servicedependency. Same OSS-allowlist concern as MX; users install it manually for now.Test Coverage
New unit test files (mock-based, CPU CI):
tests/unittest/llmapi/test_gms_args.py—GmsConfigvalidation andLoadFormat.GMSPydantic surface.tests/unittest/_torch/memory/test_gms_backend.py—GMSBackendlifecycle and helpers.tests/unittest/_torch/pyexecutor/test_model_loader_gms.py—ModelLoaderGMS RW / RO / failure / edge-case branches.Updated:
tests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.py— six call sites switched tois_weights_preloaded()after removing the redundantp2p_succeededproperty.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)
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.