fix: detect MoE across all expert-count config spellings (#112) - #114
Conversation
ModelProfiler decoded the routed-expert count from `num_local_experts`
only — the Mixtral spelling. Qwen3/Qwen3.5 MoE use `num_experts`, and
DeepSeek V2/V3, GLM MoE and MiniMax use `n_routed_experts`. Those models
profiled as dense, so Server.swift dropped --stream-experts ("qwen3_5_moe
is not MoE"), never set modelConfig.lazyLoad, and the full model was
materialised into RAM — which the OS then OOM-killed. This is why the
reporter's Qwen3.5-397B-A17B run worked on b648 but not on current main.
- Decode all three spellings, plus the same keys nested under text_config
for multimodal wrappers, and resolve them in ModelConfig.numExperts.
- Fall back to a model_type heuristic when no known key is present. The
asymmetry is deliberate: a false positive costs a weight-file walk, a
false negative costs an OOM kill.
- Keep the >1 threshold so a single-expert (dense FFN) config stays dense.
- Name the keys we looked for in the rejection log so the next
misdetection is diagnosable from the log alone.
Tests cover one case per spelling (including the exact model from the
issue), the nested text_config form, the unknown-key fallback, and
negative cases for dense / single-expert / missing config. Verified red
against the pre-fix logic: 7 of the 9 fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Runtime validation on a MacBook Pro M4 Pro with 24 GB unified memory:
Validated command: .build/release/SwiftLM \
--model lmstudio-community/Qwen3.6-35B-A3B-MLX-8bit \
--stream-experts \
--turbo-kv \
--vision \
--port 5413 |
Gwada's runtime validation on PR #114 used lmstudio-community/Qwen3.6-35B-A3B-MLX-8bit, a multimodal MoE whose top level carries no expert keys at all — only vision_config and a text_config holding num_experts: 256. Confirmed against the cached Qwen3.6-35B-A3B config.json: model_type is qwen3_5_moe (matching the reported "qwen3_5_moe is not MoE" log) and every expert and dimension key lives under text_config. The existing nested-config test used a synthetic shape. This pins the real one, including the 40-layer count from the validated run. Asserting the expert counts rather than just isMoE is what makes it meaningful: model_type contains "moe", so the heuristic fallback alone would mark the model as MoE while leaving the counts nil. Verified red with the nested lookup removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks @Gwada — that's a more demanding case than the one in the issue, and I've now pinned it in a test (95ba329). I checked your config against the cached So this model needs both halves of the fix: the Worth noting for anyone reading the diff: The 37.7 GB model running at ~4 tok/s on 24 GB unified memory is a much better demonstration of the streaming path than anything I could produce locally. Much appreciated. One clarification, since it affects what your result proves: was this PR's branch as-is, or your own patch? You mention "2 tests" and this branch carries 9 (now 10), so I want to be sure the validated code is what's up for merge here. |
|
Reproduced @Gwada's runtime validation independently, on Same command, same binary, only .build/release/SwiftLM --model <local snapshot> --stream-experts --turbo-kv --vision --port 5413Before ( After (this branch): The bogus weight figure reproduces too — 1.5 GB for the 4-bit here against Gwada's ~3 GB for the 8-bit, which is the expected 2× ratio. It comes from
Two caveats on my run, since they bound what it proves: this box has 68.7 GB of RAM, so streaming activates but is never memory-pressured the way Gwada's 24 GB machine exercised it — their throughput number is the meaningful one, not mine. And I could not test the exact 8-bit build: it is 37.7 GB against 39 GiB free here, so I used the 4-bit of the same model rather than fill the disk. Detection, streaming activation, and generation all confirmed on a second machine. |
|
Answering my own question above: #117 makes it clear the validation was of @Gwada's own patch, not this branch — which is exactly why their runtime result was useful, so no concern there. Recording how the two fixes relate, since #117 is now closed and the reasoning shouldn't disappear with it.
The third row is the one worth calling out: #117 left Credit where it is due: @Gwada found the nested For the record on this branch specifically: I ran |
|
Thanks for the detailed follow-up and for reproducing the result independently. I originally came across SwiftLM because I wanted to try the project, and I was genuinely pleasantly surprised — it is a really good tool. I tested with a smaller model because my Mac cannot reasonably handle anything larger. I only noticed #114 a few minutes after publishing my own PR. Otherwise, I would have added my findings here directly. I will keep following the project, and I am really looking forward to future improvements, especially further performance gains. Thanks again for the work! |
- An explicit expert count is now authoritative in both directions. The model_type heuristic was OR'd with the count, so a config declaring one expert still profiled as MoE if its name contained "moe" — contradicting both the doc comment and the stated ">1 threshold is preserved". It now applies only when no known key was present at all. - Resolve the count by preferring a plausible value over a degenerate one. The nil-coalescing chain let a top-level "num_experts": 0 placeholder in a multimodal wrapper mask a real count nested under text_config. - Correct the comment on modelTypeImpliesMoE: a false positive costs more than a weight-file walk. It clears the Server.swift guard, which enables lazy loading, activates SSD streaming and overrides the MLX memory limits. The asymmetry argument still holds — that path only runs under --stream-experts — but the next reader should not widen the heuristic on a false premise. - The rejection log claimed no count was found even when one was found and was <= 1; it now reports the actual count. - Drop a no-op hyphen normalization in the heuristic. Two tests added: an explicit count of 1 with a moe-ish model_type stays dense, and a degenerate top-level value does not shadow a nested count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review follow-up on d1f73df. Three related resolution defects: - A lone explicit 0 suppressed the model_type heuristic entirely, re-opening the #112 OOM path for a wrapper whose real count uses a spelling we do not know. Non-positive candidates are now dropped before resolution, so a 0 leaves the count absent and the heuristic still applies. - Preferring any value > 1 let a stale nested count beat an authoritative outer 1 from a dense conversion. With placeholders filtered out, resolution is back to plain precedence order: the first positive value wins. - activeExperts now resolves through the same filter instead of a raw nil-coalescing chain, so a top-level 0 placeholder cannot shadow the real nested per-token count. Tests: a lone 0 keeps the heuristic alive, and an explicit outer 1 beats a stale nested 128. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…esh races (#125) * fix: model-discovery review follow-ups Deferred LOW findings from the #116 review, now addressed. - The CLI server still built ModelConfiguration(id:) for anything that was not an explicit filesystem path, so `SwiftLM --model org/name` re-downloaded a model already present in a hand-copied or huggingface-cli layout — the same bug #116 fixed for the app, on the surface it did not cover. It now reuses ModelStorage.localLoadDirectory(for:). Verified end to end: a plain org/name folder under HF_HUB_CACHE logs "Loading from local cache" and generates, with no download. - rejectionReason reported a weights problem for directories that fail on metadata. validateModelFiles rejects a zero-byte config.json or tokenizer.json before it looks at weights, so a user with an empty config.json was told to inspect an index.json that never existed. Metadata is now checked first, and a too-small single-file model gets its own message instead of the index.json one. - refreshInBackground had no coalescing: a scan started before a delete could land after it and re-add the deleted model. Refreshes now carry a generation stamp, and any newer refresh — background or synchronous — cancels and invalidates the in-flight scan. Tests: localLoadDirectory across all hand-copied layouts, nil for the materialized layout (which must keep the id-based flow), and nil when absent or failing verification. Verified red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: find expert counts in any nested config container Deferred LOW findings from the #114 review. Expert counts were read from the top level and `text_config` only. Two real shapes were missed, both confirmed against the published configs: - deepseek-ai/deepseek-vl2-tiny puts n_routed_experts under `language_config`, and its model_type ("deepseek_vl_v2") contains no "moe" — so neither the key lookup nor the name heuristic caught it. Fully misdetected as dense. - Qwen/Qwen3-Omni-30B-A3B-Instruct nests two levels down, under thinker_config.text_config. Replaced the fixed two-level lookup with a breadth-first walk over nested containers. Shallowest wins, so an outer explicit count stays authoritative over one nested deeper; non-positive values are still skipped as placeholders; and the active count is paired with the container its total came from. Sibling containers are visited in a deterministic order — known language-model container names first, then alphabetically. Qwen3-Omni carries a count under both talker_config and thinker_config with *different* active counts (6 vs 8), so an unordered walk would have reported a different number run to run. Also log the config.json decode error instead of swallowing it: returning nil with no explanation is exactly the diagnosability failure that made #112 hard to pin down in the first place. Tests cover both real shapes, the thinker-vs-talker ordering (repeated to catch nondeterminism), and outer-beats-nested at depth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address review findings; bump submodule for the e2b regression fix Review of this PR found the headline fix did not cover the main case, plus a regression already shipped in b674. - The CLI's HubApi is rooted at Application Support, not ModelStorage.cacheRoot, so localLoadDirectory's "materialized layouts are resolved by HubApi" guard — true for the app — meant a model the app had downloaded was still invisible to the CLI and fetched again. The CLI now asks for any validated on-disk copy. The earlier verification used HF_HUB_CACHE, the one layout that dodged this. - localLoadDirectory could return a directory that exists but is not the one that validated, handing a caller with no download fallback a broken path. Added validatedContentDirectory. - --stream-experts silently no-opped on the newly-supported layouts: resolveModelDirectory knows none of them, so modelDirectory was nil, which skipped both the MoE guard and ExpertStreamingConfig.activate while still setting lazyLoad — lazy weights with no streamer, and no diagnostic. - findExpertCounts no longer descends into encoders (vision/audio/projector), which could outrank the language model's count from a shallower depth, and a container with a total but no per-token count inherits the nearest ancestor's. - Removed the Codable expert plumbing that findExpertCounts superseded; two implementations of the same rule is how #112 came back the first time. - rejectionReason's new branch was unreachable and described a truncated single-file model as sharded. - materializedDirectory now applies the delete guard, so an org-less id is uniformly unsupported rather than loadable-but-undeletable. - Corrected the isSafeModelDirectory comment: resolvingSymlinksInPath only resolves paths that exist, so non-existent ones fail closed. - Dropped a 20x test loop that could not surface the nondeterminism it implied (Swift seeds dictionary hashing per process). Submodule bumped to b320bc4, which carries the fix for gemma-4-e2b-it-4bit — broken since #44 and shipped in b674, because that checkpoint ships K/V weights for its KV-shared layers while e4b does not. Verified on the merge tree: e2b, e4b and Qwen3.6-27B-OptiQ all answer correctly. 151 tests across 11 suites, 0 failures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… faults (#145) Review follow-ups on #143/#144. All three findings were in code I wrote. **The MoE fixture tested nothing it claimed.** It used `qwen3_moe`, whose Swift configuration decodes from the root of config.json, so the expert count had to be at the top level; the nested copy underneath was never reached, because findExpertCounts is breadth-first and returns on the first hit. Both advertised properties — the nesting, and the `vision_config` decoy that "must not be mistaken" — were dead weight. Deleting the nested walk entirely would not have failed it. Rebuilt on `gemma4`, whose Gemma4Configuration decodes `text_config` and nothing else, so the count exists only one level down and the decoy is genuinely reached. `gemma4` also contains no "moe", so modelTypeImpliesMoE cannot rescue it — which is what made the qwen3_moe version untestable. Red-green verified, the check the previous version could not pass: reverting detection to the pre-#114 top-level single-key form fails the new assertion, and restoring it passes. The fixture now reproduces #112. The assertion also moved to the right gate. There are two: a config-level MoE check, and a model-level StreamableMoE conformance check. gemma4 passes the first and legitimately declines the second, so asserting "streaming enabled" would have tested the wrong thing. It now asserts only that detection did not reject. Incidentally covers the fused-expert remap — real gemma4 checkpoints ship `experts.gate_up_proj` as one tensor that sanitize splits into `switch_glu.gate_proj`/`up_proj`. A wrong split axis is a silent numerical fault. **Two harness faults.** cleanup() killed the server without waiting, and the readiness loop probed health before checking liveness. If a teardown outlived the 1s sleep, the next fixture would fail to bind, and its first probe would be answered by the previous server — assertions then run against the wrong checkpoint and report a false pass, not a flake. cleanup now waits, and liveness is checked first. The generator wrote into existing directories without clearing them, so files a builder stopped emitting survived and the fixture kept testing a shape the source no longer described. Each fixture's own directory is now cleared first — scoped to one known directory, which is also what keeps regeneration away from siblings like tests/fixtures/omni, whose assets belong to test-omni.sh. Suite: 6 passed, 0 failed, twice consecutively. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Fixes #112.
Root cause
ModelProfilerdecoded the routed-expert count from a single key:num_local_expertsis the Mixtral spelling. Qwen3/Qwen3.5 MoE usenum_experts; DeepSeek V2/V3, GLM MoE and MiniMax usen_routed_experts— both already known elsewhere in this repo (DeepseekV3DFlash.swift:59,KimiLinearDFlash.swift:83).So
profile.isMoEwas false for Qwen3.5-397B-A17B andServer.swift:347silently dropped--stream-experts. The reporter's own log confirms it:qwen3_5_moe is not MoE.The
zsh: killedfollows directly — with streaming off,modelConfig.lazyLoadis never set (Server.swift:355), so the full 397B model is materialised and the OS OOM-kills the process. That also explains why b648 (before the guard landed) worked.Changes
ModelProfiler.swift— decode all three spellings, plus the same keys nested undertext_configfor multimodal wrappers, resolved inModelConfig.numExperts.modelTypeImpliesMoE()— last-resort fallback for families whose expert key we haven't seen (anymodel_typecontainingmoe, plusmixtral/dbrx/grok). The asymmetry is deliberate: a false positive costs a weight-file walk, a false negative costs an OOM kill.> 1threshold is preserved, so a single-expert (dense FFN) config stays dense.Server.swift:347— the rejection message now names the keys it looked for, so the next misdetection is diagnosable from the log alone.Tests
New
tests/SwiftLMTests/ModelProfilerMoEDetectionTests.swift— 9 tests writing realconfig.jsonfixtures throughModelProfiler.profile(): one per spelling (the Qwen3.5 case reproduces the issue's exact model), nestedtext_config, the unknown-key fallback, and negatives for dense / single-expert / missing config.Verified red against the pre-fix logic — 7 of the 9 fail, including
testQwen35MoEDetectedViaNumExperts. Green after: 9/9.Full
SwiftLMTestsrun: 86 tests across 8 suites, 0 failures.PromptCacheTestsaborts underswift testwithFailed to load the default metallib— reproduced at HEAD without these changes, so it is pre-existing and environmental.Not covered here: the reporter should confirm the actual
--stream-expertsrun on their 397B model, since the OOM path itself is hardware-dependent.🤖 Generated with Claude Code