Skip to content

fix(batching): guarantee progress for bypassed requests - #798

Draft
svonava wants to merge 1 commit into
mainfrom
fix/issue-390-batch-progress
Draft

svonava wants to merge 1 commit into
mainfrom
fix/issue-390-batch-progress

Conversation

@svonava

@svonava svonava commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

An admitted expensive item can wait indefinitely when cheaper items keep arriving: each batch re-sorts the backlog and takes its cheapest prefix. The pinned GLiClass L4 comparison reproduced 300-second timeouts with both the current profile and PR656’s adaptive-batching workaround, while cheap requests continued completing. Full negative comparison.

After an ordinary batch bypasses the oldest batchable item, reserve that exact entry for the next batchable turn. The reserved turn takes its cost-sorted suffix without wrapping into cheaper arrivals; ordinary packing then resumes. An entry with k older batchable entries gets a dispatch opportunity within 2(k + 1) nonempty batchable turns. Existing isolated-item turns can interleave, and this is a dispatch-count bound rather than a wall-clock latency guarantee.

The change preserves request/cost caps, the oversized-singleton escape, isolated-item ordering, and ordinary homogeneous/fitting batch membership. Arrival identity is internal. The worker change only corrects documentation about draining a snapshot count; its executable AST is unchanged. No model profile, adaptive controller, or idle-worker scheduling change is included.

Validation completed:

  • 288 focused and related tests passed with no skips; Ruff and type checks passed.
  • Deterministic regressions cover cheap replenishment, within-budget and oversized work, equal timestamps, dynamic caps, isolated turns, zero-item drains, shutdown, and item conservation.
  • Independent review checked 32 mixed schedules / 9,500 items and the dispatch bound.
  • The original implementation fails all three retained starvation controls; the candidate dispatches the reserved item on turn two. A six-pair local batch-formation microbenchmark measured approximately 0.10 µs additional median cost at C1 and 0.82 µs at C32; this is not end-to-end performance evidence.

The planned SIE server + SIEClient L4 comparison of identical-profile baseline/candidate sources is complete and failed the original numerical criterion. This PR remains draft and unmerged, with no qualified default-behavior change or GPU performance claim; investigation continues. All original criteria and failed evidence remain unchanged.

Refs #390.

Summary by CodeRabbit

  • Bug Fixes
    • Improved request scheduling so older requests are less likely to be delayed when newer, lower-cost requests keep arriving.
    • Requests that exceed the current batch-cost limit still receive a turn, while batches continue to respect configured limits.
    • Improved consistency in how pending requests are handled during batch processing and shutdown.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Team
  • Run ID: 6c6b29c9-142a-42fc-b7d5-3e94c96aab13
📥 Commits

Reviewing files that changed from the base of the PR and between b68fe36 and 45794f6.

📒 Files selected for processing: 3
  • packages/sie_server/src/sie_server/core/batcher.py
  • packages/sie_server/src/sie_server/core/worker/model_worker.py
  • packages/sie_server/tests/core/test_batcher.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

📝 Walkthrough

BatchFormer now tracks request arrival order and reserves the oldest batchable request skipped by ordinary packing. A later batchable turn starts at that request. Drain behavior documentation and tests cover item limits, reservation cleanup, and batch selection.

Changes: Batchable request progress

Layer / File(s) Summary
Arrival ordering and reserved suffix packing
packages/sie_server/src/sie_server/core/batcher.py, packages/sie_server/tests/core/test_batcher.py
BatchFormer assigns arrival order to requests and reserves the oldest batchable request omitted by ordinary packing. A reserved turn packs from that request’s cost-sorted suffix. Tests cover progress, packing, changing budgets, and greedy-selection regressions.
Drain limits and reservation cleanup
packages/sie_server/src/sie_server/core/batcher.py, packages/sie_server/src/sie_server/core/worker/model_worker.py, packages/sie_server/tests/core/test_batcher.py
Pending-queue cleanup clears the reservation. Documentation clarifies that drain limits cap the number of extracted items but do not freeze which arrivals are eligible. Tests cover empty, capped, and shutdown-related drains.

Suggested reviewers: shaishmuel

Priority: ⬆️ High

Merge Risk: ⚪ Minimal · up to 45794

No actionable merge-blocking risk is established. Complete the planned real-server comparison before making throughput or latency claims.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 47.62% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: ensuring that bypassed batching requests eventually receive a dispatch opportunity.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@svonava

svonava commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

Please review exact head 45794f6. This draft has completed focused CPU validation and independent progress-bound review; real-server GPU evidence is still pending.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

This branch has not been deployed

No deployments
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.

1 participant