download: find MTP heads published in a dedicated MTP/ folder - #166
download: find MTP heads published in a dedicated MTP/ folder#166danielhanchen wants to merge 2 commits into
Conversation
find_best_sibling only accepts a candidate whose directory components are a prefix of the model's, so a top-level MTP/ folder is unreachable: it matches neither a model at the repo root nor one in a quant subfolder. unsloth/Qwen3.8-27B-GGUF and unsloth/Qwen3.8-Flash-Next-GGUF both publish their draft heads that way, and on the first the miss is not silent, the server refuses to start because it falls back to the target itself and the target has no MTP layers. Give find_best_sibling an optional sidecar_dir that additionally admits a candidate sitting in a single top-level folder of that name, ranked as depth 0 so a genuine same-directory sibling still wins. Empty sidecar_dir is exactly the old behaviour, which is what leaves mmproj, dflash, eagle3 and dspark untouched. Two selection details follow from having more than one head in that folder: - prefer_bits picks the designated default when nothing matches the requested tag exactly, rather than whichever head is arithmetically nearest. For MTP that is Q8_0, which measured both the fastest and the most accurate head, since a draft step is dominated by the LM head and that head is cheaper to execute at 8 bits than at bf16. An exact tag match still wins. - a head carrying its own token_embd/output is preferred over one that borrows them from the target, so the choice does not depend on repository listing order. The ranking keys are now a tuple rather than a chained condition. Order and semantics of the existing keys are unchanged: greater depth, then exact tag, then smallest quantization distance. Assisted-by: Claude
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. |
… cover it in the tests The first version applied prefer_bits and the self-contained preference to every mtp- candidate, not just to the ones the dedicated folder admits. test-model-resolution caught it: `hole default anchor` resolves an untagged Q4_K_M primary and expects mtp-model-Q4_0 on quantization distance, and the preferred quant overrode that to mtp-model-Q8_0. Rank a directory-prefix match ahead of a dedicated-folder one and gate both new rules on not being a prefix match. Any layout that resolved before this existed now resolves to the same file by construction rather than by inspection, and a head sharing the model's own directory keeps winning. Five cases added, replayed on the harness's listing reorderings like the rest: - a dedicated folder found from a model at the repo root, and from one in a quant subdirectory - the preferred quant winning when nothing matches the tag, and losing to an exact tag when one does - a same-directory head outranking the folder even when only the folder holds the exact tag - MTPX/ not being treated as the dedicated folder Assisted-by: Claude
#166 makes sidecar discovery find MTP heads in a dedicated MTP/ folder, which is how both Qwen3.8-27B-GGUF and Qwen3.8-Flash-Next-GGUF publish them. Without it `--spec-type draft-mtp` finds nothing on either repo, and on the 27B the server refuses to start. The 27941 pin was still 6b2b85c. That branch has moved twice since, to 52bc9ff and then to 21c13c4, so the nightly has been building a commit that predates both the -kv-unified NaN collapse fix and the indexer ext.x/ext.y restore fix. Repin to the current head. Assisted-by: Claude
![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: 18a6ae61b2
ℹ️ 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".
| const std::string & model, | ||
| const std::string & tag = "") { | ||
| return find_best_sibling(files, model, "mtp-", tag); | ||
| return find_best_sibling(files, model, "mtp-", tag, "MTP", 8); |
There was a problem hiding this comment.
When no HF tag is supplied for an existing flat/root layout, passing prefer_bits = 8 changes ordinary sibling selection as well as the new MTP/ selection. Running test-model-resolution on this commit aborts in hole default anchor: the code selects mtp-model-Q8_0.gguf instead of the established nearest-quant mtp-model-Q4_0.gguf. Apply the Q8 default only to candidates admitted through the dedicated-folder exception so existing layouts retain their behavior.
Useful? React with 👍 / 👎.
| const std::string path_upper = to_upper(f.path); | ||
|
|
||
| bool exact = !tag_upper.empty() && path_upper.find("-" + tag_upper + ".") != std::string::npos; | ||
| bool preferred = prefer_bits != 0 && bits == prefer_bits; |
There was a problem hiding this comment.
When a repository offers Q8_0 together with another 8-bit MTP format such as Q8_K_XL, extract_quant_bits() returns 8 for both, so both candidates are marked preferred and the selection can fall back to repository listing order. That does not reliably choose the specific Q8_0 format whose measured behavior motivates this default; compare the complete quantization tag instead of only its numerical bit width.
Useful? React with 👍 / 👎.
| sib_parts.begin(), sib_dir); | ||
| if (dir != sib_dir) { | ||
|
|
||
| size_t depth = 0; |
There was a problem hiding this comment.
When the primary model is at the repository root, an ordinary root sidecar also has depth 0, exactly like a newly admitted MTP/ candidate. In repositories containing both layouts, the folder candidate can therefore beat the model-adjacent sidecar on quantization or tie with it and make the result listing-order dependent, despite the stated same-directory precedence. Add a separate rank indicating that normal prefix matching succeeded.
Useful? React with 👍 / 👎.
| // `sidecar_dir` additionally admits candidates in a dedicated top-level folder of that name, which | ||
| // shares no directory prefix with the model and is otherwise unreachable: unsloth publishes MTP | ||
| // heads as MTP/mtp-<model>-<quant>.gguf, and without this the folder is silently never matched | ||
| // `prefer_bits` wins over proximity when no candidate matches `tag` exactly, so a repo offering | ||
| // several heads has one designated default rather than whichever is arithmetically nearest |
There was a problem hiding this comment.
This five-line block hard-wraps two sentences and embeds deployment-specific rationale, while the repository standard limits comments to usually 1-2 lines and explicitly forbids hard-wrapping. Keep only a concise generic description of the new ranking inputs and move the Unsloth example out of the code comment.
AGENTS.md reference: AGENTS.md:L74-L77
Useful? React with 👍 / 👎.
1 participant
Problem
find_best_siblingincommon/download.cpponly accepts a sidecar whose directory components are aprefix of the model's, so a top-level
MTP/folder is unreachable. It matches neither a model at therepository root nor one in a quant subfolder.
Both of our GGUF repos publish their draft heads that way, and on the 27B the miss is not silent.
With no sidecar found,
params.speculative.draft.mparams.pathstays empty and the target is used asits own draft, so the server refuses to start:
Measured on the shipped
b10715-mix-86bd2d3prebuilt:--spec-type draft-mtp, no-mdunsloth/Qwen3.8-27B-GGUFMTP/unsloth/Qwen3.8-Flash-Next-GGUFUD-IQ1_S/, sidecar inMTP/mmproj-BF16.gguf, no MTP fileThe second row is also the control that shows sibling discovery itself is fine:
mmproj-BF16.ggufsits at the repo root while the model is in
UD-IQ1_S/, and it is discovered. TheMTP/directoryis specifically what defeats the match.
Note
--mtpis registered only forLLAMA_EXAMPLE_DOWNLOAD, so onllama-serverthe switch thatenables discovery is
--spec-type draft-mtp.Change
common/download.cppandtests/test-model-resolution.cpp.git diff --stat -- ggml/is empty.find_best_siblinggains an optionalsidecar_dirthat additionally admits a candidate sitting in asingle top-level folder of that name, case insensitive. It ranks as depth 0, so a genuine
same-directory sibling still wins. An empty
sidecar_diris exactly the previous behaviour, which iswhat leaves
find_best_mmproj,find_best_dflash,find_best_eagle3andfind_best_dsparkuntouched. Only
find_best_mtppasses one,"MTP".Having more than one head in that folder forces two selection details:
prefer_bitspicks a designated default when nothing matches the requested tag exactly, instead ofwhichever head is arithmetically nearest. For MTP that is Q8_0. Without it a
UD-IQ1_Suser wouldbe handed the Q4_K_M head on bit proximity, and a
BF16user the bf16 head, which is the worst ofthe three: it is 1.88x larger than Q8_0 and slower, because a draft step is dominated by the LM
head and that head is cheaper to execute at 8 bits. An exact tag match still wins.
token_embd/outputis preferred over one that borrows them from thetarget, so the choice does not depend on repository listing order. The self contained file loads
against any build.
Both new rules are gated on the candidate having come from the dedicated folder, and a
directory-prefix match ranks ahead of a dedicated-folder one. So any layout that resolved before this
existed resolves to the same file by construction, and a head sharing the model's own directory keeps
winning even when only the folder holds the exact tag.
That gating is the second commit rather than the first, and
test-model-resolutionis what forcedit. The first version applied both rules to every
mtp-candidate, which brokehole default anchor: an untagged Q4_K_M primary expectsmtp-model-Q4_0on quantization distance, and thepreferred quant overrode it to
mtp-model-Q8_0.The ranking keys are now a tuple instead of a chained condition. Order and semantics of the existing
keys are unchanged: greater depth, then exact tag match, then smallest quantization distance.
Verification
Built CPU only from this branch. Selection was read by mapping the blobs the downloader actually
fetched back to repo paths through the HF tree API, so each assertion is about the chosen path rather
than about a download having happened.
test-model-resolutioncovers the new path with no network, on 19 cases, each replayed on theharness's listing reorderings:
mtp dir flatmtp dir subdirmtp dir exact tagmtp dir loses to siblingmtp dir near missMTPX/is not treated as the dedicated folderThe 14 pre-existing cases are unchanged and still pass.
Live checks against the two real repos, with selection read by mapping the blobs the downloader
actually fetched back to repo paths through the HF tree API, so each assertion is about the chosen
path rather than about a download having happened:
:UD-IQ1_Sshared-one:BF16:Q8_0mmproj-BF16.ggufstill discoveredb10715binary:UD-IQ1_Send to endLlama-3.2-1B-Instruct-GGUF:Q8_0, no--spec-typeThe 27B arm is the sharpest one: today it fails outright, so a clean start is only reachable if the
sidecar was found. A repo with no MTP folder still errors under
--spec-type draft-mtp, which isunchanged pre-existing behaviour, not something this introduces.
gguf_filename_is_modelalready excludes basenames containingmtp-, so publishing severalmtp-*.gguffiles cannot affect primary model selection. Confirmed by-hf ...:Q8_0still resolvingthe model to
Q8_0/Qwen3.8-Flash-Next-Q8_0-*.gguf.Scope
Fork only for now.
find_best_siblingis byte identical in ggml-org master and fork master, so thisis an upstream limitation being patched locally rather than a fork divergence being repaired. Worth
proposing upstream separately once it has run in a few nightlies.