feat(runtime): context-aware VRAM cost estimate from GGUF headers - #76
Conversation
Implements the cold-start half of the Phase B addendum (docs/NATIVE_RUNTIME_V2_SPEC.md): admission estimates now account for the KV cache and runtime overheads instead of GGUF file size alone -- the spike measured the file-size estimate 2.2x under reality at n_ctx=16384 (4180 MB actual vs 1925 MB estimated). - New GgufMetadataReader: header-only GGUF v2/v3 parser (LLamaSharp 0.27 exposes no header-only metadata API -- verified by reflection; LLamaWeights.Metadata needs a full load, useless at admission time). Early-exits before the multi-MB tokenizer arrays, memo-cached by path+size+mtime, returns null on any failure -- estimation must never be the thing that breaks admission. - OrcScheduler.EstimateRequiredBytes/TryAdmit gain an optional RuntimeOptions parameter: null preserves the legacy file-size-only behavior exactly (existing exact-fit tests unaffected); with options the estimate is fileSize + 256MB CUDA overhead allowance + the spike-validated byte-exact KV formula (blockCount x n_ctx x headCountKv x (keyLen+valLen) x 2) + 384MB compute-buffer allowance + an architecture-gated recurrent-state term (known hybrid arch prefixes only -- plain transformers never pay it, the over-reserve hazard that originally deferred this work). Header unreadable => legacy fallback, never worse than before. - RuntimeOrchestrator threads options through BOTH the admission check and the reservation-ledger commit, so a later role's admission is judged against the same footprint the earlier role was checked with. Tests: 13 new (612 total passing). GgufMetadataReaderTests writes tiny valid/invalid GGUF headers to temp files (format is simple and documented); OrcSchedulerTests covers legacy-preservation, fallback, exact formula math against Llama-3.2-3B's real dims, arch gating (mamba vs llama fixtures differing ONLY in arch name), and the headline scenario: a budget that fits the file size exactly is admitted by the legacy estimate and correctly denied by the context-aware one. /verify on real hardware: all 4 THEORC_TEST_GGUF-gated lanes green against the real Dolphin3.0-Llama3.2-3B GGUF, including the new real-file header-parse lane. All 7 consumer projects build clean. One test-authoring bug caught by running (not by review): my fallback test used the Binding() helper with adapterSizeBytes:null, which means an UNSIZED adapter (+512MB fallback), not no adapter -- the estimator was right and the test expectation was wrong. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a GGUF header metadata reader with caching and validation, uses parsed metadata plus runtime options for VRAM admission estimates, propagates options through orchestration, and adds comprehensive synthetic and real-file tests. ChangesGGUF-aware admission control
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@OrchestratorIDE/Core/Runtime/GgufMetadataReader.cs`:
- Around line 124-128: Update the early-exit condition in the GGUF
metadata-reading loop so it does not break until the explicit attention key and
value lengths are both available, while retaining the existing fallback inputs
needed to derive them. Ensure later metadata can still populate valueLength
before the code computes vl from valueLength ?? keyLength.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f4b51e3-e71a-4862-b795-5a87c738705b
📒 Files selected for processing (6)
OrchestratorIDE.NativeRuntime/OrchestratorIDE.NativeRuntime.csprojOrchestratorIDE.UnitTests/GgufMetadataReaderTests.csOrchestratorIDE.UnitTests/OrcSchedulerTests.csOrchestratorIDE/Core/Runtime/GgufMetadataReader.csOrchestratorIDE/Core/Runtime/OrcScheduler.csOrchestratorIDE/Core/Runtime/RuntimeOrchestrator.cs
Real finding: the header parser's early-exit fired once key_length was
in hand, which could skip an explicit attention.value_length written
right behind it (llama.cpp's writer emits the two adjacently) --
silently mis-sizing asymmetric-head (MLA-style) models via the
vl ?? kl fallback.
Fixed by removing the early exit entirely: a full walk of the KV
section with sized skips is milliseconds even past the tokenizer
arrays, runs once per (path, size, mtime) thanks to the memo cache,
and drops the ordering assumption for {arch}.* keys altogether. The
one remaining ordering assumption (general.architecture precedes
{arch}.* keys -- true of every llama.cpp-written file) is now
documented, and violating it degrades to null + legacy fallback, not
a wrong number.
Added a regression test with an asymmetric deepseek2-style fixture
(key_length=192, value_length=128 written after it) proving the
explicit value is honored. 613/613 tests pass; real-file gated lane
re-verified against the actual Dolphin GGUF.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Finding confirmed real and fixed in f6dea88: removed the early-exit entirely (full KV walk with sized skips — milliseconds, memo-cached) rather than patching the exit condition, added an asymmetric deepseek2-style regression fixture proving an explicit @coderabbitai review |
|
✅ Action performedReview finished.
|
… no Ollama fallback (#78) Native Runtime v2.0 Phase D (docs/NATIVE_RUNTIME_V2_SPEC.md §3.4) names an explicit requirement: a native-routed job with native prerequisites unavailable must fail closed with no silent Ollama substitution. Existing coverage only proved this via a synthetic FallbackCoordinator delegate that throws a manufactured exception — it verified the coordinator's own branching logic, not that a REAL admission denial from the REAL OrcScheduler actually surfaces through the REAL NativeWithFallbackRuntime wrapper without ever touching the fallback. Wires the real objects together instead: RuntimeOrchestrator + a real OrcScheduler making an actual TryAdmit denial against a zero-byte budget (not the simpler "no scheduler configured" case Phase A's own tests already cover) + NativeWithFallbackRuntime, and asserts RuntimeAdmissionDeniedException propagates with the fallback runtime never invoked. Needs no GGUF or hardware gate: a real capacity denial happens in EnsureAdmitted strictly before SessionManager ever opens a model file, so this specific Phase D requirement can run in every CI build rather than only the opt-in hardware-gated lane — landed early and stronger than the DoD strictly asked for. The rest of Phase D's full E2E lane (discovery through cancellation/telemetry on a real loaded model) remains open. Updated the spec's implementation-status banner and Phase B addendum status to reflect PR #76 (cost-estimate landed) and PR #77 (load-measurement, in flight) — both were stale before this edit. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…endum + D (#82) * docs: sync ROADMAP/CURRENT_STATE with Native Runtime v2.0 Phase B addendum + D Both files still described Phase B's cost-estimate half as deliberately deferred and Phase D as untouched -- stale since PR #76 (cost estimate), #77 (load-time measurement), #78 (negative fail-closed test), and #79 (cancellation-corruption fix) all landed since the last sync. Updates the Native Runtime v2.0 direction table in ROADMAP.md and the native_runtime note in CURRENT_STATE.yaml to reflect current reality: Phase B fully landed (estimate + measurement), Phase D essentially complete pending the E2E evidence-lane PR (#81, open). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: mark E2E evidence lane landed (PR #81 merged), not pending CodeRabbit finding: ROADMAP.md's banner still said the E2E lane was "built, verified on real hardware, and pending merge" while CURRENT_STATE.yaml's parallel note omitted that qualifier entirely -- inconsistent, and now stale either way since PR #81 actually merged. Updated both to say landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
What this is
Implementation of the cold-start estimator half of the Phase B addendum — the first code to land from the "measure, don't predict" research direction. The calibration cache (measure-and-remember) follows as its own PR; this PR ships the spike-validated formula path.
Why
The spike measured today's file-size-only admission estimate at 2.2× under reality at n_ctx=16384 (4180 MB actual vs 1925 MB estimated) — an admitted load that doesn't actually fit is an OOM crash waiting for a busier GPU. The KV formula in this PR was validated byte-exactly against llama.cpp's own allocator (224.00/896.00/1792.00 MiB at n_ctx 2048/8192/16384).
What ships
GgufMetadataReader— header-only GGUF v2/v3 parser. LLamaSharp 0.27 exposes no header-only metadata API (verified by reflection over the actual package;LLamaWeights.Metadataneeds a full weights load — useless at admission time). Early-exits before the multi-MB tokenizer arrays, memo-cached by path+size+mtime, returns null on any failure — estimation must never be the thing that breaks admission.OrcScheduler—EstimateRequiredBytes/TryAdmitgain an optionalRuntimeOptions:nullpreserves legacy file-size behavior exactly (existing exact-fit tests untouched); with options: file size + 256MB CUDA-overhead allowance + byte-exact KV formula + 384MB compute-buffer allowance (measured 260–285MB, and it shrank with larger ctx — a flat allowance is honest, a formula would be fake) + an architecture-gated recurrent-state term. Known-hybrid prefixes only (mamba/rwkv/jamba) — plain transformers never pay it, which was the exact over-reserve hazard that deferred this work originally. Unknown hybrids under-estimate at cold-start but are bounded by the Phase B live free-VRAM read + existing NoKvSlot defenses, and the future calibration cache covers them by measuring. Scheduler stays a pure decision function — no state added.RuntimeOrchestrator— threads options through both the admission check and the reservation-ledger commit, so later roles are judged against the same footprint earlier roles were checked with.Tests + /verify
GgufMetadataReaderTestswrites tiny valid/invalid GGUF headers to temp files;OrcSchedulerTestscovers legacy-preservation, unreadable-header fallback, exact formula math on Llama-3.2-3B's real dims, arch gating via two fixtures differing only in arch name, and the headline scenario: an exact-file-size budget that legacy admits and the context-aware estimate correctly denies./verify: all 4THEORC_TEST_GGUF-gated lanes green against the real Dolphin3.0-Llama3.2-3B GGUF, including a new real-file header-parse lane. All 7 consumer projects build clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests