Repository navigation
perf: reduce replay request bookkeeping - #386
Conversation
Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (23)
🧰 Additional context used📓 Path-based instructions (6)Review scheduler state transitions and event ordering, including cancel, preempt, drain, retry, duplicate, empty, and terminal paths.⚙️ CodeRabbit configuration file Files:
Preserve deterministic Replay semantics and explicit validation.⚙️ CodeRabbit configuration file Files:
Read REVIEW.md before commenting.⚙️ CodeRabbit configuration file Files:
Source excerpt: `core.rs` owns common queue mechanics, KV allocation, state transitions, completion, and event emission.📄 CodeRabbit inference engine (crates/core/src/engine/scheduler/vllm/CLAUDE.md) Files:
Before making any change under: `python/aisimulate/src/aisimulate/generator/**` MUST read: `python/aisimulate/.claude/rules/generator-development.md` Before making any change under `python/aisimulate/collector/**` MUST read: `python/aisimul...📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Only workflows under the repository-root `.github/workflows/` run for this repository.📄 CodeRabbit inference engine (REVIEW.md) Files:
📝 SummaryRisk: MediumThree areas for human attention
Changes and contracts
vLLM now reserves capacity for committed IDs and filters output signals to scheduled requests with zero prompt tokens. It skips a signal when the latest committed ID matches. Final sorting and deduplication remain in place. Report collection now tracks successful-request time bounds for ordinary replay as well as profile runs. The change does not declare public API changes. The stated intent is to preserve API and cancellation behavior. Evidence and readinessThe supplied test report says 111 focused Rust tests passed, including cancellation across vLLM and SGLang, multiple attention-DP ranks, speculative retraction, and report accounting. Formatting and diff checks are also reported as passing. These results were not independently run here. The reported benchmark for Tyche with dense-vLLM shows 19.654 s versus 22.956 s for the unchanged Technical quality evidence includes the expanded DP-rank and cancellation tests. Merge readiness is not established by these results alone. No current review findings were supplied, so review severity counts are unavailable. WalkthroughThe scheduler now applies additional conditions when adding requests from output signals to the committed set. Replay passes store committed request IDs in a vector and initialize a membership index on first query. Replay tests cover multiple data-parallel sizes, cancellation, and pass completion. ChangesScheduler output-signal filtering
Replay committed-request tracking
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to No confirmed issue prevents merging after normal checks. 🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
Full details: Modeling And Data EvidenceExplanation The PR changes vLLM committed-request selection in Resolution Add reproducible before/after evidence for the changed selection logic. For example, add an independent-oracle or golden test that asserts the committed request IDs for prefill, token-bearing decode, and repeated speculative signals. For the performance claim, provide the benchmark configuration and invocation plus machine-readable baseline and optimized results with anomaly checks, or provide another accepted reproducible comparison such as an explained golden diff or held-out comparison. Full details: Description checkExplanation The description explains the change and reports test and benchmark results, but it omits the required Review map and Tracking sections. The Evidence section also lacks exact test commands and results, Fast CI and Full CI status, review commit details, and several requested boundary-case and expected-value details. Resolution Add the Review map with risk level, reviewer starting points, contract impact, and compatibility concerns. Complete the Evidence fields, including exact commands and results, CI status, reviewed commits, negative or boundary cases, and expected-value derivation; mark unavailable evidence clearly. Add Modeling or data provenance and Tracking sections, using N/A where applicable.
Comment |
Restore successful-request report time bounds and cover an idle first attention-DP rank for both replay backends. Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Harrilee
left a comment
Harrilee
left a comment
There was a problem hiding this comment.
LGTM.
- vLLM: the new collection is equivalent to the old one after
sort_unstable+dedup— prefill IDs come fromscheduled, and output-signal IDs are now restricted toprompt_tokens == 0work (prefill ones were already included), with thelast()check only skipping consecutive speculative-burst duplicates. - Replay:
PendingPass.committed_requestsis only written during construction indrive_readyand only read byrequest_has_committed_pass, so the lazily builtOnceLockindex can't go stale. Moving the first rank's Vec instead of extending is safe even when it's empty. - The extended test (dp_size 1/2/3 with an idle rank 0, querying before/after cancel) covers the lazy-init-after-cancel path well.
Ran the replay engine / vLLM scheduler / committed_request tests locally at the PR head: 194 passed.
* perf: reduce replay request bookkeeping Signed-off-by: jthomson04 <jwillthomson19@gmail.com> * perf(replay): skip repeated speculative request IDs Restore successful-request report time bounds and cover an idle first attention-DP rank for both replay backends. Signed-off-by: jthomson04 <jwillthomson19@gmail.com> --------- Signed-off-by: jthomson04 <jwillthomson19@gmail.com>
Summary
Reduce the replay bookkeeping added in #307. Reuse committed request-ID vectors and build the membership index only when cancellation needs it. Reserve vLLM vector capacity and skip duplicate prefill and consecutive speculative-burst IDs. Public APIs and worker-wide membership are unchanged.
Validation
0a9ef773) was not benchmarked.Historical dense-vLLM result, 524,288 requests: the 14.4% native-time improvement was measured at
a1bf0351, against the unchanged #307 merge (a9c358c5).a1bf0351One run per build. Timing variation and SGLang performance remain unmeasured.