Repository navigation
docs(#750): mark the 2b-impl scoping complete and annotate what each increment corrected - #941
Merged
Merged
Conversation
…increment corrected All four increments landed (#913, #920, #924, #930), but the document still read as an open plan, and 2c is being scoped against it right now. The corrections are the more useful part of it now, because three of the four returned one, so they are recorded both as a table up front and inline at the exact line that was wrong -- a reader trips over the claim, not over the summary. - §3's "the pleasant surprise: KV growth stride math is already batch-general" holds only for head-major. A fixed full-context stride commit is batch-general in both layouts because the batch axis is outermost; a growing bucket is not, because seq-major's per-sequence stride IS the mutable capacity, so growth relocates every sequence b>0. #920 refuses that case with a named bail! rather than computing a capacity-dependent stride, which would have passed every batch-1 test and corrupted the first batch-N run. The section conflated the two paths. - §4 item 6 places live_prefix_ranges in onnx-genai-kv; it is in this crate at native_decode/kv_commit.rs:122, so no cross-crate coordination was needed. Line number checked against current main rather than copied from the PR, since it moved when the batch parameter was added. Records the outcome the line was started for, reproduced by me rather than quoted from the PRs: capture survives batch-N while weights stream -- enabled=true captures=1 invalidations=0 fallbacks=0 with htod_bytes_per_token 1,234,271,232 at N=2 and 547,528,704 at N=4, byte_hit_rate 68.64% -> 72.17%, solo with managed streaming forced. That closes #891's flagged risk positively. And records the near-miss, because it is the part most worth carrying into 2c: #930's first version reported capture and streaming as mutually exclusive, from a real decline naming a real predicate, because its harness called an entry point that passed cuda_offload_policy: None and stable_va fell to a conservative default. The harness asserted pointer-instability; the runtime was never asked. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #941 +/- ##
==========================================
+ Coverage 77.59% 77.81% +0.21%
==========================================
Files 342 354 +12
Lines 142769 148153 +5384
Branches 142769 148153 +5384
==========================================
+ Hits 110787 115287 +4500
- Misses 27674 28372 +698
- Partials 4308 4494 +186
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
🔴 Benchmark Regression DetectedComparison 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
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
docs(#750): mark the 2b-impl scoping complete and annotate what each increment corrected
All four increments landed (#913, #920, #924, #930), but the document still
read as an open plan, and 2c is being scoped against it right now.
The corrections are the more useful part of it now, because three of the
four returned one, so they are recorded both as a table up front and inline
at the exact line that was wrong -- a reader trips over the claim, not over
the summary.
batch-general" holds only for head-major. A fixed full-context stride
commit is batch-general in both layouts because the batch axis is
outermost; a growing bucket is not, because seq-major's per-sequence
stride IS the mutable capacity, so growth relocates every sequence b>0.
feat(native-decode): make KV growth and commit geometry batch-aware, refuse seq-major relocation (2b-impl-2, #750) #920 refuses that case with a named bail! rather than computing a
capacity-dependent stride, which would have passed every batch-1 test and
corrupted the first batch-N run. The section conflated the two paths.
at native_decode/kv_commit.rs:122, so no cross-crate coordination was
needed. Line number checked against current main rather than copied from
the PR, since it moved when the batch parameter was added.
Records the outcome the line was started for, reproduced by me rather than
quoted from the PRs: capture survives batch-N while weights stream --
enabled=true captures=1 invalidations=0 fallbacks=0 with
htod_bytes_per_token 1,234,271,232 at N=2 and 547,528,704 at N=4,
byte_hit_rate 68.64% -> 72.17%, solo with managed streaming forced. That
closes #891's flagged risk positively.
And records the near-miss, because it is the part most worth carrying into
2c: #930's first version reported capture and streaming as mutually
exclusive, from a real decline naming a real predicate, because its harness
called an entry point that passed cuda_offload_policy: None and stable_va
fell to a conservative default. The harness asserted pointer-instability;
the runtime was never asked.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
Copilot-Session: d60eb808-7cc6-4abc-b48d-2a6dd3841624