Repository navigation
server : fix swa checkpoint invalidation logic - #30666
Conversation
Assisted-by: Claude
There was a problem hiding this comment.
This fix seems to be correct, although I am wondering if it would be cleaner to just move the checkpoint erasure logic before the checkpoint search block (i.e. before line 3794). This way we don't need the temporary n_prefix local and it's simpler to reason.
Edit: nvm, it has to be after the search block.
| for (auto it = slot.prompt.checkpoints.begin(); it != slot.prompt.checkpoints.end();) { | ||
| const auto & cur = *it; | ||
| if (cur.pos_max > pos_next) { | ||
| if (cur.pos_max > pos_next || cur.n_tokens > n_prefix) { |
There was a problem hiding this comment.
Do you see any problem with simply comparing to n_past instead of introducing the n_prefix?
| if (cur.pos_max > pos_next || cur.n_tokens > n_prefix) { | |
| if (cur.pos_max > pos_next || cur.n_tokens > n_past) { |
I think it would be correct.
There was a problem hiding this comment.
Comparing to n_past is stricter and will drop the checkpoint that was just restored, because n_past is rewound to it->n_tokens - 1. Let me add some test cases that catch it.
There was a problem hiding this comment.
pushed the test case that fails with comparison to n_past, not sure if it should go into upstream.
ggerganov
added
the
merge ready
…gml-org#25592 merged with master ggml-org#30666
Overview
Fix
llama-serverrestoring SWA checkpoints saved from a previous unrelated prompt.Additional information
Add an A/B test case that fails before the fix and works after.
Requirements