[data] fix: preserve sparse MIMO modality DP ownership#4949
Conversation
Signed-off-by: Yu Yao <yaoyu.094@gmail.com>
|
/ok to test c84347a |
|
Review — sparse MegatronMIMO modality DP slicing The change tracks per-sample ownership of modality inputs at collate time (_mimo_sample_indices) and uses it in slice_batch_for_megatron_mimo so compacted modality tensors follow their owning sample into the correct DP shard, instead of being naively split by dim 0. This fixes a real correctness bug where, e.g., only samples 2 and 3 carry vision inputs but a 2-way split would hand rank 0 (samples 0-1) the vision data. Logic looks correct and the new test exercises the core case well. Findings
Notes
Suggested test cases
No perf tests impacted. |
| if key == "modality_inputs" and modality_sample_indices is not None: | ||
| local_modalities = {} | ||
| for modality_name, modality_values in value.items(): | ||
| local_values = {} | ||
| field_indices = modality_sample_indices.get(modality_name, {}) | ||
| for field_name, field_value in modality_values.items(): | ||
| indices = field_indices.get(field_name) | ||
| if not isinstance(indices, torch.Tensor): | ||
| local_values[field_name] = field_value | ||
| continue | ||
| selected = (indices >= sample_start) & (indices < sample_end) | ||
| if isinstance(field_value, torch.Tensor): | ||
| local_values[field_name] = field_value[selected] | ||
| elif isinstance(field_value, list): | ||
| local_values[field_name] = [ | ||
| item for item, keep in zip(field_value, selected.tolist()) if keep | ||
| ] | ||
| else: | ||
| local_values[field_name] = field_value | ||
| if any( | ||
| not isinstance(field_value, (torch.Tensor, list)) or len(field_value) > 0 | ||
| for field_value in local_values.values() | ||
| ): | ||
| local_modalities[modality_name] = local_values | ||
| sliced[key] = local_modalities |
There was a problem hiding this comment.
When _mimo_sample_indices is present, this branch handles all of modality_inputs and the _is_patch_packed_visual_dict(value) / recursion branches below are never reached for it. A patch-packed encoder dict ({hidden_states, grid_thw}) nested under a modality would then fall into the final else: local_values[field_name] = field_value (it is a dict, not a Tensor/list) and be passed through unsliced — every DP shard would receive the full encoder input.
This only occurs when a patch-packed modality co-exists in a batch that also has a sparse modality (which sets the key). If that combination is reachable in real VLM SFT data, the joint patch-packed slicing should still apply here. Could you confirm whether these two paths can co-occur, and add coverage if so?
Problem
MegatronMIMO explicitly supports batches where a modality is absent from some
samples. The collator compacts modality inputs by dropping absent entries, but
module-local DP slicing previously split that compacted tensor independently
from the full sample batch.
With four samples whose vision presence is
[false, false, true, true]andvision/language DP 2, DP shard 0 owns the first two text-only samples but
received the vision tensor belonging to sample 2. Equal-DP MCore routing carries
no sample identity to repair this, so training fails with a modality-token versus
embedding-count mismatch. Sparse counts not divisible by DP failed earlier in
the slicer.
Fix
The MIMO collator now records private per-field sample indices only when a field
is sparse. Module-local slicing uses those indices to keep tensor and list values
with their owning sample shard, then removes the metadata before the model call.
All-present batches retain the existing fast path, and DP 1 preserves the
model-facing batch while consuming the private metadata.
Validation
A deterministic source-level reproducer using the real collator, DP slicer, and
pinned MCore was run unchanged before and after the production fix:
Adjacent contract checks covered all-present DP 2 behavior, a single present
modality, aligned list metadata, and sparse DP 1 cleanup; all passed. The focused
regression is included in
test_dp_utils.py.Scope: this validates deterministic CPU data ownership and does not claim a full
suite, GPU, distributed end-to-end, convergence, or performance run. The normal
local pytest environment was unavailable because a locked dependency has no
wheel for this workstation platform; CI should execute the included focused unit
test in the project container.