Skip to content

Correct an ngram verdict that was measured on a broken build - #49

Merged
gilesknap merged 1 commit into
mainfrom
runbook-ngram-correction
Aug 31, 2026
Merged

Correct an ngram verdict that was measured on a broken build#49
gilesknap merged 1 commit into
mainfrom
runbook-ngram-correction

Conversation

@gilesknap

Copy link
Copy Markdown
Owner

What was wrong

The runbook's speculation section claimed the long copy-heavy case was settled.
It was measured on the installed engine, which pins b10628 — 49 commits
short of llama.cpp PR #27812,
the Vulkan graph-optimiser fix. That bug let the target accept draft tokens it
had not chosen, making the acceptance counter meaningless. Those numbers should
not have been written down as a verdict, and this corrects them.

Re-measured on b10715

Contains the fix (compare b387ddfd...b10715 = ahead 38, behind 0), same Vulkan
backend, dev/spec-sweep.py, seven samples per cell plus a discarded warm-up,
unique prefix and cache_prompt=false, served model checked either side.

prose code-edit long-copy
baseline 34.1 33.8 32.8
draft-mtp 54.8 (1.61×, 86%) 55.7 (1.65×, 87%) 59.7 (1.82×, 100%)
ngram-cache 22.2 (0.65×, 24%) 26.2 (0.78×, 0%) 28.9 (0.88×, 58%)
draft-mtp,ngram-cache 46.2 (1.35×, 74%) 50.0 (1.48×, 82%) 52.7 (1.61×, 87%)

What changed in the conclusion

The verdict survives; the reasoning behind it does not.

  • The bug was flattering ngram, not handicapping it. The natural assumption
    was that invalid acceptance numbers denied ngram a fair hearing. Prose reads
    0.87× at 47% acceptance before the fix and 0.65× at 24% after — so the
    recorded 0.88× was itself an over-estimate.
  • The workload objection was sound and still lost. long-copy lifts
    acceptance from 0% to 58%, and is ngram's best case at 0.88× against 0.65× on
    prose. Copy-heavy work is where prompt-lookup does least badly, not where it
    wins.
  • MTP is untouched by the fix (1.62× → 1.61×, acceptance identical); short,
    mostly-correct drafts left no room for spurious accepts. Its copy-heavy figure
    is restated from the good build at 1.82×.

The operational half

cli.LLAMA_BUILD still pins the pre-fix build, so anything measured on the
installed engine reproduces the invalid numbers — which is exactly how this
mistake happened. The runbook now says so and points at SWEEP_LLAMA_DIR.
Moving the pin is a separate change with its own testing and is not done here.

Docs only; no code paths touched.

Related: #48 carries the same result in docs/explanations/going-faster.md.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Fx1NpWPBagVMvfFYp29yfj

The runbook claimed the long-copy case was settled. It was measured on the
installed engine, which pins b10628 -- 49 commits short of llama.cpp PR #27812,
the Vulkan graph-optimiser fix. That bug let the target accept draft tokens it
had not chosen, which made the acceptance counter meaningless, so the numbers
should never have been written down as a verdict.

Re-measured on b10715, which contains the fix, same Vulkan backend, seven
samples per cell. The conclusion survives and the reasoning behind it does not:
the bug had been *flattering* ngram, not handicapping it. Prose reads 0.87x at
47% acceptance before the fix and 0.65x at 24% after, so the recorded 0.88x was
itself an over-estimate. Long copy-heavy prompts are where prompt-lookup does
least badly (0.88x) rather than where it wins, and its acceptance does climb
from 0% to 58% -- the objection to the original test was sound, it just did not
rescue the result.

MTP's figures are unchanged across the fix (1.62x -> 1.61x, acceptance
identical), because short and mostly-correct drafts left no room for spurious
accepts. Its copy-heavy number is restated from the good build at 1.82x.

Adds the operational half of this: cli.LLAMA_BUILD still pins the pre-fix
build, so anything measured on the installed engine reproduces the invalid
numbers. Point SWEEP_LLAMA_DIR at a newer build until the pin moves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fx1NpWPBagVMvfFYp29yfj
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 5 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e5066b2-162c-4926-8596-4ec3835dc2ac

📥 Commits

Reviewing files that changed from the base of the PR and between e546007 and 6724f99.

📒 Files selected for processing (1)
  • .claude/skills/lllm3090-runbook/SKILL.md

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gilesknap
gilesknap merged commit 1d51319 into main Aug 31, 2026
7 checks passed
@gilesknap
gilesknap deleted the runbook-ngram-correction branch August 31, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant