fix(inference): handle None top_k/top_p defaults in sample_from_logits#351
Open
dvirarad wants to merge 1 commit into
Open
fix(inference): handle None top_k/top_p defaults in sample_from_logits#351dvirarad wants to merge 1 commit into
dvirarad wants to merge 1 commit into
Conversation
`sample_from_logits` declares `top_k=None` and `top_p=None` defaults, but its guard evaluated `top_k > 0 or top_p < 1.0` whenever either value was not None. Passing only one filter (e.g. `sample_from_logits(logits, top_p=0.9)`) therefore raised `TypeError: '>' not supported between instances of 'NoneType' and 'int'`, and a None value could also be passed down to `top_k_top_p_filtering`, which performs the same comparison. Normalize the unset parameter to its neutral default (top_k=0, top_p=1.0) before the comparison. Behavior is unchanged for all existing call sites, which always pass explicit numeric values.
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
1 participant
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.
What
sample_from_logitsdeclarestop_k=None/top_p=Nonedefaults, but its guard evaluatestop_k > 0 or top_p < 1.0whenever either value is non-None. Supplying only one filter — e.g.sample_from_logits(logits, top_p=0.9)— hitsNone > 0and raisesTypeError: '>' not supported between instances of 'NoneType' and 'int'. ANonecould likewise be forwarded intotop_k_top_p_filtering, which does the same comparison.Fix
Normalize an omitted parameter to its neutral value (
top_k=0,top_p=1.0) before comparing. No behavior change for existing callers (auto_regressive_inferenceandKronosPredictor.predictalways pass explicit numbers); this only fixes the crash when one filter is left at its default.Testing
Verified the guard against
{},top_p=0.9,top_k=5,top_k=0,top_p=0.99,top_k=0,top_p=0.9: old code crashed only on thetop_p-only case; new code matches old behavior everywhere else.python -m py_compile model/kronos.pypasses.