[Reshard] Add model-weight Transfer Engine adapters - #4293
Bo-Vincent wants to merge 23 commits into
Conversation
zxpdemonio
left a comment
There was a problem hiding this comment.
Thanks for the thorough lifecycle work and test coverage. I found several issues that should be addressed before merging:
-
Acquisition rollback can lose allocation pins. In
weight/_lifetime.py, if validation of a later acquired binding fails and one token'srelease_after_terminal()raises,_release_tokens()stops immediately. Earlier tokens are neither released nor quarantined, and the caller never receives them. Constructing the temporaryAllocationTokenSetcan also fail on duplicate token IDs before cleanup starts. Please make rollback best-effort and recoverable, preserving every unresolved token in quarantine, and add a multi-token release-failure test. -
Unregistration progress is not interruption-safe. In both normal registration cleanup and
PendingTransferManager.drain_pending_transfer(), progress is only recorded after the unregister loop. An interruption after one successfulunregister_memory()return can leave remaining registrations live while the outer adapter releases allocation tokens; the pending path can also retry an address that was already successfully unregistered. Please track an unresolved-address set, checkpoint each successful unregister immediately, and catchBaseExceptionaround the complete cleanup loop. -
Completed physical I/O can be reported as
ABORTED. Reader and sink only switch toFAILED_DRAINEDforTransferCompletionFailedError. If a native batch completes and an interruption occurs before the adapter returns, all tokens are finalized withABORTEDeven though target memory was modified. Please track whether physical I/O has started/completed and classify subsequent non-pending failures asFAILED_DRAINED, as the Store paths already do. -
Per-participant execution acquires the global peer set. A source that only writes a subset of targets still requires guards and executor bindings for every target in the plan; the reader has the symmetric source-side issue. For example, source 0 in TP2-to-TP4 only touches target 0/1, but execution fails if the unrelated target 2 guard is absent. Please derive required participants/fragments from the selected executor's operation indices and validate completeness against that subset.
-
The current execution loop serializes every batch and endpoint.
execute_batch()waits for a terminal ticket before the next batch or peer is submitted, leaving only one transfer in flight. This prevents multi-peer/NIC aggregation from approaching line rate. Please introduce a bounded in-flight window while retaining each ticket's registrations and lifetime resources until terminal completion. -
The weight adapter tests do not exercise the production scatter-ticket path. The model-weight fake engine primarily exposes the flat compatibility API, whereas the real binding selects
scatter_transfer_sync_*_with_ticket. Please add reader/sink tests covering lowering through a scatter-capable engine (bases, capacities, nested offsets, direction mapping and completion), plus a native CPU/GPU or RDMA smoke test.
I reproduced the first three lifecycle failures locally. The PR-scoped TE tests pass (173 passed), Pyright reports no errors, and git diff --check passes, but these failure paths are not covered by the current suite.
053271a to
7f6e502
Compare
Thanks for the detailed review. I addressed issues 1–4 and 6.
Rollback now uses the executor’s recoverable resource finalizer for every acquired token. A release failure retains the unresolved token in pending quarantine, while the remaining tokens are still finalized. Duplicate token IDs can no longer prevent cleanup from starting. Multi-token release-failure and duplicate-ID regression tests were added.
Registration cleanup now tracks an unresolved-address set and checkpoints every successful unregister immediately. Both normal cleanup and pending-transfer draining catch
Reader and sink now derive required peers and fragments from the selected local executor’s operation indices. Binding, guard acquisition, and executor-completeness validation are limited to that operation scope, so unrelated plan participants are no longer required. Both target-initiated and source-initiated subset cases are covered.
I confirmed that the current loop keeps one transfer in flight. I have not implemented concurrency in this PR because a safe bounded window requires a native non-draining submission API, composite pending-ticket recovery, and per-batch registration and lease ownership. Wrapping the synchronous API in a thread pool would weaken the current fail-closed lifecycle. I propose handling this as a focused performance follow-up while keeping this PR’s execution contract synchronous and recoverable.
Reader and sink tests now exercise the production-shaped Validation on the exact head:
|
zxpdemonio
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked exact head 7f6e502c68d0ec445535fbd20413228f072aedd9. The rollback, interruption-safe cleanup, post-I/O terminal-state handling, and scatter-ticket coverage from items 1–3 and 6 now look addressed. I still found two correctness blockers in the operation-scoping change:
-
The peer scope expands back to every worker executor in the same participant. The new preflight correctly derives
source_requirements/target_requirementsfrom the local operation indices, but_execute_reserved()then callsresolve_runtime_executors()for each peer binding. That resolver returns all executor snapshots for the(instance_id, participant_id), not just the operation-scoped workers. Reader then compares that expanded set withexpected_source_executorsatreader.py:289-320; sink has the symmetric check atsink.py:280-311.I reproduced this with one valid
WeightRuntimeBindingManifestcontaining two fragments assigned to two worker IDs under the same participant. When local target 0 touches only source worker 0, reader fails withTransferEngineError: source executor set is incomplete; swapping the topology makes sink fail withtarget executor set is incomplete. No unrelated participant is present—the extra executor is only another worker in the same participant. Please resolve/revalidate the fresh binding against the selected executor/fragment scope rather than resolving the complete participant, and add reader/sink regressions where one participant contains multiple worker groups but the local operation subset uses only one. -
An explicit unknown worker selector now silently succeeds with no work. After filtering by
target_worker_id/source_worker_id,reader.py:125-132andsink.py:122-129return()when nothing matches. A typo, an empty string, or a non-string selector therefore looks like a successful zero-byte transfer. The Store adapters already reject invalid selectors and unknown workers, and the reserved TE paths still require exactly one selected local executor. Please preserve that fail-closed contract: only return empty when the binding itself is outside the plan (if that is intentional), but reject an explicitly supplied selector that does not identify a planned worker. Add reader/sink tests for invalid selector type/value and unknown worker ID.
The bounded in-flight item remains unresolved. I agree that a thread-pool wrapper over the synchronous API would weaken the lifecycle contract; please at least link a concrete native-async follow-up and document that this version serializes all endpoint/batch submissions.
Validation on this head: the focused TE/weight-adapter suite passes (186 passed), Pyright reports 0 errors, and git diff --check passes. The full reshard suite reached 799 passed, 3 skipped, 2 subtests passed; its only failure on this host is an unrelated installed native mooncake.store that does not expose begin_weight_snapshot.
7f6e502 to
8e9109d
Compare
Thanks for the follow-up review. Both operation-scoping issues have been fixed on the current head (
The focused regression suite passes ( Bounded in-flight execution is being handled as a separate performance follow-up because it requires native non-draining submission and recoverable multi-ticket lifetime management. |
Signed-off-by: Teng Ma <stmatengss@gmail.com>
Description
This PR adds model-weight adapters that execute validated reshard plans through
Mooncake Transfer Engine.
The adapters consume an attested
TransferPlan, fresh runtime bindingmanifests, and framework-provided allocation guards. They validate the selected
runtime fragments, lower compact N-D transfer regions into bounded TE batches,
and execute either target-initiated reads or source-initiated writes. The
resource-neutral TE executor remains independent of model names and framework
tensor layouts.
The execution lifecycle is fail-closed. GPU allocations and memory
registrations remain pinned until completion reaches a known terminal state.
Unknown completion or cleanup outcomes are retained by the pending-transfer
manager, fence further submissions on the same engine, and expose a pending ID
for status and recovery handling. Reader and sink paths share the same
completion, registration, allocation-lifetime, and operation-budget contracts.
This PR also documents the execution boundary and public APIs. Store
persistence, framework runtime discovery, model-specific conversion, and
quantization or packed-layout transformation remain separate layers.
Related RFC: #3111
Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
PYTHONPATH=mooncake-reshard/python \ python -m pytest -q mooncake-reshard/tests python -m pyright --project mooncake-reshard/pyrightconfig.json ./scripts/code_format.sh --changed-lines --check -b origin/main pre-commit run --files $(git diff --name-only origin/main...HEAD) make -C docs clean html git diff --check origin/main...HEADThe 22 commits in this PR were also checked individually by running the full
mooncake-reshard/testssuite at each commit.Test results:
Exact-head results:
786 passed, 4 skipped, 2 subtests passed0 errors, 0 warnings, 0 informationsand
git diff --checkpassedregistration cleanup, allocation-token release failure, duplicate token IDs,
pending ownership, restart-required fencing, and reader/sink symmetry
Native RDMA/CUDA execution and process-restart recovery were not rerun on the
final exact head.
Checklist
./scripts/code_format.shAI Assistance Disclosure
AI tools assisted with implementation, test generation, fault-injection review,
and documentation. The changes were independently reviewed and validated on the
exact PR head; the human submitter remains responsible for the design and code.