Account for hybrid GGUF recurrent caches in served memory admission - #952
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd673c826b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Two confirmed admission/estimation correctness gaps can still allow under-reserving hybrid recurrent memory (draft_simple rollback depth, and skipping settings stamping when backend is unresolved).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens GGUF served-backend memory admission for hybrid/recurrent architectures by deriving cache geometry from verified GGUF header facts (including separately signed registry projections) and consistently applying node llama-server serving settings (slots/speculation) across placement, reservations, worker admission, and runner startup.
Changes:
- Add artifact-derived GGUF cache geometry (attention + recurrent + embedded-MTP dimensions) and incorporate it into KV + recurrent memory estimation and context ceilings.
- Introduce
NodeResources.llama_server_settings, stamp settings onto placements, and have the llama-server runner reject configuration drift after admission. - Add signed registry auxiliary GGUF-header target support (
v1/gguf-metadata.json) and update docs + tests to describe/validate the contract.
File summaries
| File | Description |
|---|---|
| website/docs/architecture.md | Documents the new GGUF geometry + llama-server settings admission contract. |
| website/docs/architecture-reference.md | Adds dense reference entries for gguf cache admission + registry GGUF metadata target. |
| website/docs/api-guide.md | Documents gguf_cache_geometry, registry_gguf_metadata, and settings-stamped admission semantics. |
| src/skulk/worker/runner/llama_server/runner.py | Unifies settings resolution, rejects drift vs placement stamp, and pins --spec-draft-n-max. |
| src/skulk/worker/main.py | Threads resolved backend + llama-server settings into worker-side footprint checks. |
| src/skulk/shared/types/worker/shards.py | Adds llama_server_settings to shard metadata for replicated admission inputs. |
| src/skulk/shared/types/profiling.py | Advertises llama_server_settings via NodeResources when llama-server backends are present. |
| src/skulk/shared/models/tests/test_registry.py | Adds tests for signed GGUF-header projection + cache behavior and tamper resistance. |
| src/skulk/shared/models/tests/test_gguf_memory.py | New tests validating Qwen3.5 hybrid cache geometry and invariants. |
| src/skulk/shared/models/tests/test_gguf_cards.py | Ensures GGUF header parsing retains hybrid fields and avoids old early-exit behavior. |
| src/skulk/shared/models/registry.py | Loads optional v1/gguf-metadata.json, binds it to snapshot/targets version, and caches it hash-bound. |
| src/skulk/shared/models/registry_gguf_metadata.py | New strict Pydantic contract for the separately signed GGUF-header metadata target. |
| src/skulk/shared/models/model_cards.py | Projects signed GGUF header evidence into runtime card geometry without changing canonical card bytes/IDs. |
| src/skulk/shared/models/memory_estimate.py | Adds recurrent-state accounting and backend/settings-aware per-token KV sizing for GGUF. |
| src/skulk/shared/models/llama_server_settings.py | New shared settings model + env resolver for slots/speculation enablement. |
| src/skulk/shared/models/gguf_memory.py | New GGUF cache-geometry model and Qwen3.5 scalar mapping into recurrent/attention dimensions. |
| src/skulk/master/tests/test_recurrent_memory_admission.py | New integration-style tests covering slot/depth recurrent costs across placement + ceilings. |
| src/skulk/master/placement.py | Stamps llama-server settings onto placements and threads settings into exact-footprint checks. |
| src/skulk/master/placement_utils.py | Extends cycle memory filtering to include recurrent-cache costs and settings requirements. |
| src/skulk/master/main.py | Ensures teardown credit and steward memory inputs use backend/settings-aware footprint math. |
| src/skulk/api/tests/test_data_plane_dispatch.py | Updates expected nodeResources JSON to include llamaServerSettings. |
| CLAUDE.md | Updates agent-facing architecture notes to reflect the new GGUF admission and registry metadata contract. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The public API documentation currently references new fields using snake_case / incorrect code paths, which does not match the actual camelCase JSON surface and “file:line” navigation conventions.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
website/docs/api-guide.md:580
- The API guide uses snake_case field names (
gguf_cache_geometry,registry_gguf_metadata,llama_server_settings), but the public JSON surface uses camelCase (e.g.llamaServerSettingsas shown elsewhere in this doc). This is likely to mislead API consumers looking for the actual response keys.
website/docs/architecture-reference.md:22 - architecture-reference.md says each entry has a file:line to jump to code, but this new entry uses non-existent/ambiguous paths like
shared/models/gguf_memory.pyandmemory_estimate.py(missing thesrc/skulk/prefix), making it harder to navigate and inconsistent with the rest of the document (e.g.src/skulk/master/main.py).
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78f978b74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Hybrid GGUF models can pass placement and then exhaust GPU memory because recurrent-cache allocations scale with server slots and speculative draft depth, rather than ordinary attention context alone. This change derives artifact geometry from verified signed GGUF header projections and uses shared serving settings consistently for placement, reservations, worker admission, and the served runner.
The registry reader validates exact artifact identity, bounded header evidence, and projected geometry; older cards remain compatible. Nodes advertise serving controls, placements capture them, and the runner rejects a changed configuration. Public API and both architecture documents describe the contract.
Validation: 4,227 Python tests passed (2 skipped; 240 slow deselected), strict typing with zero errors, Ruff, Nix formatting, OpenAPI export, and full TypeDoc/Docusaurus build. The production signed registry target was verified by the actual candidate client. Fresh isolated NVIDIA qualification used candidate
bd673c82and the exact engine build with 16 slots and MTP draft depth 3: both ordinary and steward model contexts loaded without the prior out-of-memory failure. An 8,192-token ordinary request returned successfully and retained task records attribute it to the intended ordinary instance; the steward also completed a real tool-driven proposal. The paid resource was terminated and authenticated inventory confirmed absence.This qualifies the generic memory and routing behavior. Provider-specific capacity acquisition and final plugin distribution acceptance remain separate; one subsequent provider create returned an availability error and was retained as an unreplayed unknown outcome.
Review follow-through in
e78f978brejects unresolved hybrid placements until backend telemetry establishes serving controls. Both automatic and exact-path regression tests failed before the guard and pass afterward; the full suite passed on this latest commit. This admission-only guard does not change the GPU-qualified serving path.