Skip to content

Fix TypeError in apply_min_p when min_tokens_to_keep > 1 - #1600

Merged
zcbenz merged 1 commit into
ml-explore:mainfrom
mloiterman:fix/apply-min-p-scalar-values
Aug 27, 2026
Merged

Fix TypeError in apply_min_p when min_tokens_to_keep > 1#1600
zcbenz merged 1 commit into
ml-explore:mainfrom
mloiterman:fix/apply-min-p-scalar-values

Conversation

@mloiterman

Copy link
Copy Markdown
Contributor

apply_min_p's min_tokens_to_keep > 1 branch passes bare Python False as the values argument of mx.put_along_axis, whose binding requires an mx.array. The branch raises TypeError on every invocation:

>>> from mlx_lm.sample_utils import apply_min_p
>>> import mlx.core as mx
>>> logprobs = mx.log(mx.softmax(mx.random.normal((1, 100)), axis=-1))
>>> apply_min_p(logprobs, 0.05, 3)
TypeError: put_along_axis(): incompatible function arguments. ...
Invoked with types: mlx.core.array, mlx.core.array, bool, kwargs = { axis: int }

Verified the rejection is not mlx-version-dependent (probed mlx 0.31.2 and 0.32.0 — bare bool rejected on both). The scalar was introduced in #1083's refactor (the pre-refactor code passed proper arrays); it has stayed latent because tests/test_sample_utils.py has no min_tokens_to_keep coverage and mlx_lm.server doesn't expose the parameter — only direct library consumers of make_sampler(min_tokens_to_keep=...) can reach the branch. We hit it in a production server built on BatchGenerator the first time a client set the knob.

One-line fix: Falsemx.array(False). Adds a regression test that fails with the TypeError pre-fix and pins the keep-floor semantics (exactly min_tokens_to_keep survivors under an aggressive min_p), single-row and batched.

@zcbenz

zcbenz commented Aug 21, 2026

Copy link
Copy Markdown
Member

Thanks for the PR but at the moment the number of PRs is way beyond our capacity to review so I'm closing the non-essential ones so we can actually work on this repo.

@zcbenz zcbenz closed this Aug 21, 2026
@mloiterman

Copy link
Copy Markdown
Contributor Author

Could this one be reopened? It was closed in the capacity sweep rather than on the merits, and it is a hard crash, not a cleanup: any call of apply_min_p(logprobs, min_p, min_tokens_to_keep) with min_tokens_to_keep > 1 raises TypeError from mx.put_along_axis (the values argument is a bare Python False; the binding requires an mx.array). Still reproducible on current main, on mlx 0.32.0 as on 0.31.x — the branch cannot execute at all, so nobody using the keep-floor gets a result.

The fix is one token (Falsemx.array(False)) plus the regression test already in the PR; the test fails on main and passes with the change. We have been carrying the same one-line change locally since July with no issues. Happy to rebase if that helps.

@zcbenz

zcbenz commented Aug 25, 2026

Copy link
Copy Markdown
Member

Sure thanks for looking into this, please excuse us there were just too many pull requests and it was easy to mistakenly close valid ones.

@zcbenz zcbenz reopened this Aug 25, 2026

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

The min_tokens_to_keep branch passes bare Python False as the values
argument of mx.put_along_axis, whose binding requires an mx.array —
the branch raises TypeError on every invocation, on every mlx version.

Introduced by the ml-explore#1083 refactor (the pre-refactor code passed proper
arrays); latent since because the parameter has no test coverage and
mlx_lm.server does not expose it, so only direct library consumers of
make_sampler(min_tokens_to_keep=...) can reach it.

Fix: wrap the scalar (mx.array(False)). Adds a regression test that
fails with the TypeError pre-fix and verifies keep-floor semantics
(exactly min_tokens_to_keep survivors under an aggressive min_p),
single-row and batched.
@zcbenz
zcbenz force-pushed the fix/apply-min-p-scalar-values branch from 8ddf863 to c2c0647 Compare August 27, 2026 11:05
@zcbenz
zcbenz merged commit 94ecd0a into ml-explore:main Aug 27, 2026
2 checks passed
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.

2 participants