Fix XTC sampling threshold to be per-row for batched logits - #1575
Merged
Merged
Conversation
apply_xtc computed its masking threshold with a global .min() over all axes. With batched logits of shape [B, vocab] (e.g. the sampler applied to the full batch in batch_generate), the smallest above-threshold probability of any row in the batch became the mask threshold for every row, masking tokens that should have been kept in other rows. Reduce along the last axis with keepdims so the threshold is computed per distribution, matching apply_top_p/apply_top_k/apply_min_p. Adds a regression test asserting that each row of a batched apply_xtc call matches the same row processed alone.
michalk8
self-requested a review
July 21, 2026 15:24
amanyagami
added a commit
to amanyagami/mlx-lm
that referenced
this pull request
Aug 20, 2026
…rsing - models/qwen3.py, models/qwen3_moe.py (ml-explore#1563): transformers >= 5 nests rope config under a `rope_parameters` dict instead of top-level `rope_theta`/`rope_scaling` -- confirmed against the actual installed transformers 5.15.1 (Qwen3Config().to_dict() has no top-level rope_theta at all). Both ModelArgs classes had `rope_theta` as a required field with no fallback, so any newly-published Qwen3(-MoE) config in the new shape raised `TypeError: missing 1 required positional argument: 'rope_theta'`. Added __post_init__ lifting, matching the pattern already used by qwen3_5.py. Verified against real Qwen3Config/Qwen3MoeConfig objects (new-style and old-style configs both load correctly). - models/gemma3n.py (ml-explore#1699): Gemma3nAttention had two bugs on the batched-cache path (BatchKVCache/BatchRotatingKVCache): (1) `keys, values = cache.state` crashes on a 4-tuple state; (2) the queries' RoPE offset was bound before update_and_fetch but applied after -- batch caches keep offset as an mx.array mutated in place by the update, so the offset silently became the post-update value, shifting every query position by the prompt length and producing repetition-loop garbage. Fixed by slicing cache.state[:2] and moving the queries' RoPE application before update_and_fetch, matching keys' timing. Added two regression tests using a real (tiny) gemma3n model: one asserting batch-cache and plain-cache logits match bit-for-bit, proven to fail before the fix (reverted locally and reproduced both the ValueError and, after a minimal state[:2] patch alone, the divergent-logits case); another exercising the shared-KV-layer state unpack directly. - models/cache.py, models/gemma3_text.py (ml-explore#118): make_prompt_cache's generic max_kv_size fallback only applies when a model has no custom make_cache() -- every model with sliding-window attention (gemma3, gemma3n, gemma4, cohere2, ...) defines its own make_cache() with no parameters, so --max-kv-size was silently ignored for all of them, and Gemma 3's RotatingKVCache became permanently untrimmable past its hardcoded sliding_window (1024) regardless of what was requested. make_prompt_cache now forwards max_kv_size to model.make_cache() when its signature accepts it (checked via inspect.signature, so the other ~40 models with a fixed no-arg make_cache are completely unaffected); gemma3_text.Model.make_cache(max_kv_size=None) now caps its sliding layers at min(sliding_window, max_kv_size). A larger max_kv_size can't exceed the model's own trained window, only shrink it. - server.py (ml-explore#1183): add `context_length` to each entry in the /v1/models response (from config.json's max_position_embeddings, falling back to a nested text_config for Qwen3.5/3.6-style configs), so clients can size max_tokens without hardcoding per-model defaults. Verified end-to-end against the real /v1/models endpoint and a real downloaded model's config.json. - tool_parsers/qwen3_coder.py (ml-explore#1627, ml-explore#1236, ml-explore#1604): three related crashes/silent-drops in _convert_param_value, all confirmed against current main with the issues' exact reproductions: * a float-formatted integer arg (e.g. "140.0" for an `integer` schema field) raised ValueError from a bare int(param_value); the model emits this in practice and, combined with server.py dropping failed tool calls silently, produced an infinite generate-parse-fail-retry loop against a fully prompt-cached, deterministic request (observed ~390 iterations before being killed). * ast.literal_eval on an object/array/untyped parameter crashed with SyntaxError/ValueError on any value that isn't valid JSON *and* isn't a valid Python literal (an ISO 8601 timestamp, a large free-text/code argument, truncated JSON) -- this exception was unhandled and propagated out of the HTTP request handler. Both branches (and the float branch, latent but same shape) now fall back to the raw string on failure instead of raising, letting downstream schema validation flag a genuine type mismatch instead of crashing the request. Well-formed values are completely unaffected. No code needed -- already fixed on main or don't reproduce, each verified against real transformers/tokenizer/model artifacts rather than assumed: * ml-explore#939 (SuScaledRoPE/YarnRoPE mutation) -- fixed by ml-explore#1003; the existing test_su_scaled_rope_no_mutation/test_yarn_rope_no_mutation already cover it and pass. * ml-explore#983 (BatchRotatingKVCache.merge shape mismatch) -- fixed by ml-explore#999; merge() now sizes off c.size() instead of the internal _idx pointer. * ml-explore#1041 (Devstral tekken tokenizer emits literal "Ġ") -- downloaded the actual tokenizer.json from the named model; BPEStreamingDetokenizer is correctly selected and decodes byte-level BPE tokens to spaces when driven the way generate.py/server.py actually drive it (one persistent detokenizer instance, not re-fetching the TokenizerWrapper.detokenizer property -- itself a fresh instance -- per token). * ml-explore#1257 (XTC sampling crash on gemma-4-e4b) -- downloaded and ran the full named model with the issue's exact XTC request; generates correctly. Fixed by ml-explore#1575 (per-row XTC threshold for batched logits), landed after this issue was filed. * ml-explore#1126 (Gemma 4 tool call regex on unbalanced braces in strings) -- fixed by ml-explore#1150; the current regex has a dedicated <|"|>...<|"|> string-literal alternative that consumes braces inside strings atomically before the recursive brace-balancing kicks in. Verified against both of the issue's exact failing inputs. Testing: full suite re-run after these changes -- test_models.py (91 passed, 3 skipped, 0 failed), test_server.py (35/35, including a live end-to-end check of the new context_length field against a real downloaded model), test_tool_parsing.py (6/6), test_sample_utils.py. Zero regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pierre427
pushed a commit
to pierre427/mlx-lm
that referenced
this pull request
Aug 21, 2026
Brings in: XTC special-token/per-row fixes + 0.1 default (ml-explore#1176/ml-explore#1575/ml-explore#1372), upstream laguna support (ml-explore#1334), seed=0 fix in iterate_batches (ml-explore#1661), Falcon-Mamba long-prompt fix (ml-explore#1656), nvfp4 compressed-tensors loading. Conflict resolutions: - models/laguna.py: kept our implementation (superset: compressed-tensors unpack, expert stacking, router remap, quant/cast predicates, sink support, sigmoid/softmax toggle) and consolidated upstream's official config dialect into it: new mlp_layer_types field takes precedence over the mlp_only_layers/decoder_sparse_step fallback for dense/sparse placement. Upstream's test_laguna case in test_models.py is kept as the arbiter. - tests/test_server.py: union — upstream's TestMakeSampler XTC test added alongside our TestLogitBiasValidation; import block merged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Problem
apply_xtcinmlx_lm/sample_utils.pycomputes its masking threshold with a global.min()over all axes:Every other filter in the sampler chain (
apply_top_p,apply_top_k,apply_min_p) reduces alongaxis=-1, so samplers produced bymake_samplerare expected to vectorize over a leading batch axis — and they are called that way on the full[B, vocab]tensor viabatch_generate(GenerationBatch._step->self.fallback_sampler(logprobs)).With
B >= 2, the smallest above-threshold probability of any row in the batch becomes the mask threshold for every row. Example withxtc_threshold=0.2:[0.6, 0.25, 0.1, 0.05]— above-threshold min is 0.25, so 0.6 is correctly masked.[0.9, 0.05, 0.03, 0.02]— only 0.9 is above threshold, so XTC should mask nothing.Batched together, row A's 0.25 becomes the global threshold and row B's top token (p=0.9) is masked to
-inf, forcing row B to sample from its near-zero tail. Row B processed alone is unaffected — so batching silently changes (and corrupts) sampling results.Fix
Reduce along the last axis with
keepdimsso the threshold is computed per distribution:This matches the axis convention used by the other filters and is a no-op for the single-row case.
Testing
tests/test_sample_utils.pyasserting that each row of a batchedapply_xtccall equals the same row processed alone (fails onmain, passes with this change).python -m unittest tests.test_sample_utils -v— 7 tests, all pass.black --checkclean.Note: PR #1540 excludes XTC from its vectorized batching because the single scalar
mx.random.uniform(0, 1)gate is shared batch-wide; that gate is orthogonal to this fix and left untouched here, but the threshold bug above affects the existingbatch_generatefallback path today regardless.