[DO NOT review, pending K3 text CP side merging] [Kimi K3] Add K3 MoonViT Dynamic CP support - #4380
Draft
QIU023 wants to merge 14 commits into
Draft
[DO NOT review, pending K3 text CP side merging] [Kimi K3] Add K3 MoonViT Dynamic CP support#4380QIU023 wants to merge 14 commits into
QIU023 wants to merge 14 commits into
Conversation
KCP on the KDA layers, Ulysses on the MLA layers, on the plain CP group. Text-only slice: no vision dynamic CP, no vision-tower CP attention.
…gration cell The contract test sat in torchtitan/models/kimi_k3/tests/, the only in-model tests directory in the tree; the convention is tests/unit_tests/. Moved and renamed to carry the model name. Adds a cp2 recipe and the integration cell that runs it, so context parallel is covered by CI rather than only by a local matrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WBy1d9YVu44nYCVqykRqL1
The CPU unit-test workflow collects tests/unit_tests/cpu, not the top level, so the contract test would have sat there un-run; move it. The cp2 cell declares use_real_pg because the thing it covers is the all-to-all and the KCP state pass; under Fake PG the collectives are no-ops and the cell would pass without exercising either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WBy1d9YVu44nYCVqykRqL1
…ield Review round on the CP change, four structural points. The core accommodation becomes a protected method instead of a config field. Decoder.Config._validate_cp_backend holds the spmd_types check and a model whose CP is not ShardingConfig-driven overrides it -- no field appears on every other model's config surface, and when this model's kernels move to a torch-native KDA and its CP goes declarative, the override is deleted rather than a core field deprecated. Tensor parallel is not on this PR, so everything that existed only for the TP interaction goes: dtensor_ops.py whole (both helpers guard DTensor inputs that cannot occur without TP), the kernel-call unwraps and re-wrap in kda, the k_rope Partial-gradient boundary in the MLA CP path, and the DTensor unwraps in conv_with_halo. KDA's alternate Ulysses mode goes with its cp_mode enum: under CP the KDA layers run KCP, full stop, which is also all the PR body ever described. MLA's Ulysses stays, as the method on the attention class that orchestrates its projections; the reusable transport already lives in sharding.py. kcp.py's two helpers move into kda.py next to their only caller, with the debugging narratives cut to what they established. The head-divisibility check inlines into apply_cp_kimi_k3, which shrinks to one loop.
…override carries its import The attention keeps the branch and the call; the exchange itself -- two all-to-alls with the attention backend between them, and the rotary slice gathered outside them -- lives next to the contract that describes it. The causal mask rebuild moves with it. _validate_cp_backend called validate_cp_backend as a bare name, but that name was only bound by a function-local import in update_from_config, so a method body resolving to module globals would not find it: any run with context_parallel_degree > 1 raised NameError before reaching the check. The method now carries the import, and the dead local one is gone. Also: the module docstring no longer claims a contract is resolved per module, the error text stops naming a config field that no longer exists, and the text flavor derives from the debug flavor. Configuration-identical, 4810 and 4678 fields over the union of both key sets.
…t parallel
Upstream ships one kimi_k3 flavor and it is multimodal. Turning context
parallelism on with it raises upstream's own alignment check:
torchtitan/models/common/multimodal.py:72, in get_vision_positions
ValueError: Multimodal misalignment: found 0 contiguous run(s) of
placeholder id 2016 in the token sequence but received 1 visual item(s).
prepare_context_parallel_input shards inputs, labels and positions along the
sequence but leaves pixel_values whole, so every rank encodes every image while
holding only a slice of the placeholders -- and a slice may split a visual item
or contain none at all. get_vision_positions needs whole items, so it refuses.
Each rank now scatters the feature slice its own placeholders correspond to. The
features are ordered by sequence position and CP shards are contiguous and equal
-- the config already rejects a load balancer under CP, because a permuting one
would break exactly that -- so a rank's slice starts after however many
placeholders the lower ranks hold, which one all-reduce establishes.
The rows a rank does not consume still reach the graph through
add_zero_valued_dependency: FSDP2 issues the tower's reduce-scatter from the
autograd hooks on its output, so leaving them out would have a subset of the
process group issue the collective.
This is correctness, not the report's sec 5.2.3 vision parallelism: the encoder
still runs redundantly on every CP rank. Splitting the tower itself belongs to
the DEP and dynamic-CP work, and nothing here anticipates it.
Verified on the multimodal debug flavor at cp2, two steps, exit 0. Before this
the same command dies in the check quoted above.
…avor goes Same review direction as the EP PR: no text-only flavor. With the vision splice aligned to the sequence shard (previous commit), everything here runs on the flavor upstream ships: the cp2 CI cell and its recipe move to kimi_k3_debugmodel, and the flavor plus its model-registry entry leave the branch. Measured on this tip before the switch: dp1/cp2/cp4/cp8/dp2/ fsdp2_cp2/fsdp2_cp4 all train from one seed (mx3_cp_mm_0826_182604), with cp2/cp4/cp8 at 1.30e-2/1.24e-2/8.78e-3 from dp1 against 2.47e-2 for dp2 measured the same way.
The branch's only remaining delta here was a trailing blank line, which pre-commit would re-normalize on the next touch anyway -- as it did on the EP branch. Restoring the upstream bytes takes the file out of the diff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WBy1d9YVu44nYCVqykRqL1
…ndaries The CP path rebuilt a causal-only mask for the reassembled sequence and used the context window to reject streams that might hold several documents. After the all-to-all every rank holds the full sequence, so the global packed-document mask applies as-is: gather the contiguous positions shards (no load balancer under CP) and build the same causal x document mask the non-CP path uses. The window guard, its config plumbing and the shape-keyed mask cache go away -- the mask follows the data now. A two-rank gloo test packs three documents with one boundary on the shard cut and one inside a shard; the gathered mask equals the mask built from the global positions, and both boundary attentions are refused where a causal-only mask lets them through.
…-KV, sub-CP groups
QIU023
requested review from
fegin,
tianyu-l,
wconstab and
wwwjn
as code owners
August 30, 2026 06:40
QIU023
marked this pull request as draft
August 30, 2026 06:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft, stacked on the text-side CP PR: the diff tab shows that PR's content too, so review only the last commit (
kimi_k3: dynamic CP for the vision encoder). It will be rebased when the text PR lands.Summary
Report sec 5.2.3, both halves: a single large image is partitioned along the PATCH dimension across the ranks of a sub-CP group, with attention gathering k/v across the group (gather-KV), and each CP group is divided into sub-CP groups with the large images distributed across them so the communication fraction does not grow with the group. Images below the partition threshold, or whose grid height does not divide the merge kernel, stay whole and are encoded replicated, which is exactly the text PR's behavior for every image.
Design
vit_cp_plan.pyis pure planning -- no collectives, no tensors in signatures -- so the scheduling decisions (row_partition,subgroup_layout,balance_images,classify) are testable without spawning ranks.khgrid rows), and a video is cut as "rows r0..r1 of EVERY frame" because the projector's temporal mean spans all frames.cp_size:new_groupmust be called by every rank with the same lists in the same order, so a per-batch call is exactly the mismatch that hangs._PlainGradBoundarykeeps the tower plain in both directions: the gather's transpose is a reduce-scatter with no DTensor sharding strategy, and neitherto_local()norgrad_placementscan say "do not re-wrap".context_parallel_degree > 1and the batch holds an image at or abovedynamic_cp_min_patches(default 256); the debug dataset's images reach that threshold, so the debug matrix exercises it as-is.VisionAttention.forwardandVisionTransformerBlock.forwardgain a pass-throughcp_planargument (ignored in common, consumed by the subclass), because activation checkpointing recomputes forwards from saved arguments and module state set around the call is gone by recompute time.Results
Draft placeholder: the tables below were measured on the integration tree this commit is extracted from (trees
2f9dd3098and77a298ac5, 8x RTX 5060 Ti, seed 42,--debug.deterministic, steps 1/3/10 protocol), not on this branch; they will be re-measured on this branch before the draft is undrafted.Sequence 512, one large image per stream: cp2 differs from dp1 at step 2 by 2.62e-4 and cp4 by 3.52e-3; sequence 1024 taken to cp8: 9.47e-3 / 9.46e-3 / 2.22e-3 for cp2/cp4/cp8, no trend with degree. Every CP cell logs its actual partition ("N large image(s) over M sub-CP group(s)"), so an inert path fails loudly rather than producing a plausible table.
Changed files
CI/CD Coverage
None added in this draft; planned before undrafting: CPU unit tests for the pure planners (
row_partitionband invariants,subgroup_layout, the non-prefix key mask), which need no ranks.