qwen3-8b: Q8_0 e2e on lexie - #13
Conversation
Adds qwen3-8b to the variant registry (Qwen3-8B dir name, GGUF name). The factory now derives Config from config.json on disk, so untied lm_head (tie_word_embeddings=false on 8B) flows through naturally. Wires sk_qwen_load_safetensors_index from libsk.dylib so the python loader picks the sharded entry point when model.safetensors.index.json is present; falls back to the single-file path otherwise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The prior 8B registration defaulted to bf16 safetensors, which sends 16 GB through the python-driven copy_into loop (10-15+ min per load on lexie/derek). Switch the 8B factory to default quant="q8_0" so it mmaps the GGUF via the existing sk_qwen_load_gguf path (matches the 0.6B fast path). - _VARIANT_DIMS hardcodes Qwen3-0.6B/8B dims so Config can be built without an on-disk config.json (Q8_0-only deployments commonly skip the snapshot). - _resolve_tokenizer falls back to huggingface_hub.hf_hub_download for tokenizer.json when the snapshot dir lacks one. - tie_word_embeddings=0 baked into the 8B variant dims (untied LM head); the existing C++ sk_qwen_load_gguf already reads output.weight in this case (weights.c++:374). - 8B snapshot dir name now resolves to Qwen3-8B-GGUF to match the hf download layout used on lexie. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afd8b866a6
ℹ️ 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".
| if idx_path.exists(): | ||
| if not hasattr(lib, "sk_qwen_load_safetensors_index"): | ||
| raise RuntimeError("libsk.dylib has no sk_qwen_load_safetensors_index symbol; rebuild dylib") | ||
| rc = lib.sk_qwen_load_safetensors_index(m._h, str(idx_path).encode()) |
There was a problem hiding this comment.
Bind safetensors-index ABI before calling via ctypes
When model.safetensors.index.json is present, this path calls lib.sk_qwen_load_safetensors_index(...) directly, but that symbol is not declared in QWEN_ABI (see SuperKittens/models/qwen/qwen.py), so bind() never assigns argtypes/restype. In ctypes, untyped calls default to int argument conversion, which can mis-handle the model handle pointer on 64-bit builds and cause load failures or crashes for sharded safetensor checkpoints. Add load_safetensors_index to QWEN_ABI (with the same signature as in C) before using this call path.
Useful? React with 👍 / 👎.
Lazarus-931
left a comment
There was a problem hiding this comment.
remove all 0.6b mentions
| n_layers=28, d_model=1024, n_heads=16, n_kv_heads=8, head_dim=128, | ||
| n_int=3072, vocab_size=151936, eps=1e-6, rope_freq_base=1_000_000.0, | ||
| tie_word_embeddings=1, | ||
| ), |
There was a problem hiding this comment.
rmv, we will nvr use 0.6b.
Also I thinkt this model dims needs to be centralized, not model wise, what do you think?
|
|
||
| _VARIANT_TO_DIR = { | ||
| "qwen3-0.6b": "Qwen3-0.6B", | ||
| "qwen3-8b": "Qwen3-8B-GGUF", |
There was a problem hiding this comment.
yeah same here, remove all mentions of 0.6b
…llapse 4 parallel dicts into one spec table Two changes addressing PR #13 review: 1. Add `load_safetensors_index` to QWEN_ABI (qwen.py) so the central binder sets argtypes/restype. Previous code called `lib.sk_qwen_load_safetensors_index` directly without registering the signature — ctypes default int coercion would corrupt the handle pointer on 64-bit builds. 2. Drop 0.6b mentions per review. Consolidate the four parallel per-variant dicts (_VARIANT_DIMS, _VARIANT_TO_DIR, _VARIANT_TO_GGUF, _VARIANT_TO_HF_REPO) into a single _QWEN3_VARIANTS spec table with one row per variant. Adding a new qwen3 size now means one entry, not five. This is the same anti-pattern as the dropped 0.6b lit-everywhere: family-specific knowledge funnels through ONE seam (QWEN_ABI for ctypes, _QWEN3_VARIANTS for per-variant metadata) instead of leaking across files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sharded-safetensors load path calls sk_qwen_load_safetensors_index but the symbol was not registered in QWEN_ABI, so the central binder never set argtypes/restype. On 64-bit builds ctypes' default int coercion corrupts the handle pointer and the call segfaults or silently fails. Add it as an optional entry (back-compat with older dylibs that lack the symbol). This is the only novel content remaining from PR #13 after PR #14's registry absorbed the qwen3-8b variant + Q8_0 default. PR #13 will be closed. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
qwen3-8bin the qwen factory: hardcoded HF config.json dims (n_layers=36, d_model=4096, n_heads=32, n_kv_heads=8, head_dim=128, n_int=12288, vocab_size=151936, tie_word_embeddings=0), plus on-diskconfig.jsonderivation when a snapshot is present.sk.load("qwen3-8b")to the Q8_0 GGUF path. Derek (M4, 16 GB unified) cannot fit the 16 GB fp16 weights + activations + KV cache; the bf16 safetensors load OOM'd into swap and stuck. Q8_0 GGUF (~8 GB) fits with headroom, and the existing qwen GGUF loader already handles untied lm_head (tie_word_embeddings == falsetriggersoutput.weightinstead oftoken_embd.weight).sk_qwen_load_safetensors_indexctypes binding so the python loader picks the sharded entry point whenmodel.safetensors.index.jsonis present (used by 8B's 5-shard layout if the snapshot ever fits).Validation (derek, Q8_0 GGUF)
Both produce coherent English. The leading
d\n</think>\n\nis the Qwen3 thinking-mode tag (same artifact present in the 0.6B baseline path).Argmax probe
Input:
"Hi!"→ tokens[13048, 0](tokenization matches HF reference exactly).SK argmax of next-token logits: 358 →
' I'(a sensible Qwen3 continuation for a greeting). Full HF logit parity check skipped — the 8B fp16 weights aren't available on derek due to disk and the prompt/paragraph outputs are coherent.Decode performance (derek M4, Q8_0)
llama-bench tg64llama-bench pp64SK is at ~90% of llama.cpp's decode tok/s on the same Q8_0 weights and same hardware.
Notes on 8B-specific bugs / surprises
cfg.tie_word_embeddingsfor both safetensors and GGUF paths, andlauncher.c++allocates thew_lm_headbuffer when untied — no kernel changes needed.sk_qwen_load_gguf(and the newsk_qwen_load_safetensors_index). Rebuilt locally with./build.sh(Xcode 16) and scp'dlibsk.dylib+libsk.metallibtoderek:~/SuperKittens/build/.Test plan
sk.load("qwen3-8b")returns a Model with weights loaded and tokenizer attachedquant=Nonedefault; 8B opted intoquant="q8_0"via registry defaults)Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com