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

[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).

[ngxson]

/bot review

[ggml-gh-bot]

Automated code review

The refactor since the last reviewed commit (68d9a1a) is clean and directly addresses the earlier nits. Here's my follow-up review of dd71b82 (moving the GGUF auto-detect into common/speculative.cpp).

Summary

The GGUF-peek logic was extracted from common/arg.cpp into a new helper common_speculative_types_from_gguf in the speculative module, with arg.cpp reduced to a 4-line call site. This is the right move: the detection now lives next to the speculative-type machinery it feeds, and arg.cpp no longer needs gguf.h. The previous nits (4, 6) are fixed - comment spacing now matches the codebase (/* .no_alloc = */), and an SPC_INF line logs the detected type.

Improvements worth noting

• Type validation added (speculative.cpp:2243): gguf_get_kv_type(...) != GGUF_TYPE_STRING before treating general.architecture as a string is a good robustness step over the previous version, which called gguf_get_val_str unconditionally. This guards a malformed GGUF where the key exists but holds a non-string value.
• RAII via gguf_context_ptr (speculative.cpp:2237): cleaner than the manual gguf_free before, and consistent with src/llama-adapter.cpp:158 / src/llama-quant.cpp:938.
• The #include "gguf.h" removal from arg.cpp is correct - no remaining direct gguf calls there (verified).

No new blocking or correctness issues in the changed lines. Function signatures, header declaration ordering, and the SPC_INF / common_speculative_type_to_str usage all check out.

Will slow the review

(point 1) Split-GGUF limitation note was lost in the move. The single-file-assumption comment you added in 07d7f1f (in response to the prior review's point 1) did not survive the relocation to speculative.cpp. The limitation itself still holds: gguf_init_from_file reads one file and does not follow multi-part splits, so a sharded DSpark drafter with markov_w1.weight in a non-first part would be silently classified draft-dflash. Consider re-adding a one-line note at the call site in arg.cpp:577 (e.g. on the if comment) so the caveat travels with the code. Optional, but you explicitly committed it last round.

(point 2) Point 3 from the prior review (duplicated dspark detection) is only half-addressed. Moving the peek into the speculative module is an improvement, but the markov_w1.weight string check is still duplicated against src/models/dflash.cpp:86, which detects the same variant from the loaded model's dspark_markov_w1 tensor. The deeper fix (infer is_dspark from model.dspark_markov_w1 != nullptr after load, dropping this GGUF re-read entirely) is still open and would also close the split-GGUF edge case in point 1. You've stated this is filed as a future cleanup, which is fine - flagging only so it's not forgotten.

Nits

(point 3) speculative.cpp:2257-2258: the SPC_INF line is split across two lines mid-argument. The rest of the file generally keeps SPC_INF(...) call arguments on one line where they fit comfortably; this one fits easily. Minor.

Overall the follow-up is a solid refactor; nothing here blocks merge beyond the optional comment restoration in point 1.

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

[ngxson]

I haven't tested this but looks ok

[ngxson]

cc @CISC if you can have a quick look

[CISC]

I had been thinking about this myself, so will make a small follow-up to this shortly.