Skip to content

Adaptive P sampler: update review logic, delete old code comments, put prep stage after logit bias#1386

Merged
ikawrakow merged 22 commits intoikawrakow:mainfrom
dungquixote42:adaptive_p-0
Mar 14, 2026
Merged

Adaptive P sampler: update review logic, delete old code comments, put prep stage after logit bias#1386
ikawrakow merged 22 commits intoikawrakow:mainfrom
dungquixote42:adaptive_p-0

Conversation

@dungquixote42
Copy link
Contributor

This PR deletes old code comments left in #1359 .

The review logic now tracks slot.token_buffer.size(), instead of n_rewind. Looking at the end result of bans/rewinds should be more reliable than recreating the math. As a side effect, this corerctly handles a case where send_token_results(2) does not actually send all tokens.

This removes all llama_review_adaptive_p_impl() related variables from buffer_and_check_string_ban(). Hopefully, this helps with the supposed bug regarding #1243 .

@dungquixote42
Copy link
Contributor Author

Converting to draft until #1243 is merged.

@dungquixote42 dungquixote42 marked this pull request as draft March 11, 2026 04:34
@dungquixote42 dungquixote42 marked this pull request as ready for review March 11, 2026 23:38
@dungquixote42
Copy link
Contributor Author

dungquixote42 commented Mar 12, 2026

Tokens belonging to banned strings often have high logits, and llama_prep_adaptive_p() drops tokens with logit<max-30 while collecting the probabilities. It looks like collecting before logit bias leaves too few tokens as a result. Restoring the original llama_prep_adaptive_p() with dd3bfac.

@dungquixote42 dungquixote42 changed the title Adaptive P sampler: update review logic, delete old code comments Adaptive P sampler: update review logic, delete old code comments, put prep stage after logit bias Mar 12, 2026
@ikawrakow
Copy link
Owner

@dungquixote42

I see you keep changing things around. When you get to a version that does not need multiple follow up PRs, let me know.

@dungquixote42
Copy link
Contributor Author

@dungquixote42

I see you keep changing things around. When you get to a version that does not need multiple follow up PRs, let me know.

Yeah. I just keep finding these cases I did not account for. It is over now, at least from my end.

@dungquixote42
Copy link
Contributor Author

@ikawrakow This PR is ready for review.

@ikawrakow ikawrakow merged commit be2940f into ikawrakow:main Mar 14, 2026
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