Fix reversed FCFS priority in PagedAttentionScheduler preemption - #2250
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes FCFS priority inversion in PagedAttentionScheduler completion (decode) scheduling by removing an incorrect reverse() after sorting running sequences by their creation timestamp. This ensures the oldest sequences are scheduled first (pop_front) and, under KV-cache pressure, the newest sequences are preempted first (pop_back), aligning behavior with FCFS and the DefaultScheduler approach.
Changes:
- Remove the post-sort
reverse()insort_running_by_priority_fcfs, keepingrunningordered oldest-to-newest by timestamp. - Correct decode-loop scheduling/preemption behavior so FCFS matches the function name and intended semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Metrics Report━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Language Files Lines Code Comments Blanks ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ C Header 23 4454 3116 790 548 CSS 3 282 252 6 24 CUDA 119 23620 19165 1704 2751 Dockerfile 1 35 19 9 7 HTML 2 27 27 0 0 JavaScript 3 577 562 12 3 Jinja2 7 694 656 5 33 JSON 27 15864 15861 0 3 Makefile 1 16 14 0 2 MDX 36 5865 0 4303 1562 Metal Shading Lan| 37 14287 11284 1136 1867 PowerShell 1 484 385 40 59 Python 143 11695 9677 473 1545 Shell 2 865 668 111 86 Plain Text 3 3723 0 2413 1310 TOML 27 1331 1150 43 138 TypeScript 11 1641 1404 66 171 YAML 3 25 23 2 0 ───────────────────────────────────────────────────────────────────────────────── Jupyter Notebooks 3 122 83 23 16 |- Markdown 1 60 30 22 8 |- Python 1 122 113 1 8 (Total) 304 226 46 32 ───────────────────────────────────────────────────────────────────────────────── Markdown 266 11215 0 8338 2877 |- BASH 22 282 208 43 31 |- Dockerfile 2 5 5 0 0 |- JSON 6 289 289 0 0 |- PowerShell 1 1 1 0 0 |- Python 129 7052 5852 310 890 |- Rust 62 3817 2838 385 594 |- TOML 7 77 65 0 12 (Total) 22738 9258 9076 4404 ───────────────────────────────────────────────────────────────────────────────── Rust 647 286197 254469 5897 25831 |- Markdown 410 9768 452 8136 1180 (Total) 295965 254921 14033 27011 ───────────────────────────────────────────────────────────────────────────────── Svelte 19 1969 1826 51 92 |- CSS 1 4 4 0 0 |- JavaScript 19 921 767 25 129 (Total) 2894 2597 76 221 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Total 1384 407386 331265 34344 41777 ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ |
|
@EricLBuehler the failing Check jobs arent from this PR, its just a crates.io flake fetching tendril (html2text dep) durin cargo check, the actual Test Suites all pass green so the fix is fine, just needs a re-run whenver you get a sec |
767c126 to
72ab6ec
Compare
EricLBuehler
left a comment
There was a problem hiding this comment.
Hey @pjdurden - thanks for the PR. Can you please remove Claude from your commit authorship?
sort_running_by_priority_fcfs sorted self.running ascending by timestamp (oldest first) and then reversed it to newest first. The decode loop then schedules with pop_front and preempts under KV-cache pressure with pop_back, so the reverse made the oldest (highest priority) sequences get preempted first and the newest scheduled first, the opposite of FCFS and of the function's own name. Drop the reverse so running stays oldest-first: pop_front schedules the oldest sequence first and pop_back preempts the newest (lowest priority) first. This matches DefaultScheduler, which sorts ascending without reversing.
72ab6ec to
b8c9f40
Compare
Done, removed Claude from the commit authorship. Thanks for the review! |
Fixes #2033.
sort_running_by_priority_fcfs sorts self.running ascending by timestamp (oldest
first) and then reverses it to newest first. The decode loop schedules with
pop_front and, under KV-cache pressure, preempts with pop_back. With the reverse
in place the oldest (highest priority) sequences get preempted first and the
newest get scheduled first, the opposite of FCFS and of the function's own name.
Dropping the reverse keeps running oldest-first: pop_front schedules the oldest
sequence first and pop_back preempts the newest (lowest priority) first. This
matches DefaultScheduler, which sorts ascending without reversing.