Skip to content

test: make the MoE fixture actually test nesting, and fix two harness faults - #145

Merged
solderzzc merged 1 commit into
mainfrom
fix/fixture-review-followups
Aug 14, 2026
Merged

test: make the MoE fixture actually test nesting, and fix two harness faults#145
solderzzc merged 1 commit into
mainfrom
fix/fixture-review-followups

Conversation

@solderzzc

Copy link
Copy Markdown
Member

Review follow-ups on #143/#144. All three findings were in code I wrote; the first is the substantive one.

1. 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, and the nested copy underneath it was never reached. findExpertCounts is breadth-first and returns on the first hit.

Both advertised properties were dead weight:

  • the count being "nested under text_config rather than at top level"
  • the vision_config: {num_experts: 999} decoy that "must not be mistaken"

Deleting the nested-container walk entirely would not have failed it.

Rebuilt on gemma4

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 exactly what made the qwen3_moe version untestable.

Red-green verified — the check the previous version could not pass:

detection result
pre-#114 (top-level num_local_experts only) fails5 passed, 1 failed
current passes — 6 passed, 0 failed

The fixture now reproduces #112.

The assertion also moved to the right gate

There are two gates, and I had been asserting on the wrong one. A config-level MoE check, and a model-level StreamableMoE conformance check. gemma4 passes the first and legitimately declines the second (it has no expert-streaming implementation), so asserting "streaming enabled" would have tested an unrelated capability. It now asserts only that detection did not reject — which is what #112 was about.

Incidentally this also 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 there is a silent numerical fault.

2. Port reuse could produce a false pass

cleanup() killed the server without waiting, and the readiness loop probed health before checking liveness. If a teardown outlived the 1 s 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 before the probe in both loops.

3. Regeneration was not idempotent

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 and are not generated here. Verified by planting a stale file, regenerating, and confirming it is gone while omni is untouched.

Verification

Suite green twice consecutively: 6 passed, 0 failed.

🤖 Generated with Claude Code

… faults

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>
@solderzzc
solderzzc merged commit a08ffd1 into main Aug 14, 2026
14 checks passed
@solderzzc
solderzzc deleted the fix/fixture-review-followups branch August 14, 2026 19:06
solderzzc added a commit that referenced this pull request Aug 16, 2026
…e it exposed (#152)

* feat: auto-detect VLM checkpoints on the CLI, and fix a false positive it exposed

Extracted from fix/pr57-speculative-ci rather than a rebase (see #109's
resolution and the discussion around it). Its ModelArchitectureProbe.swift and
tests were already shipped verbatim; only the CLI wiring and its test were
still missing.

**The gap.** Server.swift already probed the architecture at load time but
discarded the result for the CLI entry point: `let isVision = self.vision`.
Confirmed the user-facing effect directly: `SwiftLM --model
LiquidAI/LFM2.5-VL-450M-MLX-4bit` (no --vision) printed "Loading LLM" and the
subsequent image request failed. InferenceEngine.swift — SwiftBuddy's own
loader — already auto-detects unconditionally; the CLI now mirrors that,
keeping --vision/--audio as explicit overrides:

    let isVision = self.vision || (!self.audio && architecture.supportsVision)

Verified both directions: LFM2.5-VL-450M-MLX-4bit now loads as a VLM and
answers an image request without --vision; a plain LLM (Qwen2.5-0.5B) still
loads as LLM with no flags; --vision still forces VLM loading explicitly.

**What wiring this up exposed.** The fixtures suite broke: `moe-nested`
started failing with "Key vision_tower.patch_embedder... not found", because
that fixture's `model_type: "gemma4"` — carried since #145, for the earlier,
narrower reason that Gemma4Configuration decodes text_config only, giving the
MoE-nesting test a real nested config to exercise — was now read by the CLI's
newly-active auto-detection and routed to VLMModelFactory.

That is not a fixture bug on its own. `MLXLLM/Gemma4.swift`'s
Gemma4Configuration (the LLMModelFactory "gemma4" entry) has no vision_config
field at all — a real, intentionally-supported text-only checkpoint shape.
ModelArchitectureProbe.knownVisionModelTypes listed bare "gemma4" as
vision-triggering regardless, which is a genuine false positive for any real
text-only Gemma4 checkpoint using that shape, not just this fixture — auto-
detection would have broken loading one in production. Removed "gemma4" from
that list; the separate `vision_config != nil` check already distinguishes
the two correctly, since only VLMModelFactory's Gemma4Configuration requires
that field.

Also swapped the fixture's decoy container from vision_config to audio_config
— unrelated to the bug above, but vision_config as a decoy was already the
wrong choice: a real text-only Gemma4 checkpoint would never carry that key,
so the fixture is more honest this way regardless of the probe fix.
audio_config is excluded from the expert-count walk by
ModelProfiler.nonLanguageContainers the same way vision_config was, so the
decoy still proves what it proved before.

Regression test added: testVLM_AutoDetectsLFM25WithoutVisionFlag in
tests/SwiftBuddyTests/VLMTests.swift, adapted from fix/pr57-speculative-ci to
match main's already-refactored captureStartupOutput helper (main had the
refactor; only this second test case was missing). Red-green verified: fails
with "Unsupported model type: lfm2-vl" against the pre-fix isVision line,
passes once restored.

Verified together: fixtures 6/0, contract 10/0/2, both VLMTests cases,
LFM2.5-VL-450M-MLX-4bit auto-detects, Qwen2.5-0.5B does not false-positive,
explicit --vision still works.

Refs #109

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix: don't auto-detect vision when speculative/MTP decoding is requested

CI's speculative-decoding suite started crashing (Trace/BPT trap on the
first real generation request) after auto-detection began routing
mlx-community/Qwen3.5-*-4bit through VLMModelFactory instead of
LLMModelFactory. That checkpoint genuinely ships a vision_config even
when used purely as a text draft/main pair in this test, and qwen3_5 is
registered in both factories — the same ambiguity as gemma4, just
surfaced through a different flag combination.

--draft-model/--dflash/--mtp only wire up BaseLanguageModel from
LLMModelFactory, so auto-detection now backs off whenever speculative
decoding is requested, matching the pre-auto-detect behavior for that
path. --vision remains a valid explicit override.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant