Repository navigation
Fix static-cache generate() with host-side inputs and under TP - #49317
Conversation
With inputs kept on another device (#46590), `prepare_inputs_for_generation` moves only the top-level tensors of `model_inputs` onto the model device, so a tensor nested in a dict stayed behind (example: causal masks for some models). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ithout TP Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…olds Under tensor parallelism `_get_static_cache_init_shape` divided the KV head count by `tp_size` whenever `tp_size > 1`, whether or not the plan shards attention. With no plan (Qwen3-VL), a gathered key projection (`colwise_gather_output`) or an expert-parallel plan, every rank keeps all heads, so the chunked-prefill cache is allocated with too few and the first `index_copy_` fails: "Incompatible shapes for broadcasting: (1, 8, 512, 128) and requested shape (1, 4, 512, 128)" for Qwen3-VL-8B-Instruct at tp=2. Only divide when the key projection resolves to `colwise`, the style that shards heads; skip early init when no key projection is found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
vasqu
left a comment
There was a problem hiding this comment.
Some initial comments, definitely see the overall issues but we can keep the PR description a bit shorter especially since the issue is already detailed
- Move tensors nested at any depth in dicts, lists and tuples to the device. - Read the KV heads per rank from the TP plan instead of scanning parameters. - Use a tiny random checkpoint in the nested inputs test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
vasqu
left a comment
There was a problem hiding this comment.
Smaller comments overall but cc @3outeille whenever you have time to sanity check as well
…jections into the TP util Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
gently pinging @3outeille |
| def _move_to_device(self: "GenerativePreTrainedModel", value): | ||
| """Moves the tensors in `value`, including those nested in dicts, lists and tuples, onto the model device.""" | ||
| if isinstance(value, torch.Tensor): | ||
| return value.to(self.device) | ||
| if isinstance(value, dict): | ||
| return {k: self._move_to_device(v) for k, v in value.items()} | ||
| if isinstance(value, (list, tuple)): | ||
| return type(value)(self._move_to_device(v) for v in value) | ||
| return value |
There was a problem hiding this comment.
I don't think it should be a method of GenerationMixin, better inside pytorch_utils or something as a separate function
There was a problem hiding this comment.
Lol just saw @vasqu said the opposite before on a resolved comment 😆 IMO it should rather be a general util, let me know your thoughts and sorry for the back and forth 😅
There was a problem hiding this comment.
I dont mind either way, I see the point of it being more general as well
There was a problem hiding this comment.
OK, just tell me which you prefer, and i will lazily ask claude to move it 😅
There was a problem hiding this comment.
Lets move it to something more general, e.g. torch utils or similar (not sure about the exact file name again)
There was a problem hiding this comment.
Moved to pytorch_utils.py
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@vasqu @Cyrilvallez any blockers ? |
CI recapDashboard: View test results in Grafana |
vasqu
left a comment
There was a problem hiding this comment.
Should be good, sorry for the delays
What does this PR do?
Fixes #49261, two
generate()bugs with a static cache:tp_sizeeven when the key projection isn't sharded. It now checks the TP plan.Note for #47811:
colwise_unitsshould count as sharded inget_kv_heads_per_rank. cc @kaixuanliuTests
test_generate_with_inputs_on_cpu_moves_tensors_in_dict_inputs(new)test_get_kv_heads_per_rank(new)test_chunked_prefill_initializes_static_cache_eagerly(extended: empty plan, gatheredk_proj)Also checked on Neuron, including Qwen3-VL-8B at TP=2.
Who can review?
@ArthurZucker @vasqu @Cyrilvallez @3outeille