Skip to content

fix adaptive p sampler rewinding too far back#1359

Merged
ikawrakow merged 7 commits intoikawrakow:mainfrom
dungquixote42:fix-adaptive_p-string_ban-2
Mar 4, 2026
Merged

fix adaptive p sampler rewinding too far back#1359
ikawrakow merged 7 commits intoikawrakow:mainfrom
dungquixote42:fix-adaptive_p-string_ban-2

Conversation

@dungquixote42
Copy link
Contributor

@dungquixote42 dungquixote42 commented Mar 4, 2026

#1287 does not account for partial rewinds. Fixing it with this PR.


ctx->record_samplers = false;
ctx->rewind_samplers = false;
// add stateful samplers here
Copy link
Owner

Choose a reason for hiding this comment

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

Why did you remove the check for ctx->adapt_p_ctx being null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now in llama_review_adaptive_p_impl(). I think of common_sampler_review() as a place to list stateful samplers that needs reviewing, so the check felt a bit too preemptive. I can put the check back in here.

auto & weighted_sum = adapt_p_ctx->weighted_sum;
auto & total_weight = adapt_p_ctx->total_weight;
if (n_rewind < 0) {
// clear history except most recent
Copy link
Owner

Choose a reason for hiding this comment

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

What if weighted_sum or total_weight is empty (i.e., size() returns 0)?

My concept is that erase is kind of slow. I would write something like this (assuming weighted_sum and total_weight have the same size):

if (weighted_sum.size() > 1) {
    weighted_sum.front() = weighted_sum.back();
    total_weight.front() = total_weight.beck();
    weighted_sum.resize(1);
    total_weight.resize(1);
}

Copy link
Contributor Author

@dungquixote42 dungquixote42 Mar 4, 2026

Choose a reason for hiding this comment

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

What if weighted_sum or total_weight is empty (i.e., size() returns 0)?

That really should not happen, but I added a check.

My concept is that erase is kind of slow. I would write something like this (assuming weighted_sum and total_weight have the same size):

if (weighted_sum.size() > 1) {
    weighted_sum.front() = weighted_sum.back();
    total_weight.front() = total_weight.beck();
    weighted_sum.resize(1);
    total_weight.resize(1);
}

Neat. Fixed.

@dungquixote42
Copy link
Contributor Author

dungquixote42 commented Mar 4, 2026

weighted_sum and total_weight is combined into a vector of pairs in the most recent commit. I need to test it a bit more, but I think it is an improvement overall. If it is acceptable, I will remove the old code that has been commented out.

@ikawrakow ikawrakow merged commit a903409 into ikawrakow:main Mar 4, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 5, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 7, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 9, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 10, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 10, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 10, 2026
Nexesenex added a commit to Nexesenex/ik_llama.cpp.nxs that referenced this pull request Mar 10, 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