feat(gemma4): paged KV serving across both attention families - #885
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 572758b175
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
572758b to
c1d7411
Compare
|
@codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1d74119ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| anyhow::ensure!( | ||
| ctx.device_ordinal == self.local_pool.buffer().ordinal(), | ||
| "this context is on device {} but the pools live on device {}", | ||
| ctx.device_ordinal, | ||
| self.local_pool.buffer().ordinal() |
There was a problem hiding this comment.
Require the serving stream, not just its device
When step receives a different DeviceContext for the same GPU—or successive decode steps alternate such contexts—this check passes even though DeviceContext::new_with_device creates a fresh CUDA stream and explicitly disables cross-stream event tracking (pegainfer-kernels/src/tensor.rs:287-297). Work on the new stream can therefore race the constructor's RoPE/pool initialization or a preceding step's KV writes, producing logits from incomplete cache state; require the construction stream/context or add explicit synchronization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Device equality does not establish stream ordering here because event tracking is disabled. I’ll bind the serving path to the construction stream so pool/RoPE initialization and successive KV writes remain ordered without adding per-step synchronization.
c1d7411 to
0edb4db
Compare
|
@codex check again please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0edb4dbe83
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| kv.local.advance(seq_len); | ||
| kv.global.advance(seq_len); |
There was a problem hiding this comment.
Commit KV frontiers only after a successful step
When any fallible operation after these lines returns Err—starting with plan_step's GPU metadata allocations and continuing through H2D, scratch allocation, kernel launches, and the logits tail—both frontiers already claim the new tokens even though their KV rows may be unwritten or only partially written. If the caller retries or otherwise reuses this GemmaKv, the next start_pos skips the failed span and attention reads invalid cache contents; defer the frontier commit until success or explicitly roll back/poison the state on every error path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Great. The current plan construction depends on a prematurely advanced frontier, and an Err leaves reusable-looking state that may describe unwritten KV.
Signed-off-by: Feathbow <feathbow@gmail.com>
0edb4db to
00ca5cd
Compare
|
@codex review again please |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Description
Closes #884.
Serving the 12B tower needs two KV families, not one: local layers are sliding-window attention at head_dim 256 with 8 KV heads, global layers are full attention at head_dim 512 with 1 KV head. One pool with one page geometry cannot hold both, and a request holds pages in both at once. This adds a pool per family, a per-request state in each, and the step machinery over them — prefill, greedy decode, and the admission that keeps the two pools consistent. The pool-write prep (#881) and the windowed attention wrapper (#883) it rides on are already in.
KvStateonly ever moves forward.Test Env
Single GPU (sm_89, x86_64), CUDA 12.9.
Verification
cargo fmt --all -- --check;cargo build/cargo clippy --release -p pegainfer-gemma4 --features gemma4 --all-targets -- -D warnings; the same pair featureless — all clean. 26 library tests pass.generate()token for token on three prompts, 50 tokens each. The dumper refuses anything that is not reachable rather than recording it: sdpa and eager must agree on every token, a replay must reproduce the run, and each case is truncated at the first step whose top1-top2 margin falls to the gate. The three admitted cases measured minimum margins of 2.69/5.62/6.81 at dump time — over 26 candidate prompts, exactly three stayed decisive for 50 straight steps, because near-tie steps flip between independent implementations even where HF agrees with itself (flips observed up to margin 1.69). The fixture records the gate and the agreement claim, not the per-step margins themselves.Type of Change