fix(skippy): preserve token ordering during sampled verification - #1173
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR replaces the previous sampled-verification patch with token-history validation, sampler-state alignment checks, causal token recording, and full-batch recording when output sampling is disabled. ChangesSampled verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SampledVerification
participant TokenHistory
participant SamplerState
SampledVerification->>TokenHistory: Remove eagerly appended tokens
SampledVerification->>SamplerState: Validate history alignment
SampledVerification->>TokenHistory: Record tokens before sampling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request is currently a draft. Reviews will not take place until the PR is marked as ready for review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@third_party/llama.cpp/patches/0048-Fix-sampled-verification-token-history.patch`:
- Around line 45-46: Update the non-sampled branch around skippy_record_tokens
to reject sampler/history disagreement before recording tokens: if
sampling_accepted_token_count exceeds session->token_history.size(), return
SKIPPY_STATUS_RUNTIME_ERROR; otherwise preserve the existing recording behavior.
Add a regression test covering accepted sampler tokens exceeding the existing
history, including non-output recording, and verify the runtime error is
returned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e03bffd0-6518-432f-90f8-2910cac2017d
📒 Files selected for processing (2)
third_party/llama.cpp/patches/0048-Fix-sampled-verification-token-history.patchthird_party/llama.cpp/patches/0048-Preserve-grammar-state-during-sampled-verification.patch
💤 Files with no reviewable changes (1)
- third_party/llama.cpp/patches/0048-Preserve-grammar-state-during-sampled-verification.patch
Why this matters
While checking several predicted tokens, Mesh could let an earlier decision see tokens from later in the batch or count tokens twice. That can make accelerated generation choose different tokens from normal generation.
This change makes each prediction see only the token sequence that should exist at that exact point.
Technical details
This keeps repetition penalties, grammar state, and other history-sensitive sampling behavior aligned with serial decoding.
Validation
bash scripts/prepare-llama.sh— complete 65-patch queue applied successfullycargo test -p skippy-runtime --quiet— 68 passedThis PR is based directly on
mainand has no dependency on the companion fixes.Summary by CodeRabbit