[Bugfix] Restore DSpark draft param views after TMS resume - #406
[Bugfix] Restore DSpark draft param views after TMS resume#406CalvinXKY wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces synchronization of DSpark draft model parameters after the optimizer step in vime/backends/megatron_utils/model.py to fix broken view relationships, and removes the normalization argument from the model provider in vime/backends/megatron_utils/model_provider.py. Feedback on the changes points out potential TypeError issues when unpacking ParamIndexSpec and param_to_index entries due to differences in Megatron-LM versions, and suggests a robust fallback to handle both dataclass attributes and slice objects.
| for param_obj, (_start, _end, bucket_id) in pim.items(): | ||
| if id(param_obj) not in draft_param_ids: | ||
| continue | ||
| bucket = buffer.buckets[bucket_id] | ||
| if hasattr(bucket, "param_to_index") and param_obj in bucket.param_to_index: | ||
| local_start, local_end = bucket.param_to_index[param_obj] | ||
| if isinstance(local_start, int): | ||
| view = bucket.param_data.view(-1)[local_start:local_end].view(param_obj.data.shape) | ||
| param_obj.data.copy_(view) |
There was a problem hiding this comment.
Unpacking spec directly as (_start, _end, bucket_id) can cause a TypeError because ParamIndexSpec in standard Megatron-LM is a standard Python dataclass and is not iterable by default. Additionally, bucket.param_to_index[param_obj] can be a slice object rather than a tuple in some Megatron-LM/NeMo versions, which would also raise a TypeError when unpacked directly as local_start, local_end.
To ensure compatibility and robustness across different Megatron-LM versions, we should check if spec has a bucket_id attribute and handle entry as either a slice or a tuple/list.
| for param_obj, (_start, _end, bucket_id) in pim.items(): | |
| if id(param_obj) not in draft_param_ids: | |
| continue | |
| bucket = buffer.buckets[bucket_id] | |
| if hasattr(bucket, "param_to_index") and param_obj in bucket.param_to_index: | |
| local_start, local_end = bucket.param_to_index[param_obj] | |
| if isinstance(local_start, int): | |
| view = bucket.param_data.view(-1)[local_start:local_end].view(param_obj.data.shape) | |
| param_obj.data.copy_(view) | |
| for param_obj, spec in pim.items(): | |
| if id(param_obj) not in draft_param_ids: | |
| continue | |
| bucket_id = spec.bucket_id if hasattr(spec, "bucket_id") else spec[2] | |
| bucket = buffer.buckets[bucket_id] | |
| if hasattr(bucket, "param_to_index") and param_obj in bucket.param_to_index: | |
| entry = bucket.param_to_index[param_obj] | |
| if isinstance(entry, slice): | |
| local_start, local_end = entry.start, entry.stop | |
| else: | |
| local_start, local_end = entry[0], entry[1] | |
| if isinstance(local_start, int): | |
| view = bucket.param_data.view(-1)[local_start:local_end].view(param_obj.data.shape) | |
| param_obj.data.copy_(view) |
d5a2fc5 to
b37f002
Compare
b37f002 to
a58a2e4
Compare
|
Thanks for tracking this down. I think the current patch fixes the symptom, but the synchronization is placed too late and on a very hot path. The root problem described here is that
A more fundamental and smaller fix would be to restore the view once, immediately after TMS resume: torch_memory_saver.resume()
if self.args.dspark_enabled:
restore_dspark_param_views(self.model)The helper should live with the DSpark model integration (for example Conceptually: def restore_dspark_param_views(model):
for chunk in model:
draft_params = set(unwrap_model(chunk).draft_model.parameters())
for buffer in chunk.buffers:
for param, spec in buffer.param_index_map.items():
if param in draft_params:
param.data = buffer.param_data[
spec.start : spec.end
].view_as(param)The exact The normalization correction looks right. Using I would also add a focused regression test instead of relying only on the 100-step convergence run:
That directly tests the reported failure mode and would make the fix both smaller and more first-principled. |
a58a2e4 to
c82a0c4
Compare
a839773 to
056da57
Compare
After torch_memory_saver resume, DSpark draft params lose their views into the DDP buffer, so rebind them before training continues. Signed-off-by: kaiyuan <kyxiezju@163.com>
c0b47b1 to
934a91f
Compare
IPC engine weight sync sends only updated weights via load_weights, leaving some draft attention params on meta device. _build_context_kv_buffers then crashes on torch.cat with mixed CUDA/meta tensors. Apply runtime monkeypatch based on vllm-project/vllm#55076: skip rebuild when previous CUDA buffers exist (incremental IPC update), raise on first-load failures, recompute RoPE cos_sin_cache from meta. Remove DSpark smoke test and CI assertions (blocked by TE sm80 incompatibility on A800; will re-add after vLLM upstream fix lands). Signed-off-by: CalvinXKY <kyxiezju@163.com>
934a91f to
0d6921d
Compare


Summary
8fbfd891): Fixes param view corruption when TorchMemorySaver pause/resume interacts with DSpark draft model weight sync.934a91ff): Runtime monkeypatch for vllm-project/vllm#55076. IPC engine weight sync leaves some draft attention params on meta device;_build_context_kv_buffersthen crashes ontorch.catwith mixed CUDA/meta tensors. The patch skips rebuild when previous CUDA buffers exist (incremental IPC update), raises on first-load failures, and recomputes RoPEcos_sin_cachefrom meta.Dependencies
What was removed
tests/test_qwen3_4B_dspark_short.py) — blocked by TE 2.16.1 sm80 incompatibility on A800; will re-add after vLLM upstream fix landsspec_accept_rateintrain_metric_utils.pyTest Plan