fix(generate): guard per-sequence logits_processors against None in batched generation - #1657
Open
chronoshift wants to merge 1 commit into
Open
Conversation
In a mixed batch where some sequences carry logits_processors and others do not, the absent entries are None, so the per-sequence step loop raises `TypeError: 'NoneType' object is not iterable` and kills the generation thread. The HTTP thread keeps answering /health with 200 while every completion hangs forever. Mirrors the guard the sibling samplers path already uses twelve lines below (`self.samplers[e] or self.fallback_sampler`). Refs ml-explore#1472, ml-explore#1505. Co-Authored-By: Claude Opus 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
In a mixed batch — some sequences carrying
logits_processors(e.g. repetition or frequency penalties), others not — the absent entries areNone. The per-sequence step loop inGenerationBatchthen does:which raises
TypeError: 'NoneType' object is not iterableand kills the generation thread. The HTTP thread keeps answeringGET /healthwith 200 while every completion hangs indefinitely, so the failure is invisible to a health check and the server has to be restarted.The practical effect is that
--decode-concurrencyabove 1 is unsafe whenever concurrent requests use different sampling settings, which is the normal case for a shared server.Reproduction
Start
mlx_lm.server --decode-concurrency 2and send two concurrent chat completions where one setsrepetition_penalty(orfrequency_penalty) and the other does not, so they land in the same batch.Fix
One line, mirroring the guard the sibling
samplerspath already uses twelve lines below in the same method:Relationship to existing issues and PRs
_generateleaves the server answering 200s while generation is dead.[None]→[[]]) and was closed unmerged.samplers/logits_processorsbookkeeping infilter/extendand is still open.This patch is deliberately the reader-side guard instead. It is one line, it cannot regress a caller that legitimately passes
None, it matches the established pattern immediately below it, and it is orthogonal to #1225 — if that lands, this becomes redundant rather than conflicting. It is offered as a minimal stop-gap for a crash that has been reported repeatedly and is still reproducible onmain.