common : auto-detect spec type from draft GGUF metadata

[aic0d3r]

Overview

When -md loads a local draft model without --spec-type, the sidecar inference only checks HF repo sidecars and misses local files. The draft model loads into VRAM but speculative decoding never activates (types stays NONE, tok/decode-pass = 1.000).

Reads general.architecture from the draft GGUF header and maps dflash + markov_w1.weight to draft-dspark, dflash without markov to draft-dflash.

Related: #26636 #26339

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: YES - code written with AI assistance, reviewed and understood

[ggml-gh-bot]

Hi @aic0d3r, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

• PR Template not respected: Please respect the template when creating a new pull request. Make sure to fill out all required sections.

---

Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

[taronaeo]

/bot review

[ggml-gh-bot]

Automated code review

Code Review: common : auto-detect spec type from draft GGUF metadata (#26814)

The change is small and well-scoped: when a local draft model is loaded with -md and no explicit --spec-type/sidecar was selected, it peeks at the draft GGUF metadata to set COMMON_SPECULATIVE_TYPE_DRAFT_DFLASH or _DSPARK. The placement (after the sidecar block, guarded by spec_types_is_default) is correct: it only fires when no sidecar was found, so plan_spec is already empty and nothing downstream needs clearing. The tensor name markov_w1.weight matches the one already used in src/models/dflash.cpp:86, and the dflash arch string matches src/llama-arch.cpp:136. The include of gguf.h matches the pattern in common/common.cpp.

No blocking issues found. Items below.

Will slow the review

(point 1) Split-GGUF misclassification (correctness edge case). gguf_init_from_file reads a single file and does not follow multi-part splits, while llama_model_loader aggregates all splits. If a DSpark draft is sharded and markov_w1.weight lands in a part other than the first, gguf_find_tensor here returns -1 and the model is silently classified as draft-dflash instead of draft-dspark. The downstream model loader will then load the markov head (dspark_markov_w1 != nullptr) while the speculative impl runs in dflash mode (is_dspark=false), which affects n_draft_max and the anchor-first block layout. Either document this limitation (single-file drafts only) or reuse the split-aware loader path. Worth confirming with a sharded dspark GGUF.

(point 2) Auto-enabling speculative decoding is a behavior change. Previously, -md local-dflash.gguf without --spec-type left types={NONE} and speculative decoding stayed off (the symptom the PR fixes). Now it is silently enabled. This is the intended fix, but it is no longer opt-in for the dflash arch. Confirm this is desired; users can still opt out with --spec-type none (which makes types non-default, so the new block is skipped - verified via common_speculative_types_from_names returning {NONE} and the insert producing a non-default vector). The Related: #26636 #26339 context should make this explicit.

(point 3) Duplicated dspark detection / double GGUF read. src/models/dflash.cpp:86 already detects the dspark variant from the same markov_w1.weight tensor and stores model.dspark_markov_w1. The speculative impl (common/speculative.cpp:945) only uses is_dspark to pick block layout, and both draft-dflash/draft-dspark use the same impl class. A cleaner long-term design would infer is_dspark from the loaded draft model (model.dspark_markov_w1 != nullptr) rather than re-reading the GGUF header in arg.cpp and duplicating the tensor-name check. Not blocking for this PR, but worth a follow-up issue so the decision lives in one place.

Nits

(point 4) Initializer comment spacing. /*.no_alloc =*/ and /*.ctx =*/ (lines 581-582) omit the inner spaces used everywhere else, e.g. /* .no_alloc = */ in common/common.cpp:1878 and common/imatrix-loader.cpp:85. Match the surrounding style for consistency.

(point 5) Magic strings. "general.architecture" and "markov_w1.weight" are hardcoded here and also in src/llama-arch.cpp / src/models/dflash.cpp. Minor; a shared constant would avoid drift if either name changes, but the existing code already hardcodes them so this is consistent.

(point 6) No diagnostic log. When auto-detection sets the type, there is no LOG_INF (the sidecar path similarly defers logging, so this is consistent). A one-line log here would help users diagnose why speculative decoding turned on. Optional.

This review was generated automatically by pi coding agent using zai-org/GLM-5.2. It may contain mistakes. Maintainers make the final call.

[taronaeo]

@aic0d3r Can you check the bot's comments? #26814 (comment)

[aic0d3r]

@aic0d3r Can you check the bot's comments? #26814 (comment)

Thanks for the review @taronaeo. Addressed the bot's feedback in 07d7f1f:

Point 1 (split-GGUF edge case): Added a comment documenting the single-file assumption. DSpark drafters are single-file in practice — the bf16 drafter is ~11GB and the Q2K is ~6.5GB, both well below any split threshold. A sharded drafter would be unusual, but if it happens the user can work around it with explicit --spec-type draft-dspark. A proper fix (split-aware read) would require routing through llama_model_loader which isn't available at this point in arg.cpp — noted as a future follow-up if sharded drafters become common.

Point 2 (behavior change): Intended. The previous behavior (load draft into VRAM but silently never use it) was the bug this PR fixes. Users can still opt out with --spec-type none, which makes spec_types_is_default() return false and skips the new block entirely.

Point 3 (duplicated detection): Fair point on long-term design. The clean approach would be to infer is_dspark from the loaded model's dspark_markov_w1 tensor rather than re-reading the GGUF header here. But that would require restructuring the model loading pipeline to expose the tensor before common_models_handler_apply runs — a larger change than this fix warrants. Filed as a future cleanup.

Nits 4-6: Fixed comment spacing, added LOG_INF lines for both detection branches.

This directly fixes a real issue I hit: loading a local DSpark drafter with -md but no --spec-type loaded the model into VRAM and then silently ran with types={NONE} — speculative decoding never activated, tok/decode-pass stayed at 1.000. No warning, no error, just wasted VRAM.

[ngxson]

remove out-of-context comments

[ngxson]

see agents.md for acceptable code comment style

[ngxson]

use RAII version of gguf_context in ggml-cpp.h

[ngxson]

I believe the whole code should be part of speculative.cpp/.h

[aic0d3r]

Thanks for the detailed review, i agree with your points and have made the needed changes.

all three of ngxson's comments addressed in dd71b82, pushed to the PR branch:

1. arg.cpp:589 "should be part of speculative.cpp/.h" → logic moved to new common_speculative_types_from_gguf() in common/speculative.cpp:2231; arg.cpp keeps a 7-line call site; #include "gguf.h" dropped from arg.cpp.
2. arg.cpp:586 "use RAII" → now uses gguf_context_ptr from ggml-cpp.h; also added a gguf_get_kv_type string-type check so a malformed GGUF can't hit the gguf_get_val_str assert.
3. arg.cpp:580 "out-of-context comments" / agents.md style → split-size NOTE deleted; one-line comment matching the adjacent sidecar block.

Verified: llama-common builds clean; runnable check against real models — DSpark drafter Q2K and bf16 → draft-dspark, vocab GGUF and missing file → empty (no crash).