fix(scaling): honor live CSR allocation budget for rankings - #727
Merged
seonghobae merged 3 commits intoAug 11, 2026
Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR bounds LSR and I-LSR ranking CSR buffer growth by the live byte budget. It streams validated item indices into the flat ChangesLSR ranking CSR allocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…lization Cap geometric uint64 buffer growth by the declared MAX_RANKING_CSR_BYTES ceiling and stream validated ranking indices without a list-to-uint64 temporary beside the live CSR payload.
seonghobae
force-pushed
the
fix/scaling-ranking-csr-allocation-budget-green
branch
2 times, most recently
from
August 11, 2026 10:41
3e0d0d8 to
6e362c5
Compare
seonghobae
marked this pull request as draft
August 11, 2026 10:54
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/doctoring/lsr_ranking_csr_allocation_budget.md (1)
25-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the primary-source record.
The Maystre and Grossglauser reference has no primary-source link or paper-specific summary. Add a stable primary-source link or permitted PDF and summarize its relevance to the ranking-model context.
As per coding guidelines, “Substantive feature or process PRs should include relevant academic paper PDFs with full citations when redistribution is permissible; otherwise include a citation, link, and summary.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/doctoring/lsr_ranking_csr_allocation_budget.md` around lines 25 - 29, Update the Maystre and Grossglauser citation in the References section with a stable primary-source link or permitted paper PDF, and add a concise paper-specific summary explaining its relevance to the ranking-model context. Preserve the existing full citation and follow the redistribution guideline when selecting the source.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/fast_mlsirm/scaling.py`:
- Around line 508-538: Update the CSR growth logic around flat and starts so
replacement allocations include both live source-buffer bytes and
replacement-buffer bytes, rejecting before allocation or using another strategy
when the MAX_RANKING_CSR_BYTES ceiling cannot be preserved; update
python/fast_mlsirm/scaling.py lines 508-538 accordingly. In CHANGELOG.md line 8,
claim the live-budget guarantee only after this overlap accounting is enforced.
Correct the intermediate-allocation claim in
docs/changelog.d/719-scaling-ranking-csr-allocation-budget.md line 5, document
replacement-overlap accounting at
docs/doctoring/lsr_ranking_csr_allocation_budget.md lines 13-14, and require the
total concurrent allocation ceiling in its evidence contract at lines 21-22.
---
Nitpick comments:
In `@docs/doctoring/lsr_ranking_csr_allocation_budget.md`:
- Around line 25-29: Update the Maystre and Grossglauser citation in the
References section with a stable primary-source link or permitted paper PDF, and
add a concise paper-specific summary explaining its relevance to the
ranking-model context. Preserve the existing full citation and follow the
redistribution guideline when selecting the source.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c641d417-86ac-41d6-a66a-639de6275897
📒 Files selected for processing (5)
CHANGELOG.mddocs/changelog.d/719-scaling-ranking-csr-allocation-budget.mddocs/doctoring/lsr_ranking_csr_allocation_budget.mdpython/fast_mlsirm/scaling.pytests/test_scaling_ranking_live_allocation_budget.py
Comment on lines
+508
to
+538
| # Geometric growth is optional; never allocate more uint64 capacity than | ||
| # the declared CSR byte budget can hold when combined with the reserved | ||
| # start-offset slots for this ranking (including its closing offset). | ||
| geometric = max(need, max(8, flat.shape[0] * 2 or 8)) | ||
| max_flat_elems = (MAX_RANKING_CSR_BYTES // 8) - (start_count + 1) | ||
| capacity = min(geometric, max_flat_elems) | ||
| if capacity < need: | ||
| raise ValueError( | ||
| f"{name}: ranking CSR byte limit exceeded " | ||
| f"(MAX_RANKING_CSR_BYTES={MAX_RANKING_CSR_BYTES})" | ||
| ) | ||
| grown = np.empty(capacity, dtype=np.uint64) | ||
| if flat_count: | ||
| grown[:flat_count] = flat[:flat_count] | ||
| flat = grown | ||
| flat[flat_count : flat_count + len(ranking_items)] = np.asarray( | ||
| ranking_items, dtype=np.uint64 | ||
| ) | ||
| # Stream validated integers into the live buffer without a list→uint64 | ||
| # temporary that would double the working set outside the CSR budget. | ||
| for offset, xi in enumerate(ranking_items): | ||
| flat[flat_count + offset] = xi | ||
| flat_count += len(ranking_items) | ||
| if starts.shape[0] < start_count + 1: | ||
| grown_starts = np.empty( | ||
| max(start_count + 1, max(8, starts.shape[0] * 2)), dtype=np.uint64 | ||
| ) | ||
| need_starts = start_count + 1 | ||
| geometric_starts = max(need_starts, max(8, starts.shape[0] * 2)) | ||
| max_start_elems = (MAX_RANKING_CSR_BYTES // 8) - flat_count | ||
| start_capacity = min(geometric_starts, max_start_elems) | ||
| if start_capacity < need_starts: | ||
| raise ValueError( | ||
| f"{name}: ranking CSR byte limit exceeded " | ||
| f"(MAX_RANKING_CSR_BYTES={MAX_RANKING_CSR_BYTES})" | ||
| ) | ||
| grown_starts = np.empty(start_capacity, dtype=np.uint64) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Account for source and replacement buffers during CSR growth.
The replacement-capacity calculations only reserve the final CSR payload. They exclude source buffers that remain live during copying. With MAX_RANKING_CSR_BYTES=48, flat can hold four elements (32 bytes), the original starts holds one element (8 bytes), and grown_starts can hold four elements (32 bytes). Line 538 then reaches 72 live bytes before starts = grown_starts. This fails tests/test_scaling_ranking_live_allocation_budget.py lines 46-83.
python/fast_mlsirm/scaling.py#L508-L538: include current source-buffer bytes and replacement-buffer bytes in the live-budget calculation. If no safe replacement fits, reject before allocation or use an allocation strategy that preserves the ceiling.CHANGELOG.md#L8-L8: state the live-budget guarantee only after the replacement path enforces it.docs/changelog.d/719-scaling-ranking-csr-allocation-budget.md#L5-L5: correct the intermediate-allocation claim to match the enforced behavior.docs/doctoring/lsr_ranking_csr_allocation_budget.md#L13-L14: document the replacement-overlap accounting strategy.docs/doctoring/lsr_ranking_csr_allocation_budget.md#L21-L22: update the evidence contract to require the total concurrent allocation ceiling.
📍 Affects 4 files
python/fast_mlsirm/scaling.py#L508-L538(this comment)CHANGELOG.md#L8-L8docs/changelog.d/719-scaling-ranking-csr-allocation-budget.md#L5-L5docs/doctoring/lsr_ranking_csr_allocation_budget.md#L13-L14docs/doctoring/lsr_ranking_csr_allocation_budget.md#L21-L22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/fast_mlsirm/scaling.py` around lines 508 - 538, Update the CSR growth
logic around flat and starts so replacement allocations include both live
source-buffer bytes and replacement-buffer bytes, rejecting before allocation or
using another strategy when the MAX_RANKING_CSR_BYTES ceiling cannot be
preserved; update python/fast_mlsirm/scaling.py lines 508-538 accordingly. In
CHANGELOG.md line 8, claim the live-budget guarantee only after this overlap
accounting is enforced. Correct the intermediate-allocation claim in
docs/changelog.d/719-scaling-ranking-csr-allocation-budget.md line 5, document
replacement-overlap accounting at
docs/doctoring/lsr_ranking_csr_allocation_budget.md lines 13-14, and require the
total concurrent allocation ceiling in its evidence contract at lines 21-22.
Validate rankings into pure-Python integer lists first, then allocate exact-size uint64 flat/start arrays once so reallocation peaks cannot exceed MAX_RANKING_CSR_BYTES when old and replacement buffers are live.
seonghobae
marked this pull request as ready for review
August 11, 2026 11:12
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Advances #719. Ranking CSR materialization enforced the byte budget on logical counts but geometric
np.emptygrowth and list→uint64copies could allocate intermediate capacities beyond the declared live payload ceiling.The first GREEN attempt removed the list→
uint64temporary and capped each replacement buffer individually, but fresh exact-current review found a remaining transient live-allocation accounting defect: during reallocation the old buffer and its replacement coexist until assignment, soold_flat + new_flat + startsorflat + old_starts + new_startscan exceedMAX_RANKING_CSR_BYTESeven when every single allocation and final payload fits.Current test-first state
Exact Draft head:
0ecf1704b9ef85150ea45a68aa6014134e98d836over protected base3bc1222bf9552bb74f98cf34c89eba0b8d29c0dc.A new fail-first regression tracks weak references to live
uint64buffers during one tiny-budget growth and requires the simultaneous live bytes, including the old and replacement arrays, never exceed the declared budget. Keep Draft until CI proves the intended RED at that production boundary; setup/import/fixture failure is not valid RED.Minimum GREEN after exact RED proof
uint64temporaries;n+1inner consumption, total/outer bounds, accepted one-shot generators, stable redacted iteration errors, process-control propagation, duplicate/out-of-range semantics, contiguousuint64Rust transport and exact LSR/I-LSR numerical results;Do not widen into
_top1_to_csr(#632), Rust ranking arithmetic, dependencies/workflows/version/release, hosted state or canonical #604 docs. Draft #719 remains the predecessor RED line and will be closed only after a reviewed replacement reaches protected main; no checks/reviews transfer.Summary by CodeRabbit
Bug Fixes
Documentation