huggingface / huggingface/candle

PagedKvCache::write_new_kv's host readback breaks CudaGraph capture for the decode step

Open
#3,732 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.