Skip to content

fix: select the text-model family by prefix instead of one exact string - #686

Merged
waybarrios merged 2 commits into
waybarrios:mainfrom
janhilgard:fix/gemma4-textmodel-dispatch
Aug 11, 2026
Merged

waybarrios merged 2 commits into
waybarrios:mainfrom
janhilgard:fix/gemma4-textmodel-dispatch

Conversation

@janhilgard

Copy link
Copy Markdown
Collaborator

Fixes the first half of #685.

Problem

_import_text_model_classes matched model_type == "gemma4_text" exactly and defaulted everything else to qwen3_5.TextModel:

gemma4_text            -> mlx_lm.models.gemma4_text.Model
gemma4_unified_text    -> mlx_lm.models.qwen3_5.TextModel     # <-- wrong
gemma4_unified         -> mlx_lm.models.qwen3_5.TextModel     # <-- wrong
anything-else          -> mlx_lm.models.qwen3_5.TextModel

Gemma 4 reports gemma4_text on some checkpoints and gemma4_unified_text on others, so the latter reached qwen3_5.TextModelArgs, which leaves num_experts as None, and construction died on args.num_experts > 0:

TypeError: '>' not supported between instances of 'NoneType' and 'int'

A wrong guess does not fail where it is made. It fails deep inside the chosen constructor with an error naming neither the model nor the class, build_text_model catches it and returns None, and the engine then reports itself loaded with _text_model=None. The whole visible trace is:

ERROR:vllm_mlx.text_model_from_vlm:Failed to build TextModel from vlm: '>' not supported between instances of 'NoneType' and 'int'

That reads like a warning rather than a route losing its backend, which is why it sat there.

Changes

  • Families are matched by prefix, longest first, so one entry covers a family's variants instead of needing a line per checkpoint spelling.
  • The generic fallback stays. qwen3_5.TextModel handles dense and MoE natively, and making unknown types raise would be a regression for every model that works through it today — but the fallback is now logged instead of silent.
  • A failed build names the model_type and the class that was selected, with exc_info=True so the traceback survives.

Verification

gemma-4-12B-it (text_config.model_type: gemma4_unified_text, mlx_vlm conversion, M3 Ultra):

before after
build_text_model None + TypeError builds
engine._text_model None mlx_lm.models.gemma4_text

Repo suite: 2305 passed (four local failures reproduce on pristine main — three Python 3.14, one missing ffmpeg). Confirmed by mutation: restoring the exact-match dispatch fails four of the new tests, including the gemma4_unified_text case specifically.

Scope: this does not fix the other half of #685, and I want to be clear about why

#685 also reports stream_generate(prompt=...) returning empty chunks, and in the issue I guessed that the missing chat template on the mlx_vlm fallback was to blame. That guess was wrong and I have corrected the issue.

_stream_generate_impl calls the mlx_vlm path regardless of whether a TextModel exists, so a working TextModel does not change that route. More to the point, I drove mlx_lm against the now-correctly-built Gemma 4 TextModel with the same raw prompt:

backend raw prompt output
mlx_vlm path '-..1.1______'
mlx_lm + Gemma 4 TextModel '-......1.1___-______'
either, with the chat template applied 'The three primary colors are:\n\n1. **Red**'

Both backends agree, so this is an instruct-tuned model being handed a bare completion prompt — which is exactly what /v1/completions is for. Applying a chat template inside stream_generate would break that endpoint's semantics for every VLM, so I have deliberately left it alone.

The one thing I still cannot explain is why the engine surfaces empty text where a direct call on the same model instance surfaces the garbage above; the leading chunks are empty in both and the pump stops at completion_tokens >= max_tokens, which accounts for part of it but not the differing chunk counts. Both are wrong outputs from a wrong-shaped prompt, so I have left that noted in #685 rather than guessing at a fix.

`_import_text_model_classes` matched `model_type == "gemma4_text"` exactly and
sent everything else to `qwen3_5.TextModel`. Gemma 4 reports `gemma4_text` on
some checkpoints and `gemma4_unified_text` on others, so the latter reached
`qwen3_5.TextModelArgs`, which leaves `num_experts` as None, and construction
died on `args.num_experts > 0`:

    TypeError: '>' not supported between instances of 'NoneType' and 'int'

A wrong guess does not fail where it is made. It fails deep inside the chosen
constructor with an error naming neither the model nor the class,
`build_text_model` catches it and returns None, and the engine goes on to
report itself loaded with `_text_model=None` — a route quietly losing its
backend while the log shows one line that reads like a warning.

Families are now matched by prefix, longest first, so one entry covers a
family's variants. The generic fallback stays: `qwen3_5.TextModel` handles
dense and MoE natively, and making unknown types raise would be a regression
for every model that works through it today. It is now logged rather than
silent, and a failed build names both the `model_type` and the class that was
selected, with the traceback preserved.

Verified on `gemma-4-12B-it` (`text_config.model_type: gemma4_unified_text`,
mlx_vlm conversion, M3 Ultra): the TextModel now builds as
`mlx_lm.models.gemma4_text` and the error is gone.

Scope note, since waybarrios#685 reports two symptoms: this fixes the dispatch, and it
does **not** by itself change the empty-chunk symptom on
`stream_generate(prompt=...)`. That route calls the mlx_vlm path regardless of
whether a TextModel exists, and the raw prompt is the real problem there — I
measured `mlx_lm` driving the correctly-built Gemma 4 TextModel with the same
raw prompt and it produces the same garbage (`'-......1.1___-______'`) as the
mlx_vlm path. So it is an instruct model being handed a bare completion
prompt, which is what `/v1/completions` is meant to do, not a routing bug.
Applying a chat template inside `stream_generate` would break that endpoint's
semantics, so I have left it alone and corrected the issue.

Repo suite: 2305 passed. Confirmed by mutation — restoring the exact-match
dispatch fails four of the new tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@Thump604 Thump604 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right split. The family dispatch now handles Gemma 4 variants without changing the existing fallback, and failures identify the selected class with a traceback. Focused dispatch/build tests pass at the exact head. The remaining blank-chunk behavior stays correctly scoped to #685.

@waybarrios

Copy link
Copy Markdown
Owner

I noticed the new test file was missing from the Apple Silicon CI list, so I added it. This way the Gemma dispatch tests will actually run on every PR. Small oversight, but it’s covered now.

@waybarrios
waybarrios merged commit 2387b0c into waybarrios:main Aug 11, 2026
9 checks passed
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 18, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 23, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Aug 27, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Sep 23, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TimotejLabsky added a commit to TimotejLabsky/vllm-mlx that referenced this pull request Sep 25, 2026
…ng, wanted deltas, stream-global restore

engine/simple.py was kept wholesale over upstream's c65c356/d7c1c98
rewrites (PATCHES.md 2026-08-17 rebase note). This commit restores the
upstream deltas we do want and reconciles the plumbing that assumed
upstream's engine:

- accept upstream waybarrios#574's prefix_trie_cache* kwargs fail-closed (raise if
  enabled — the fork's system-KV supersedes the trie; no silent no-op)
- upstream waybarrios#681: stamp finish_reason=length at the token-budget cutoff
  when the backend chunk carries None
- upstream waybarrios#686 folded into fork semantics: _import_text_model_classes
  candidate chain with gemma4/qwen3 family rules; unknown families raise
  instead of guessing qwen3_5
- engine_core: keep upstream's owns_worker teardown guard AND the fork's
  waybarrios#49 SSD flush; getattr-guard close_ssd_tier for duck-typed schedulers
- fix(streams): snapshot the pre-bind generation-stream globals at first
  worker bind and restore them in SimpleEngine.stop() — a retired worker
  otherwise leaves mlx_lm/mlx_vlm generation_stream naming a dead
  thread's stream (surfaced by upstream waybarrios#702's parity tests)

Co-Authored-By: Claude Fable 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.

3 participants