[None][refactor] Delegate MX checkpoint loading to ModelExpress#14151
[None][refactor] Delegate MX checkpoint loading to ModelExpress#14151zhengluo-nv wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR migrates the in-tree MX checkpoint loader to an external ChangesModelExpress Integration Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/pyexecutor/test_model_loader_mx.py (1)
92-169: QA list updates are unnecessary for this PR scope.These changes are unit-test-only and do not add or modify integration test definitions, so no update to
tests/integration/test_lists/qa/*is needed in this PR.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/pyexecutor/test_model_loader_mx.py` around lines 92 - 169, The reviewer notes QA list updates are unnecessary because these changes are unit-test-only; update the PR description (or add a brief comment in the PR/commit message) to explicitly state that QA list updates are unnecessary per the guideline, referencing that this PR only touches unit tests such as test_mx_success_initializes_mapper_skips_weight_mapping_and_reload_works, test_mx_partial_fallback_merges_returned_weights, and test_mx_fallback_runs_standard_weight_mapping so maintainers know no changes to tests/integration/test_lists/qa/* are required.
🤖 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.
Nitpick comments:
In `@tests/unittest/_torch/pyexecutor/test_model_loader_mx.py`:
- Around line 92-169: The reviewer notes QA list updates are unnecessary because
these changes are unit-test-only; update the PR description (or add a brief
comment in the PR/commit message) to explicitly state that QA list updates are
unnecessary per the guideline, referencing that this PR only touches unit tests
such as
test_mx_success_initializes_mapper_skips_weight_mapping_and_reload_works,
test_mx_partial_fallback_merges_returned_weights, and
test_mx_fallback_runs_standard_weight_mapping so maintainers know no changes to
tests/integration/test_lists/qa/* are required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6c2df948-8a9c-4bbd-aeec-274b73e739a8
📒 Files selected for processing (5)
tensorrt_llm/_torch/models/checkpoints/mx/checkpoint_loader.pytensorrt_llm/_torch/pyexecutor/model_loader.pytensorrt_llm/llmapi/llm_args.pytests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pytests/unittest/_torch/pyexecutor/test_model_loader_mx.py
chienchunhung
left a comment
There was a problem hiding this comment.
A couple of things to flag initially:
- In TRTLLM we would like to preserve the existing fallback
class MXCheckpointLoader(HfCheckpointLoader): if MX checkpoint loading fails, we can still fallback to the default Hf checkpoint loader. The better approach here is MX defines an interface that downstream (e.g., TRTLLM and other frameworks) can inherit and implement custom logic. This allows upstream to unify the core logic, while the downstream can still customize instead of running as blackbox. PS: I think that's what the offline discussion concluded. - In PR#13531, the writer publishes the weights first, then both reader and writer transform. While in this PR the order is reversed: the writer transforms the weights first before publishing. Wondering if there is any reason why the order is reversed?
|
|
/bot run --disable-fail-fast |
|
PR_Github #48630 [ run ] triggered by Bot. Commit: |
|
PR_Github #48630 [ run ] completed with state |
|
Wanted to raise a few concerns around: what need to be hidden in the upstream vs. what can be customized in TRTLLM. The "publish before or after weight loading" ordering is exactly one critical example. Imagine a production environment with:
IIUC B's 8 ranks each find rank-matched A donors, pull A's post-transform weights (laid out for TRTLLM + cutlass). B skips its own post_load_weights (assuming the skip will be in place which seems not currently). B is now running with A's backend choices even though it's configured for FlashInfer + cutedsl. Either B silently does the wrong thing, or some hidden mismatch detection has to fail the boot. Another example is EP. If The receiver's pipeline becomes: receiver:
receive(post-transform bytes from MX)
# SKIP module.post_load_weights according to this PR
rebuild_aliases(model) # but alias rebuilding is still required
if isinstance(moe_load_balancer, MoeLoadBalancer):
moe_load_balancer.finalize_model() # AND MoE finalize is also required
torch.cuda.current_stream().synchronize() It seems like the current mental model assumes "universally homogeneous" deployment configuration, which is generally common in practice. However TRTLLM needs to cover all possible setups and, even if not supported, fails out gracefully rather than silently failing. Overall, I think there are multiple edge cases (attn_backend, quant backend list, FP8 scale fusion strategy, future quant schemes, etc) that make "transform-once-then-publish" hard to adapt. Either MX guarantees to incorporate all sources of customization into identity building / matching (which is unbounded), or the safer approach is to allow downstream to handle these customization. PS: Even if we later decide to go the publish-post-transform route, it requires factoring A couple of suggestions for what can be abstracted in MX:
# MX provides:
chain = LoadStrategyChain([
RdmaStrategy(mx_client=...),
GdsStrategy(...),
ModelStreamerStrategy(...),
])
# Engine uses (this lives in tensorrt_llm or vllm):
class MXCheckpointLoader(HfCheckpointLoader): # ← engine owns this class
def load_weights(self, checkpoint_dir, ...):
result = chain.try_load(model, identity, ...)
if result.success:
self._weights_preloaded = True
return result.fallback_weights # if partial
# Engine-owned fallback decision
return super().load_weights(checkpoint_dir, ...) # disk fallback
|
|
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>
976bc76 to
c8a810a
Compare
Signed-off-by: Zheng Luo <zheluo@nvidia.com>
c8a810a to
088aa36
Compare
| from tensorrt_llm._torch.models.modeling_utils import register_config_loader | ||
|
|
||
|
|
||
| @register_config_loader("MX") |
There was a problem hiding this comment.
Change from MX -> modelexpress to keep consistent branding across all inference engines. In vLLM and SGLang we use modelexpress because MX may confuse customers with MXFP4/MXFP8 format.
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>
Description
This PR narrows the TensorRT-LLM side of the ModelExpress checkpoint-loading integration.
TensorRT-LLM now stays responsible for lifecycle orchestration only:
modelexpresscheckpoint loader name;post_load_publish()after TRT-LLM finishes weight loading, post-load processing, and CUDA stream synchronization.The canonical user-facing checkpoint/load format is
modelexpress, notmx, to avoid confusion with MXFP4/MXFP8 formats. The in-tree shim keeps the existingMXCheckpointLoaderclass name only as an internal compatibility name for the ModelExpress-owned Python loader.The low-level ModelExpress behavior is delegated to the
modelexpressPython package instead of living in TensorRT-LLM. Source discovery, identity construction, RDMA transfer, fallback behavior, source publication, and metadata lifecycle are owned by ModelExpress.The post-load publish hook is intentionally ordered after TRT-LLM's post-load path. That prevents publishing tensor addresses/content before TRT-LLM has finished applying fallback weights or model-specific post-processing, while still allowing successful P2P receivers to become sources for later replicas.
Test Coverage
tensorrt_llm/_torch/models/checkpoints/mx/checkpoint_loader.pytensorrt_llm/_torch/models/checkpoints/auto_mapper.pytensorrt_llm/_torch/models/checkpoints/hf/config_loader.pytensorrt_llm/_torch/models/checkpoints/hf/weight_loader.pytensorrt_llm/_torch/models/checkpoints/hf/weight_mapper.pytensorrt_llm/_torch/pyexecutor/model_loader.pytensorrt_llm/llmapi/llm_args.pytests/unittest/_torch/models/checkpoints/mx/test_mx_checkpoint_loader.pytests/unittest/_torch/pyexecutor/test_model_loader_mx.pypost_load_publish()running after CUDA stream synchronization.tests/integration/test_lists/qa/*, while this PR only updates unit tests such astest_mx_success_initializes_mapper_skips_weight_mapping_and_reload_works,test_mx_partial_fallback_merges_returned_weights, andtest_mx_fallback_runs_standard_weight_mappingintests/unittest/_torch/pyexecutor/test_model_loader_mx.py.trtllm-servewithnvidia/Kimi-K2.5-NVFP4;/healthand/v1/models.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.