Skip to content

fix(server): evict the least recently used group when two blocks cost the same - #294

Merged
krisztian-gajdar merged 1 commit into
mainfrom
fix/group-eviction-recency
Sep 17, 2026
Merged

krisztian-gajdar merged 1 commit into
mainfrom
fix/group-eviction-recency

Conversation

@krisztian-gajdar

@krisztian-gajdar krisztian-gajdar commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

What this changes

When a model with tensor_parallel_size above one has to evict residents to get a block of devices, the registry frees the block that needs the fewest evictions. Until now, ties between blocks went by device order.

With two groups resident on a four-device worker, loading a third group therefore always evicted whichever group held cuda:0, even if it was the busy one. The idle group kept its devices. Requests alternating between the busy model and the newcomer then evicted each other in turn.

Ties now go to the block whose most recently used resident was used longest ago. Single-device loads that displace a group already follow this rule (_group_holder_to_evict), and both paths now share one helper for a model's last use. The fewest-evictions rule, pinned-model protection and the preference for a free block are unchanged.

Tests

test_between_equal_blocks_the_least_recently_used_group_is_evicted runs in both directions (the first group idle, then the second), so device order cannot pass it.

  • On the previous registry the second case fails, because it evicts the busy group on cuda:0/cuda:1.
  • With this change both cases pass.
  • packages/sie_server/tests/core/test_registry_device_groups.py: 41 passed.
  • packages/sie_server/tests/core: 1087 passed, 1 skipped, 1 failed. The failure is test_offline_cache_only_derived_snapshot_materializes_without_hub_fallback, a macOS PermissionError that fails the same way on main without this change.
  • ruff format --check, ruff check and ty check pass on the changed files.

Summary by CodeRabbit

  • Bug Fixes

    • Improved multi-device placement eviction so eligible blocks are selected based on blocker count and recency rather than device order.
    • Ensured the least-recently-used resident group is evicted when loading a new group with equal placement requirements.
  • Tests

    • Added coverage verifying recency-based eviction and device reassignment behavior.

… the same

When a tensor-parallel load had to evict, it freed the block needing the fewest evictions and broke ties by device order. With two resident groups, a load could evict the busy group while an idle one kept its cards, and the busy model and the newcomer would then evict each other in turn. Ties now go to the block whose most recently used resident was used longest ago, the rule single-device displacement of a group already follows.
@krisztian-gajdar
krisztian-gajdar requested a review from a team as a code owner September 17, 2026 12:18
@coderabbitai

coderabbitai Bot commented Sep 17, 2026 •

Copy link
Copy Markdown

Review Change StackReview 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: 5dfb154a-aae0-453e-be7b-ab3cb059a7e9

📥 Commits

Reviewing files that changed from the base of the PR and between a03f5db and 2b64774.

📒 Files selected for processing (2)
  • packages/sie_server/src/sie_server/core/registry.py
  • packages/sie_server/tests/core/test_registry_device_groups.py

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Multi-device placement now prioritizes eviction candidates by blocker count and blocker recency. Registry tests verify that the older equal-sized resident group is evicted regardless of device ordering.

Changes

Placement eviction ordering

Layer / File(s) Summary
Eviction candidate selection
packages/sie_server/src/sie_server/core/registry.py
Multi-device placement ranks candidates by fewest blockers, then by the oldest blocker timestamp. Recency lookup uses _last_used_at with an untracked-model default of 0.0.
Recency-based placement tests
packages/sie_server/tests/core/test_registry_device_groups.py
Parameterized tests set resident-group timestamps and verify that the older group is evicted while the newer group remains loaded.

Suggested reviewers: dragosboca

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 2b647

The placement change retains blocker-count priority and applies the intended recency tie-breaker, with coverage for either device ordering. No merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: tie-breaking equal-cost multi-device blocks by evicting the least recently used group.
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/group-eviction-recency

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

@krisztian-gajdar

Copy link
Copy Markdown
Contributor Author

Checked on real engines at this head (2b647744), on a machine with four NVIDIA L4 GPUs. The run used a scratch catalog where three shipped SGLang generation models declare tensor_parallel_size: 2, and drove the registry through load_async.

Step Result
Load Qwen/Qwen3-0.6B 152 s, claimed cuda:0, cuda:1
Load ibm-granite/granite-guardian-3.0-2b 172 s, claimed cuda:2, cuda:3
Generate with Qwen3-0.6B again, so the granite group is least recently used " Paris. The capital of France is also"
Load Qwen/Qwen3-4B-Instruct-2507 154 s. Evicted granite-guardian from cuda:2, cuda:3, and claimed those two cards. Qwen3-0.6B stayed on cuda:0, cuda:1, which is the group device order alone would have evicted
Generate after the eviction Qwen3-0.6B: " Paris. The capital of France is also". Qwen3-4B: " Paris. The capital of Germany is Berlin"
unload_all_async No claims left. Every card back to 196 MiB or less

Memory per card:

  • With two groups resident: 18557 and 18366 MiB for Qwen3-0.6B, 20061 and 19870 MiB for granite-guardian.
  • After Qwen3-4B replaced granite-guardian: 20587 and 20396 MiB on cuda:2 and cuda:3.

granite-guardian's own completion of a raw prompt returned invalid_guard_verdict. That is the guard model's verdict contract for a prompt sent without its guardian chat template. The engine served the request at width two.

@krisztian-gajdar
krisztian-gajdar merged commit 120059f into main Sep 17, 2026
21 checks passed
@krisztian-gajdar
krisztian-gajdar deleted the fix/group-eviction-recency branch September 17, 2026 12:43
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