Repository navigation
feat(cuda): retain native KV across pipeline turns - #1152
Conversation
Keep the native decoder and its device-resident KV bindings alive across sequential pipeline requests. Rewind directly to the shared token prefix and prefill only the new suffix, preserving BF16 KV on GPU without host conversion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1152 +/- ##
==========================================
- Coverage 80.58% 80.39% -0.20%
==========================================
Files 370 359 -11
Lines 162350 157243 -5107
Branches 162350 157243 -5107
==========================================
- Hits 130827 126412 -4415
+ Misses 26786 26270 -516
+ Partials 4737 4561 -176
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…reshed The retained native decoder and `self.retained` were stored under different conditions: the decoder whenever `use_native_decoder && paged_session.is_none()`, but the context only when `retains_kv && inputs_digest.is_some()`. Those diverge. `digest_request_identity` returns `None` when `absorb_value` cannot canonicalize an input, and `use_kv()` can be false. Such a request overwrote the decoder's KV while leaving `self.retained` describing an earlier request. The next request derives its rewind target from `retained`, so it would rewind to a prefix length computed against a context the KV no longer holds -- reusing one request's KV as another's prefix, silently, with plausible output and no error. Store the decoder inside the same branch that refreshes the context, so the two cannot diverge. When the context is not refreshed the decoder is dropped, which costs only the reuse.
Honor model.runtime_configurable.chunked_prefill for autoregressive pipelines. Process cache-miss suffixes in bounded chunks while retaining KV between chunks, reducing dynamic activation peaks without changing decode semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
Only add missing-attachment guidance when the underlying session error is actually a missing required input, so CUDA OOM and other decoder failures retain their real cause. Keep the retained decoder slot available in non-native builds so default-feature compilation remains valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0190e2eb-abe4-451f-b36d-44a035a99b7e
|
Reviewed the diff and pushed one fix ( The bug I foundThe retained decoder and the context describing it were stored under different conditions:
These diverge. This fails silently. No crash, no error, just plausible text conditioned on the wrong context. That is the worst failure mode for a caching change, and it is exactly the class of bug a TTFT measurement cannot see. Fixed by storing the decoder inside the same branch that refreshes the context, so the two cannot diverge. When the context is not refreshed the decoder is dropped, costing only the reuse. What I verified
What I could not verifyThe Muse Glimmer 30B BF16 numbers (48/73 tokens reused, TTFT 6.8s -> 1.5s). This box has no CUDA runtime. I accept them as reported and am explicit that they are unreproduced here; the correctness review above is from the code, which is where the risk actually was. Follow-up (not blocking)This PR adds a second prefix-reuse implementation: the native path computes |
✅ Benchmarks — No RegressionComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
Extracting the policy stops the two copies that exist. It does not stop the third. Two mechanisms now do. **Compile time.** `ReusedPrefix` is a newtype with a private field whose only constructors live in `prefix_reuse`. A backend cannot produce a reuse length without going through `apply_prefix_reuse`; forging one outside the module is `error[E0603]: tuple struct constructor `ReusedPrefix` is private`. The inline computation #1152 added would not compile today. **Test time.** `the_kv_rewind_primitives_are_only_driven_by_the_shared_policy` scans the crate source and fails if `.rewind_kv(`, `.truncate_past(` or `.current_kv_len(` is called outside the `KvPrefixStore` adapters. A second copy of the policy has to touch one of those primitives, so confining the primitives confines the policy. It runs under `cargo test`, so it cannot be missed by not looking at CI, and it names the offending file and line. The allowlist is deliberately three entries and the failure message says to route the new backend through `apply_prefix_reuse` rather than widen it. Widening the allowlist is how the duplication this module exists to prevent gets back in. Both mechanisms falsified: a probe call in `paged_decode.rs` reddens the guard (`pipeline\paged_decode.rs:311`), and a probe construction fails to compile.
…#1170) The decode loop was already shared. Prefix reuse was not. ## What was wrong One `run_decode_loop`, one `PipelineDecodeLoopBackend`, both backends behind `&mut dyn PipelineDecoderComponent` — the loop is properly unified. But #1152 added a **second** prefix-reuse implementation inline on the native path, alongside `reusable_prefix_len` used by ORT. Two copies of one policy, already drifted in **opposite** directions: | guard | native copy | ORT copy | |---|---|---| | `use_kv` gate | missing | present | | clamp `shared` against the KV's real length | present | missing | Each was missing a guard the other had. Neither omission can surface as a crash: a prefix-reuse bug produces fluent text conditioned on the wrong context, so no test fails and no benchmark moves. ## What this does - `pipeline::prefix_reuse::plan_prefix_reuse` — the decision, as a **pure function**. - `apply_prefix_reuse` over a `KvPrefixStore` seam — the application. - Only the *mechanism* differs by backend now: a native decoder rewinds its session-resident cache (`rewind_kv`), ORT slices pipeline-held past tensors (`truncate_past`). Both adapters live in `flat_autoregressive.rs` and are ~20 lines each. Both backends now get both guards. ## A latent hazard this closes Sharing the *application* means a declined rewind now **always resets**. Reporting "reuse nothing" while leaving the previous turn's tokens in the cache would prefill the new prompt on top of them. The native path had exactly that shape — unreachable today only because `NativePipelineDecoder::rewind_kv` never returns `false`, which is a property of the current sole implementor, not of the contract. ## Validation - `cargo test -p onnx-genai-engine --features native-backend --lib` — **507 passed, 0 failed, 1 ignored**. The pre-existing ORT rewind-semantics tests (`failed_rewind_of_ort_owned_kv_leaves_session_unchanged`, `failed_rewind_of_runner_backed_state_leaves_session_unchanged`, `failed_rewind_of_windowed_evicted_position_leaves_session_unchanged`) still pass — that is the no-regression evidence for the ORT path. - `cargo clippy -p onnx-genai-engine --features native-backend --all-targets -- -D warnings` — clean. - The policy is now testable without a model or a device: **11 unit tests where there were none.** **Falsified, not just written.** Both new guards were deliberately broken and confirmed to redden: - removing `shared.min(current)` → `the_cache_length_bounds_the_reuse_when_the_context_claims_more` and `an_empty_cache_reuses_nothing_however_much_the_prompt_shares` FAIL - removing the reset-on-decline → `a_declined_rewind_empties_the_cache_rather_than_leaving_its_tail` FAILS ## Performance No behaviour change on either backend for any input the two copies already agreed on. Same work per turn; one added trait dispatch per *request* (not per token), which is unmeasurable against a prefill. --------- Co-authored-by: justinchuby <223556219+Copilot@users.noreply.github.com>
Summary
model.runtime_configurable.chunked_prefill.chunk_sizeto bound multi-token prefill activation peaksValidation
cargo build --release -p onnx-genai-cli --features native-cudacargo test -p onnx-genai-cli only_input_not_found_errors_are_classified_as_missing_required_input