Repository navigation
fix(mlx): penalties count the latest token, and only the tokens they're defined over - #2527
Open
AlexCheema wants to merge 1 commit into
Open
AlexCheema wants to merge 1 commit into
AlexCheema wants to merge 1 commit into
Conversation
…re defined over The batch engine's step handed logits processors the tokens already in the KV cache, which gain the token being fed only at the end of the step: repetition, presence and frequency penalties never saw the token just generated, so "the the" went unpenalised. It now gives them mlx-lm's per-sequence token context, which includes the fed token (as mlx-lm's own step and generate_step do), and costs one append per step instead of converting the whole history. What the penalties count also differed between the engines and from the usual definitions: the batch engine's history started with one stray prompt token, the sequential engine's with the last prompt token (or two, depending on mlx-lm's version), and neither had the rest of the prompt. exo now builds each request's processors to count: - presence and frequency penalties: the generated tokens, as OpenAI defines them (counts of tokens sampled so far); - the repetition penalty: the prompt and the generated tokens, as Hugging Face transformers applies it. Each processor learns, on its first call (the first generated token, when the history holds only prompt tokens), how many prompt tokens its engine's history starts with, so both engines and both mlx-lm histories give the same result. Co-Authored-By: Claude Opus 5.5 (1M context) <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.
This PR fixes which tokens the repetition, presence and frequency penalties count. Logits are unchanged when no penalty is set, and so is greedy decoding without penalties.
The bug
The batch engine never penalised the token just generated.
opt_batch_gen._patched_step) handed logits processorsself.tokens, the tokens already in the KV cache.generate_stepboth include the fed token.What the penalties counted also depended on the engine.
Recorded on Qwen3.5-2B (greedy, repetition penalty 1.1, presence penalty 1.5), for the first generated token, the processors saw:
At every later step, the batch engine's history also lacked the token being fed.
What each penalty should count
How common implementations define them:
repetition_penaltyis applied overinput_ids, which for decoder-only models includes the prompt (an option excludes it). It has no presence or frequency penalty.This PR follows the OpenAI definition for presence and frequency, which is what exo's OpenAI-compatible API exposes and what card values such as Qwen's
presence_penalty = 1.5are tuned for. It follows the transformers definition for the repetition penalty:The windows are unchanged: mlx-lm's last 20 tokens for presence and frequency, and
repetition_context_size(default 20) for repetition.Counting the prompt in the presence penalty would penalise every number and word of the question at 1.5, which matters for tasks that copy from the prompt. The quality check below includes that variant.
The change
GenerationBatch._token_context): the tokens in the cache plus the one being fed, as mlx-lm's own step does. It is evaluated with the step, and it costs one append per step instead of converting the whole history to an array every step.penalty_processors(...)ingenerator/generate.py: used by both engines. It builds mlx-lm's processors with the same values, the same skipping of unset or zero penalties and the same order, each wrapped in aPenaltyProcessorthat hands its penalty the tokens it counts.Evidence
Unit tests (
src/exo/worker/engines/mlx/tests/test_penalty_history.py):make_logits_processors.Both engines, recorded, Qwen3.5-2B (greedy, repetition 1.1, presence 1.5, 12 tokens):
Quality: GSM8K at temperature 1. Qwen3.5-2B (8-bit) with its card's sampling defaults: temperature 1, top-p 0.95, top-k 20, presence penalty 1.5, repetition penalty 1.0. 150 problems × 3 seeds, 8 requests decoding at once through the batch engine, thinking off (to keep answers short), at most 2048 tokens. Variants:
Paired by problem and seed, with an exact two-sided sign test on the answers only one variant got right:
So counting the latest token (and dropping the stray prompt token) is a significant improvement here, with fewer answers running into the token limit. Whether presence also counts the prompt made no measurable difference on this test, so the choice between the two rests on the definitions above.
Interplay with #2524
Both PRs touch the batch step's processor loop. Once this lands, #2524's windowed history is redundant (the token context here is already one append per step), and its repetition-penalty-1.0 skip moves into
penalty_processors. Whichever lands second gets rebased.Checks
uv run basedpyright: 0 errors.uv run ruff check: clean.nix fmt: no changes.uv run pytest: 721 passed, 3 skipped. The image-generation tests need torch and weren't run.🤖 Generated with Claude Code