Skip to content

test(scaling): fail first on top-1 CSR input bounds - #731

Closed
seonghobae wants to merge 1 commit into
mainfrom
test/top1-csr-bounds-red-632
Closed

test(scaling): fail first on top-1 CSR input bounds#731
seonghobae wants to merge 1 commit into
mainfrom
test/top1-csr-bounds-red-632

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Buyer-visible resource/trust-boundary gap

Advances #632 after the shared ranking boundary reached protected main through #727. Public lsr_top1/ilsr_top1 still route through _top1_to_csr(), which currently consumes the outer observation iterable without a package-owned count/resource ceiling and materializes each caller-controlled loser iterable with list(losers) before stable bounded validation. Ordinary outer/inner iterator failures can therefore escape caller-controlled exception text, and winner/loser/start uint64 payload is not checked against the accepted shared CSR byte budget.

Intentional fail-first contract

Exact test head: 58b3ba56b844fc63a172b257a44346ff987243e0, created from protected commit ae36c4a2efaa8f3bf3d25befa0069826be54266e after #727. Protected main subsequently advanced non-overlapping inference tests/doctoring to 3afb302ae94e7c996d6c64e6871d35e3c288ddcd via #730; this Draft must use fresh live-base integration evidence before any GREEN/Ready decision.

tests/test_scaling_top1_input_bounds.py requires the public top-1 materialization boundary to:

  • bound impossible loser streams before an unbounded caller next();
  • normalize ordinary outer and inner iterator failures to stable non-reflective ValueError messages;
  • apply the accepted package-owned MAX_RANKING_CSR_BYTES ceiling to simultaneously encoded winner/loser/start uint64 payload;
  • accept the exact 32-byte boundary for one winner/one loser/two start offsets without changing contiguous Rust transport; and
  • continue propagating KeyboardInterrupt, SystemExit, and GeneratorExit.

Current protected code is expected to RED at the actual production boundary because it uses for ... in data, list(losers), Python lists, and unbudgeted np.asarray(..., dtype=np.uint64) handoff. Setup/import/fixture failure is not valid RED.

Minimum GREEN after exact RED proof

  1. Reuse the accepted shared CSR checked-byte convention from protected main rather than inventing a second resource model.
  2. Consume the outer and inner iterables through explicit package-owned bounds; no unbounded list(losers) or equivalent materialization.
  3. Normalize ordinary caller iteration failures without reflecting exception text while preserving process-control exceptions.
  4. Account for winner, loser, and start-offset fixed-width handoff arrays before allocation; include any package-owned temporary that would coexist with those arrays, or use an exact-size strategy that avoids such a temporary.
  5. Preserve existing integer/range/empty-set semantics, Rust duplicate/winner-in-losers validation, contiguous np.uint64 transport, and exact LSR/I-LSR numerical results.
  6. Add focused boundary-1/boundary/boundary+1, accepted list/tuple/one-shot-generator and Rust-parity regressions, complete docstrings, Python 3.14 primary technical doctoring, and authoritative changelog material.
  7. Require one unchanged final head with full Python 3.12/3.14, Rust/PyO3, package/reinstall/release acceptance, GPU-existing, fuzz, Security Scan, SAST, current-head review, and zero valid unresolved findings.

Do not widen into ranking arithmetic, CAT/ATA work, dependencies/workflows/version/release, hosted state, or canonical #604 documentation. Keep Draft through RED→GREEN.

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 14d02702-575e-4df7-b1e7-70efdc8ed11f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

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

Copy link
Copy Markdown
Contributor Author

@opencode-agent address

Take the sole bounded GREEN writer lease for Draft #731 / issue #632 only after a final refetch confirms exact source head 58b3ba56b844fc63a172b257a44346ff987243e0, protected main 3afb302ae94e7c996d6c64e6871d35e3c288ddcd, exact python/fast_mlsirm/scaling.py blob d82e4e34c08c2c54f6117275fc6c5717759e0322, and no other current source writer. Abort without writing if any identity moved.

The fail-first boundary is proven on CI run 31485942111, Python 3.12 job 93761237308, synthetic merge 6ccc6db9e27f3eb765b382158f885ec892fc3f2b: normal setup, editable Rust/PyO3 build, and Rust-primary resolution succeeded; the full suite reached the real top-1 materialization boundary and failed exactly four new cases while 3,258 passed / 2 skipped. The four REDs are: unbounded list(losers) consumes into caller RuntimeError, ordinary outer iteration leaks caller RuntimeError, ordinary inner loser iteration leaks caller RuntimeError, and a 31-byte shared CSR budget fails to reject the 32-byte winner/loser/start payload. The exact-boundary and process-control tests are already GREEN. Security Scan and SAST are GREEN on the fail-first head. Do not rewrite or erase this RED history.

Before production code, add one further deterministic fail-first regression for the issue's explicit outer observation count ceiling: under a tiny monkeypatched MAX_RANKING_CSR_BYTES, use a finite probe that would keep yielding (winner, one_loser) and assert the package rejects at the derived maximum with at most one bounded look-ahead rather than reaching a sentinel/unbounded next(). Run it alone and preserve its RED evidence.

Then implement the narrowest GREEN in _top1_to_csr only:

  1. Reuse the accepted protected-main fixed-width MAX_RANKING_CSR_BYTES convention from _rankings_to_csr; do not create a second independent memory policy.
  2. Replace implicit/unbounded outer iteration and list(losers) with explicit iterators. Normalize ordinary iterator construction/next() failures to stable non-reflective package ValueError; propagate KeyboardInterrupt, SystemExit, and GeneratorExit unchanged.
  3. Bound each loser collection to at most n - 1 accepted entries, with at most the necessary one-step exhaustion check; reject an overlong choice set before asking the caller for an unbounded extra value. Preserve Rust ownership of duplicate-loser and winner-in-losers semantic validation.
  4. Derive an explicit outer observation ceiling from the same fixed-width budget and the mandatory minimum one winner + one loser + one new CSR offset per observation. Infinite outer streams must terminate at that ceiling with bounded look-ahead; exact feasible boundary must remain accepted.
  5. Count winner + loser + start-offset uint64 handoff slots before growth/allocation. Include every simultaneously live package-owned fixed-width temporary or, preferably, mirror fix(scaling): honor live CSR allocation budget for rankings #727's validate-then-exact-size-allocate strategy: accumulate bounded admitted Python integers, allocate exact-size contiguous np.uint64 winner/loser/start arrays once, and populate them elementwise so list→uint64 temporaries cannot exceed the declared live payload.
  6. Preserve current n cap, integer/range/negative/empty-loser semantics, accepted list/tuple/one-shot-generator inputs, no-candidate/duplicate behavior in Rust, and exact LSR/I-LSR numerical results.
  7. Expand focused tests with boundary-1/boundary/boundary+1 and public lsr_top1/ilsr_top1 parity/transport evidence without changing Rust stationary-distribution arithmetic.
  8. Add beginner-readable docstrings, Python 3.14 primary technical doctoring and an authoritative changelog fragment only after focused GREEN. Do not render managed CHANGELOG.md until feature GREEN; use the repository renderer, not manual editing.
  9. Run focused tests (tests/test_scaling_top1_input_bounds.py, existing scaling tests, changelog contract), git diff --check, then leave Draft for full unchanged-head Python 3.12/3.14, Rust/PyO3, package/reinstall/release acceptance, existing GPU, fuzz, Security Scan, SAST and current-head review.

Do not widen into ranking arithmetic, CAT/ATA, dependencies/workflows/credentials/version/release, hosted state, or canonical #604 docs. Stop writing immediately if source/base/writer state moves.

Copy link
Copy Markdown
Contributor Author

Superseded by #732. Fresh verification shows the fail-first regression file tests/test_scaling_top1_input_bounds.py is byte-identical on #731 and #732 (same blob 2a73fe60b1c7013052ef731401933182ec8066cf), while #732 adds the bounded production implementation, doctoring, changelog fragment, and has exact-head CI/Security/SAST GREEN. Keep #732 as the sole implementation path; #731's historical RED remains useful evidence but must not stay as a competing writer/merge candidate.

@seonghobae seonghobae closed this Aug 11, 2026
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