Fix/qwen35 gguf loading#466
Merged
Merged
Conversation
The portable driver could parse a qwen35/qwen35moe GGUF's architecture but not actually load it: `build_qwen3_5_` threw "layer_types missing", and even past that the GGUF→HF tensor mapper had no linear-attn / shared-expert cases and did not invert the converter's value transforms. Only the safetensors path was ever exercised. This teaches the GGUF path the full hybrid arch. gguf_hparams.cpp: derive `layer_types` from `qwen35.full_attention_interval` (layer i is full attention iff (i+1) % interval == 0, else linear), and fill the linear-attention dims from the `qwen35.ssm.*` keys (group_count→num K heads, time_step_rank→num V heads, state_size→head dim, conv_kernel). Enable `qwen35_attn_output_gate` — Qwen 3.5/3.6 fuse the attention output gate into a double-wide q_proj, which the graph splits. gguf_archive.cpp: map the qwen35 GGUF tensor names — `attn_qkv`/`attn_gate` → linear_attn.in_proj_qkv/in_proj_z, `ssm_alpha`/`ssm_beta` → in_proj_a/b, `ssm_a`/`ssm_dt.bias`/`ssm_conv1d`/`ssm_norm`/`ssm_out` → A_log/dt_bias/conv1d /norm/out_proj, and the `*_shexp` shared-expert tensors. model.cpp: invert the llama.cpp converter's value transforms at load for a GGUF source (`A_log` stored as `-exp`, `*norm.weight` folded `+1` except `linear_attn.norm`); accept F32/F16 conv1d; and select the multimodal tensor prefix from the archive so a flat text GGUF is not looked up under the `model.language_model.` wrapper. Verified: Qwen3.5-0.8B-Q4_K_M.gguf now boots on portable Metal and completes "The capital of France is" -> "**Paris**. Located in the region of Île-de-France" (coherent). Larger dense (27B) and MoE (35B-A3B) follow-ups: V-head reorder for num_v_heads != num_k_heads, and fused-expert assembly.
Extends the qwen35/qwen35moe GGUF loader past the small dense smoke case:
- GQA V-head order: the converter permutes the linear-attention V heads
grouped->tiled when num_v_heads != num_k_heads (so ggml's tiled
broadcast aligns), while HF safetensors keep grouped order. Carry a
`qwen35_linear_v_tiled` hparam (set for a GGUF source) and have the
graph expand Q/K with a matching tiled `ggml_repeat` instead of the
grouped `repeat_interleave`. The equal-head case is unaffected.
- MoE: read `expert_shared_feed_forward_length` into
`shared_expert_intermediate_size`, and load the experts from the GGUF's
three SEPARATE stacked tensors (`ffn_gate_exps` / `ffn_up_exps` /
`ffn_down_exps`) plus the shared-expert tensors directly, rather than
splitting a fused `mlp.experts.gate_up_proj` (which only the safetensors
layout provides).
Verified on portable Metal: Qwen3.6-27B-Q4_K_M (dense, 64 layers) and
Qwen3.6-35B-A3B-UD-Q4_K_M (MoE, 256 experts + shared expert) both boot and
complete a prompt ("The capital of France is" -> "Paris"; coherent MoE
reasoning output).
WalkthroughChangesQwen 3.5/3.6 GGUF support
Sequence Diagram(s)sequenceDiagram
participant GGUFArchive
participant Model
participant Qwen35Loaders
participant Qwen35Graph
GGUFArchive->>Model: provide mapped tensors and GGUF metadata
Model->>Qwen35Loaders: load experts and transformed small weights
Qwen35Loaders->>Qwen35Graph: provide configured Qwen 3.5/3.6 tensors
Qwen35Graph->>Qwen35Graph: expand Q/K according to V-head ordering
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@driver/portable/src/model.cpp`:
- Around line 2250-2293: The GGUF type constants in read_small_weight_as_f32_
are incorrect, causing BF16 tensors to be rejected. Replace the local
TF32/TF16/TBF16 numeric enum and switch cases with the existing GGML_TYPE_F32,
GGML_TYPE_F16, and GGML_TYPE_BF16 constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ed4fee8b-322a-46b1-b196-278518d4abb0
📒 Files selected for processing (8)
driver/portable/src/gguf_archive.cppdriver/portable/src/gguf_archive.hppdriver/portable/src/gguf_hparams.cppdriver/portable/src/graph_qwen3_5.cppdriver/portable/src/hf_config.hppdriver/portable/src/model.cppdriver/portable/src/model.hppdriver/portable/src/safetensors.hpp
Comment on lines
+2250
to
+2293
| // Read a small source weight (BF16 safetensors or BF16/F16/F32 GGUF) as a | ||
| // flat F32 vector. Used by the Qwen 3.5 synth helpers below, which need the | ||
| // raw float values to invert convert-time transforms. The element count is | ||
| // derived from the source dtype so it works regardless of how the tensor was | ||
| // stored. Throws on an unexpected (quantized) source. | ||
| static std::vector<float> read_small_weight_as_f32_(const StTensor& src, | ||
| const std::string& hf_name) { | ||
| // GGUF tensors carry their ggml type in `ggml_type_override`; safetensors | ||
| // leaves it -1 and uses `dtype`. | ||
| const bool is_gguf = src.ggml_type_override >= 0; | ||
| enum { TF32 = 0, TF16 = 1, TBF16 = 24 }; // ggml_type values | ||
| std::size_t elem_bytes; | ||
| int kind; // 0=f32, 1=f16, 2=bf16 | ||
| if (is_gguf) { | ||
| switch (src.ggml_type_override) { | ||
| case TF32: elem_bytes = 4; kind = 0; break; | ||
| case TF16: elem_bytes = 2; kind = 1; break; | ||
| case TBF16: elem_bytes = 2; kind = 2; break; | ||
| default: | ||
| throw std::runtime_error( | ||
| "model qwen3_5: expected an unquantized small weight for " | ||
| + hf_name); | ||
| } | ||
| } else if (src.dtype == StDtype::BF16) { | ||
| elem_bytes = 2; kind = 2; | ||
| } else if (src.dtype == StDtype::F32) { | ||
| elem_bytes = 4; kind = 0; | ||
| } else { | ||
| throw std::runtime_error( | ||
| "model qwen3_5: unsupported small-weight dtype for " + hf_name); | ||
| } | ||
| const std::size_t n = src.nbytes / elem_bytes; | ||
| std::vector<float> out(n); | ||
| if (kind == 0) { | ||
| std::memcpy(out.data(), src.data, n * sizeof(float)); | ||
| } else if (kind == 2) { | ||
| const auto* p = reinterpret_cast<const std::uint16_t*>(src.data); | ||
| for (std::size_t i = 0; i < n; ++i) out[i] = bf16_to_f32(p[i]); | ||
| } else { // f16 | ||
| const auto* p = reinterpret_cast<const std::uint16_t*>(src.data); | ||
| for (std::size_t i = 0; i < n; ++i) out[i] = ggml_fp16_to_fp32(p[i]); | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Searching for ggml_type enum and override usage...\n'
rg -n "GGML_TYPE_BF16|ggml_type_override|enum ggml_type|typedef enum ggml_type|case GGML_TYPE_BF16|TBF16|GGML_TYPE_I8" driver/portable/src . -g '!**/node_modules/**' -g '!**/build/**' -g '!**/dist/**' || trueRepository: pie-project/pie
Length of output: 9350
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Intentionally empty placeholder to keep the request minimal.
PYRepository: pie-project/pie
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Locating ggml_type enum definition...\n'
fd -a 'ggml.h' . || true
printf '\nSearching for enum members in likely headers...\n'
rg -n "GGML_TYPE_F32|GGML_TYPE_F16|GGML_TYPE_BF16|GGML_TYPE_I8|typedef enum ggml_type|enum ggml_type" . -g '*.h' -g '*.hpp' -g '*.c' -g '*.cpp' | head -n 200
printf '\nInspecting gguf_archive.cpp around type assignment...\n'
sed -n '260,350p' driver/portable/src/gguf_archive.cppRepository: pie-project/pie
Length of output: 18323
🌐 Web query:
GGML_TYPE_BF16 enum value ggml_type BF16 24 30 ggml.h
💡 Result:
In the GGML library, the enum constant GGML_TYPE_BF16 is defined with the value 30 [1][2][3]. This definition is part of the ggml_type enumeration found in ggml.h, which maps various data types (including floating-point, integer, and quantized formats) to specific integer values [1][2]. For reference, other types defined in the same enumeration include GGML_TYPE_I8 at 24 and various other types, which are organized to maintain backward compatibility [2][3]. The library uses this enumeration to manage tensor data types during operations [1][4].
Citations:
- 1: https://ggml-org-ggml.mintlify.app/concepts/tensors
- 2: https://github.com/ggml-org/llama.cpp/blob/74ade527/ggml/include/ggml.h
- 3: https://github.com/ggml-org/llama.cpp/blob/1191758c/ggml/include/ggml.h
- 4: https://github.com/ggml-org/llama.cpp/blob/f486ce9f/ggml/src/ggml.c
Use GGML_TYPE_BF16 here, not 24
ggml_type_override already stores the real GGUF type. 24 is GGML_TYPE_I8, so a BF16 small-weight tensor will fall into the default: path and fail to load. Switch on GGML_TYPE_F32 / GGML_TYPE_F16 / GGML_TYPE_BF16 directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@driver/portable/src/model.cpp` around lines 2250 - 2293, The GGUF type
constants in read_small_weight_as_f32_ are incorrect, causing BF16 tensors to be
rejected. Replace the local TF32/TF16/TBF16 numeric enum and switch cases with
the existing GGML_TYPE_F32, GGML_TYPE_F16, and GGML_TYPE_BF16 constants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix loading errors when loading Qwen 3.5/3.6 GGUF.
Summary by CodeRabbit
New Features
Bug Fixes