Skip to content

feat(rpc): #470 Phase 1 — GGUF-derived model identity getters, remove model_hash - #63

Merged
ddvnguyen merged 2 commits into
hydra-forkfrom
feat/470-gguf-model-identity
Jul 24, 2026
Merged

feat(rpc): #470 Phase 1 — GGUF-derived model identity getters, remove model_hash#63
ddvnguyen merged 2 commits into
hydra-forkfrom
feat/470-gguf-model-identity

Conversation

@ddvnguyen

Copy link
Copy Markdown
Owner

Summary

Part of ggml-org#470 ("Merged P/D StatePut + Decode with GGUF-derived model identity"), Phase 1 of a 4-phase rollout. Companion PR on the Core side: ddvnguyen/hydra_vortex#489 (Phase 3, Store schema + CrossModelGuard).

Replaces the RPC wire's model_hash (SHA-256 of the GGUF file) with 4 GGUF-derived semantic identity fields: tokenizer, model_name, model_quant, model_capabilities (bitfield: MTP/VISION/REASONING/TOOL_USE/CODE). A file hash changes on rebuild even when content is unchanged, and gives no information about model capabilities — which matters for deciding whether it's safe to reuse a KV cache across a prefill/decode split.

model_hash is removed entirely from the RPC wire and the server_task_result_hydra_state/server_task_result_hydra_engine structs — not kept as a fallback. llama_model_hash() itself is left in place (still used elsewhere); only its RPC-layer call sites are removed.

Getter design (grounded in real GGUF metadata, not the spec assumption)

I dumped 3 in-fleet production GGUFs with gguf_dump.py before finalizing the bit derivation, since the original design assumed a general.capabilities key that does not exist in any of them:

File MTP (nextn_predict_layers) Tags general.name base_model.0.name
Qwopus3.6-35B-A3B-MTP-Balanced present (1) has "reasoning" Safetensors (useless) Qwen3.6 35B A3B
Qwopus3.6-35B-A3B-Balanced (non-MTP) absent has "reasoning" Safetensors (useless) Qwen3.6 35B A3B
Qwopus3.6-27B-Coder-Compat-MTP present (1) absent Qwopus3.6 27B Coder... (useful) absent

Key findings that shaped the implementation:

  • general.name is frequently a conversion-tool artifact ("Safetensors") — not usable as the primary display-name source. llama_model_get_display_name() prefers general.base_model.0.name, falling back to general.name only when the former is absent (validated against the 27B file, where the fallback is the useful one).
  • MTP bit is derived from the real, non-heuristic <arch>.nextn_predict_layers GGUF key (via the existing LLM_KV(arch)(...) arch-suffix lookup pattern, not hardcoded to one architecture) — confirmed present/absent correctly across MTP and non-MTP variants of the same model family.
  • REASONING/TOOL_USE/CODE bits are explicitly documented in-code as heuristic (tag/name substring matching), unlike MTP which has a real signal. VISION is left unset with a comment explaining why (vision projectors are separate --mmproj GGUFs in this codebase, not embedded).

Changes

  • include/llama.h — 4 new public getters: llama_model_get_tokenizer_model, llama_model_get_display_name, llama_model_get_quant_label, llama_model_get_capabilities_bitfield.
  • src/llama-model.h/.cpp — implementations; added gguf_tags field (the general.tags array is skipped by the generic gguf_kv scalar loop, needed its own read).
  • tools/server/server-task.h/.cppmodel_hash removed from both hydra result structs; new fields added.
  • tools/server/server-context.cpp — all 5 RPC response meta_j sites (STATE_GET/PUT/META/PREFILL) + population sites + log lines updated.

Test plan

  • cmake --build build --target llama llama-server-impl — clean compile, verified independently by me (reviewer), not just agent self-report. Only pre-existing warnings (unrelated deprecated-API and format-string warnings already present before this change).
  • GGUF-dump validation against all 3 in-fleet files — MTP/REASONING/CODE bit derivation and display-name fallback all behave correctly, including the absent-key cases.
  • Not yet run against a live engine instance — E2E verification is a later, separate step.

Notes for reviewer

Does not touch DECODE (0x43) handler internals or hydra_classify_config_key() — those are Phase 2 (separate PR, depends on this one for the new identity getters).

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

Replace model_hash (SHA-256 of GGUF file, changes on rebuild) with
GGUF-derived semantic identity for cross-model KV safety and capability
awareness in STATE_GET/STATE_PUT/STATE_META/PREFILL/DECODE responses.

New public API (include/llama.h):
- llama_model_get_tokenizer_model() — reads tokenizer.ggml.model
- llama_model_get_display_name() — reads general.base_model.0.name
  with fallback to general.name (avoids 'Safetensors' artifact)
- llama_model_get_quant_label() — derives human-readable label from
  general.file_type via llama_ftype enum mapping
- llama_model_get_capabilities_bitfield() — bitfield derived from GGUF:
  bit 0 MTP (<arch>.nextn_predict_layers > 0)
  bit 1 VISION (reserved, unset — needs mmproj)
  bit 2 REASONING (general.tags contains 'reasoning')
  bit 3 TOOL_USE (heuristic from tags/display name)
  bit 4 CODE (heuristic from tags/display name)

GGUF dump validation (3 in-fleet files):
- Qwopus3.6-35B-A3B-v1-APEX-MTP-I-Balanced.gguf:
  arch=qwen35moe, MTP=1, tags=[reasoning,...], ftype=17
- Qwopus3.6-35B-A3B-v1-APEX-I-Balanced.gguf:
  arch=qwen35moe, MTP=absent, tags=[reasoning,...], ftype=17
- Qwopus3.6-27B-Coder-Compat-MTP-Q5_K_M.gguf:
  arch=qwen35, MTP=1, tags=absent, name='Qwopus3.6 27B Coder...'

Closes ggml-org#470 (Phase 1).
@ddvnguyen

Copy link
Copy Markdown
Owner Author

Review — Phase 1 of ggml-org#470 rollout

Verdict: REQUEST_CHANGES

Verified against the actual diff (hydra-fork..feat/470-gguf-model-identity), rebuilt clean (only pre-existing deprecated-API/format warnings). Confirmed no model_hash references remain anywhere in tools/server/ — writer/reader sides of the JSON-based RPC meta blob are consistent across all 5 call sites. Pointer-lifetime of the 4 new getters is safe (const char* into llama_model::gguf_kv, populated once in load_hparams, never mutated after).

Findings

  1. CODE capability bit false-positives on common substrings (src/llama-model.cpp, llama_model_get_capabilities_bitfield, bit 4 / 0x10). The heuristic does lower.find("code") != npos against tags and display name. "code" is a substring of "encode", "decode", "encoder", "decoder", "autoencoder" — all common HF/GGUF tags for non-code models. Concrete failure: a plain causal-LM GGUF tagged ["text-generation", "decoder-only"] gets CODE set even though it has nothing to do with code generation. Downstream, CrossModelGuard (Phase 3) would apply a spurious WarnAndProceed for an unrelated model swap based on this false bit. Needs a word-boundary check or explicit deny-list, not raw substring find.

  2. Minor/non-blocking: the inline capability-heuristic's display-name fallback checks .count("general.base_model.0.name") only, without the empty-string check that the dedicated llama_model_get_display_name() getter applies — a small inconsistency between what the getter reports and what the capability heuristic actually sees if that key is present-but-empty.

  3. No unit tests added for the 4 new getters or gguf_tags parsing — not a new regression in convention (matches pre-existing lack of tests for llama_model_hash()), but worth calling out since these getters are what Phase 3's CrossModelGuard will gate cross-model KV safety on.

Everything else checked out: MTP-bit derivation correctly uses the arch-suffixed LLM_KV(arch)(...) pattern (not hardcoded), correctly distinguishes "key absent" from "present with 0 layers", and general.tags array parsing has no leak/double-free.

Cross-repo note: capability bit values (MTP=0x01, VISION=0x02, REASONING=0x04, TOOL_USE=0x08, CODE=0x10) were confirmed to match Core PR ggml-org#489's ModelIdentity.cs exactly — the wire contract between fork and Core is consistent.

- Replace raw substring find() with explicit kCodeTags/kToolTags sets
  to eliminate false positives on 'decoder-only', 'encoder', 'autoencoder'
- Fix display-name fallback to skip empty values (match getter logic)
- Add CamelCase-aware word-boundary check for display names
- Add test-model-identity: 28 tests for capability bitfield derivation logic

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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