Fix weight sync selector for frozen speculative drafts - #1926
Merged
guapisolo merged 2 commits intoJul 30, 2026
Merged
Conversation
XinyuJiangCMU
requested review from
Shi-Dong,
Zhichenzzz,
fzyzcjy,
maocheng23,
yueming-yuan and
yushengsu-thu
as code owners
July 29, 2026 02:59
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
XinyuJiangCMU
force-pushed
the
pr/weight-update-selector-frozen-draft
branch
from
July 29, 2026 03:00
339b141 to
bb6c97d
Compare
yueming-yuan
approved these changes
Jul 29, 2026
…ate session In raw conversion mode a trainer can enable speculative decoding without training any MTP layers. The draft is then frozen and never receives our weights, but weight sync still opens every SGLang update session with the default selector "all", so the draft is restored in begin_weight_update and finalized again in end_weight_update on every training step. On ROCm, finalization applies the aiter weight pre-shuffle, which is not idempotent. Applying it a second time changes a weight that is already in the shuffled layout, and the draft then reads the wrong permutation. The job does not crash; spec_accept_length drops from about 2.71 to 1.00. Add weight_update_selector(args), returning "target" only when speculative decoding is enabled, mtp_num_layers is missing or zero, and the conversion path is not bridge mode. Bridge derives the block count from the HF config, so an unset count proves nothing there. Pass the same selector to begin_weight_update and to the tensor and distributed weight payloads so the session and the payloads always target the same runners. Co-authored-by: Zhiyao Jiang <jessicajiang324@gmail.com>
guapisolo
force-pushed
the
pr/weight-update-selector-frozen-draft
branch
from
July 29, 2026 22:21
fdf7d17 to
aabbf77
Compare
XinyuJiangCMU
added a commit
to XinyuJiangCMU/miles
that referenced
this pull request
Aug 1, 2026
Brings in 107 upstream commits, including the merged versions of the weight sync selector (radixark#1926), the DSv4 rollout knob cleanup and MTP recipe (radixark#1733), and the JIT norm pin (radixark#2040), all of which started here. Conflict resolution: - update_weight/common.py, update_weight_from_tensor.py, sglang_engine.py: take upstream. These conflicts were only placement, import wrapping and a docstring; upstream also carries review changes the local copies predate (the unmarked grouped-expert gather helper, tighter partition_dim checks, the check_equal parameter). All three files now match upstream exactly. - scripts/amd/run_deepseek_v4.py: keep enable_eval=False and the multinode parallel config, take upstream's extra_env_vars verbatim. Upstream's dict is the reviewed end state of radixark#1733 and radixark#2040: SGLANG_OPT_USE_COMPRESSOR_V2 is gone and SGLANG_OPT_USE_JIT_NORM=false is in. Keep the cuda-graph note, which has no upstream counterpart. Neither side sets --disable-cuda-graph, so cuda-graph stays on by default. Co-authored-by: Zhiyao Jiang <jessicajiang324@gmail.com>
guapisolo
pushed a commit
that referenced
this pull request
Aug 3, 2026
…t side (megatron) Port of slime #1926 adapted to miles: - new miles/utils/dp_schedule.py::build_dp_schedule — pure, CPU-testable - first_fit_pack / expand_bins_by_splitting in seqlen_balancing - rollout manager precomputes the schedule when the training backend advertises a full parallel config (megatron, non-indep_dp); shards carry num_microbatches + micro_batch_indices - get_data_iterator consumes the precomputed schedule (no all_reduce) - legacy train-side path kept for: multi-LoRA, fsdp/torchtitan, indep_dp/FT (delay_split), multimodal (media-token expansion changes lengths), non-divisible sample counts
Zhichenzzz
added a commit
that referenced
this pull request
Aug 4, 2026
…t side (megatron) Port of slime #1926 adapted to miles: - new miles/utils/dp_schedule.py::build_dp_schedule — pure, CPU-testable - first_fit_pack / expand_bins_by_splitting in seqlen_balancing - rollout manager precomputes the schedule when the training backend advertises a full parallel config (megatron, non-indep_dp); shards carry num_microbatches + micro_batch_indices - get_data_iterator consumes the precomputed schedule (no all_reduce) - legacy train-side path kept for: multi-LoRA, fsdp/torchtitan, indep_dp/FT (delay_split), multimodal, non-divisible sample counts
Zhichenzzz
added a commit
that referenced
this pull request
Aug 4, 2026
…t side (megatron) Port of slime #1926 adapted to miles: - new miles/utils/dp_schedule.py::build_dp_schedule — pure, CPU-testable - first_fit_pack / expand_bins_by_splitting in seqlen_balancing - rollout manager precomputes the schedule when the training backend advertises a full parallel config (megatron, non-indep_dp); shards carry num_microbatches + micro_batch_indices - get_data_iterator consumes the precomputed schedule (no all_reduce) - legacy train-side path kept for: multi-LoRA, fsdp/torchtitan, indep_dp/FT (delay_split), multimodal, non-divisible sample counts
Zhichenzzz
added a commit
that referenced
this pull request
Aug 4, 2026
…t side (megatron) Port of slime #1926 adapted to miles: - new miles/utils/dp_schedule.py::build_dp_schedule — pure, CPU-testable - first_fit_pack / expand_bins_by_splitting in seqlen_balancing - rollout manager precomputes the schedule when the training backend advertises a full parallel config (megatron, non-indep_dp); shards carry num_microbatches + micro_batch_indices - get_data_iterator consumes the precomputed schedule (no all_reduce) - legacy train-side path kept for: multi-LoRA, fsdp/torchtitan, indep_dp/FT (delay_split), multimodal, non-divisible sample counts
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.
Co-authored-with: @JessicaJiang-123
The problem
In raw conversion mode, a trainer can enable speculative decoding without training MTP layers. In that configuration, the target model changes during training, but the speculative draft is frozen and should not receive weight updates.
Before this change, Miles did not tell SGLang which model runners a weight update covered. Both the update session and the weight payloads therefore used SGLang's default selector,
"all", which includes the target and the draft.That default is wrong for a frozen draft. On every training step, SGLang still includes the draft in the weight update. The draft loads none of the incoming tensors because its loader filters out every name, but SGLang still runs the post-update processing on it.
The failure is silent. The job does not crash, no assertion fires, and the training loss can still look normal because the target model is updated correctly. The visible symptom is on the rollout side:
spec_accept_lengthdrops from about 2.71 to 1.00, so speculative decoding stops accepting useful draft tokens.How the draft gets corrupted
Before this change, every weight update also processed the frozen draft, even though the draft did not receive any new weights.
The target receives fresh, unshuffled weights, so applying the AITER shuffle is correct. The draft keeps its old weights, which have already been shuffled.
SGLang therefore applies the shuffle to the draft a second time. Since the shuffle is not idempotent, this produces a different weight permutation, and the inference kernel then reads the weights in the wrong layout. As a result, the draft generates incorrect predictions without raising an error.
A minimal reproduction is shown below:
Changes
weight_update_selector(args)as the single place that decides which SGLang runners a weight sync covers."target"only when the trainer can prove that the draft is frozen:mtp_num_layersis missing or zero;begin_weight_updateand all weight payloads, so the update session and the actual weight loading cover the same runners.Verification
The end-to-end test used 4 nodes, DeepSeek-V4-Flash FP8 on gfx950, colocated rollout, and a 16k response length.
The two weight-sync runs used the same configuration and differed only by this fix. A run with the draft kept frozen was used as the reference.
spec_accept_lengthWithout the selector, the update session corrupts the draft and accepted length collapses to 1.00. With the selector, accepted length returns to the frozen-draft reference range.
The fixed run also recovers the expected rollout benefit: rollout time is about 320 seconds with MTP off and about 210 seconds with MTP on, roughly 34 percent lower.