huggingface / huggingface/candle
PagedKvCache::write_new_kv's host readback breaks CudaGraph capture for the decode step
- Dominant language
- Rust
- Stars
- 21k
- Forks
- 1.8k
- Avg merge
- 16h 42m
- Merged PRs (30d)
- 25
Description
## Use case
A downstream inference-serving deployment wants to capture the steady-state
single-token decode step with `candle_core::CudaGraph` and replay it every
subsequent step instead of relaunching each kernel individually, composed
with the paged-attention KV cache. This surfaced after wiring graph-replay-safe
rotary position handling (a device-tensor decode-position seam for RoPE) on
top of the existing paged-attention path.
## The gap: `PagedKvCache::write_new_kv`'s scatter-index computation is not graph-capturable
`CudaGraph::capture`'s own doc comment states the constraint plainly: a
captured closure's operations must be pure device-side kernel launches with
stable buffer addresses — no host/device synchronization, no new allocations.
`write_new_kv` (`candle-transformers/src/models/llama.rs`) is called on
*every* forward pass through the paged path (prefill and decode) and its
very first operation is:
```rust
fn write_new_kv(&self, k: &Tensor, v: &Tensor, index_pos: usize) -> Result<()> {
...
let block_table = self.block_table.to_dtype(DType::U32)?.to_vec2::()?;
...
let last_pos = index_pos + seq_len - 1;
let mut slots = Vec::with_capacity(b_sz * seq_len);
for row in &block_table {
// host-side arithmetic over the block table's *contents*
...
slots.push((physical_block * self.page_block_size + offset) as u32);
}
let indices = Tensor::from_vec(slots, (b_sz * seq_len, 1, 1), device)?...;
...
key_flat.scatter_set(&indices, &k_flat, 0)?;
value_flat.scatter_set(&indices, &v_flat, 0)?;
Ok(())
}
```
`Tensor::to_vec2` on a CUDA tensor is a blocking device-to-host copy. Issuing
this (or any synchronizing operation) on a stream that's mid-capture is
invalid in CUDA's stream-capture API and would either fail the capture
outright, or — if some layer tolerated it — silently freeze the *destination*
scatter indices to whatever `index_pos`/block-table contents produced at
capture time, while `k`/`v` *values* keep updating correctly on each replay
via the captured kernels. That would silently misplace every subsequent
decode step's K/V into the position captured at step one. `Tensor::from_vec(slots,
...)` inside the closure is also a fresh allocation, independently disallowed
during capture.
Note this is a real correctness/capturability gap in the *existing*,
already-merged paged-attention path, found by reading the source before
attempting a graph capture — not a regression from the decode-position work.
## Proposed additive change
A way to drive `write_new_kv`'s scatter destinations without a host
round-trip. Two shapes seem reasonable (illustrative, not prescriptive):
1. **Compute scatter indices on-device.** `block_table` (`(batch,
max_blocks)`, already a device tensor) combined with `index_pos`/`seq_len`
could derive the flat scatter offsets via `Tensor::gather`/arithmetic ops
entirely on-device (e.g. `logical_block = pos / page_block_size`, `offset
= pos % page_block_size`, gather the physical block id from `block_table`
at `logical_block`, then `physical_block * page_block_size + offset`),
replacing the `to_vec2` + host loop with device tensor ops. `index_pos`
itself would need the same device-tensor treatment (not a host `usize`)
to keep the whole thing graph-safe end to end for the decode step.
2. **Accept caller-precomputed device indices directly**, e.g. a new method
like `write_new_kv_at(&self, k: &Tensor, v: &Tensor, indices: &Tensor)`
that skips the internal derivation entirely. The caller already tracks
each sequence's block assignment on the host before ever uploading
`block_table` to the device — it doesn't strictly need to re-derive those
same indices from a device readback of the tensor it just built. For the
graph-capture case specifically, the caller would maintain a persistent
device index tensor (written in place before each replay) and pass it
straight through.
Either way, this should remain scoped to the decode step (`seq_len == 1`) if
that simplifies the derivation — prefill can keep the existing
`write_new_kv` behavior unconditionally.
## Scope
Additive only — default behavior (existing `write_new_kv` call sites,
unchanged) must be unaffected.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in candle-transformers/src/models/llama.rs at PagedKvCache::write_new_kv and trace its paged-attention callers, especially the decode path. Compare the proposed device-index and caller-precomputed-index approaches, then verify that CUDA graph capture avoids host readback and fresh allocation while existing behavior and prefill remain unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- machine-learning, performance
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100