feat: Rust+PyO3 kernel for BPR's SGD inner loop (phase 2)

[Burton-David]

Phase 2 — Rust+PyO3 kernel for BPR

Phase 1.1 measured BPR fit at 4,503 ms on MovieLens 100k — an order of magnitude slower than anything else in the library. 85% of that was 500,000 Python calls to _step, each doing a few vector ops on 16-dim arrays. Numpy can't help (rows are mutated in-place; loop body is too small for SIMD). The interpreter is the bottleneck. A compiled inner loop is the only intervention that closes the gap.

What's in the PR

• crates/recsys-kernels/ — a new Rust crate exposing one PyO3 function, bpr_train, that runs the full epoch loop in Rust: Fisher–Yates shuffle, uniform negative resampling, the same sigmoid-margin gradient update _step always did. RNG is PCG64 seeded by random_state.

• Build backend Hatchling → maturin. pip install -e . now compiles the extension and drops _kernels.<abi>.so into the package. Building from source requires a Rust toolchain — CONTRIBUTING.md and README.md updated.

• CI installs Rust via dtolnay/rust-toolchain@stable + Swatinem/rust-cache@v2 before the pip install step. Same change to the docs workflow.

• BPR.fit calls the kernel; pure-Python fallback when the extension isn't importable. Every existing tests/test_bpr.py test exercises the fallback path when the Rust install step is skipped.

• tests/test_bpr_kernel.py — smoke test of the binding on tiny inputs, plus a recommendation-quality regression test on MovieLens 100k. The Python and Rust paths consume RNG bytes in different orders, so the float arrays differ; we assert quality clears a conservative floor rather than bit-equivalence.

• __version__ sourced from installed package metadata via importlib.metadata, since maturin doesn't drive a Hatchling-style dynamic version.

ADR

docs/evolution/02-rust-kernel-bpr.md covers context, the options that were rejected (Cython, numba, Hogwild parallel SGD, pre-sampled negatives), and what's deliberately out of scope (ALS is already 9× faster than BPR; recommend() top-N selection is only worth optimizing once Phase 3 unlocks larger catalogs).

Expected impact

	Before (Python)	After (Rust kernel)	Speedup
BPR fit, ML-100k, 5 epochs	4,503 ms	~150 ms target	~30×

Concrete numbers will land in benchmarks/profile.md once CI confirms the build (I don't have a Rust toolchain locally to verify; the change has been written carefully and the Python side is fully lint/format/mypy/pytest-clean, but the Rust compile is happening for the first time in CI).

Verification status

• Python side: ruff check / ruff format --check / mypy clean; pytest 130 passed, 1 skipped (the kernel-equivalence test skips when _kernels isn't importable).
• Rust side: unverified locally — no rustc/cargo on this machine. This PR is the first build of the crate; CI's Rust install step will tell us if there's anything to fix. Marking the PR ready for review on the assumption that quick CI iteration is acceptable; happy to address any compile errors in follow-ups.

[JohnJacob-coder]

Reviewed Phase 2 in depth — checked out the branch, built the crate (maturin develop --release, ~15s clean), ran the kernel tests, and benchmarked. The kernel is correct and fast: 88 ms vs the 4,503 ms Python baseline = 51× on this machine (your ~30× is conservative — confirmed). test_kernel_backed_bpr_matches_python_on_quality and test_bpr_reproducible_for_a_given_seed both pass, and with a real (non-zero) init the kernel writes factors back correctly. The maturin build, the has_negatives-filtered positives (so the kernel's resample loop can't hang), and the import-guard fallback are all sound. Good work — three things before it lands.

Blocking — CI is red on a flawed test. tests/test_bpr_kernel.py::test_bpr_train_kernel_is_callable initializes both factor matrices to np.zeros then asserts they become non-zero. BPR is degenerate at zero init: margin=0, i_vec − j_vec=0, u_vec=0, so every gradient is exactly 0 and the factors never move — a correct kernel still leaves them at zero, which is why this fails. Init with small random values like the real fit (rng.normal(0, 0.01, size=...)); then the kernel produces non-zero updates and the smoke test actually validates write-back. (Confirmed locally: with random init the factors do move.)

ADR accuracy — the divergence isn't only RNG ordering. 02-rust-kernel-bpr.md (the Bit-equivalence bullet) says the arrays differ because the paths consume RNG bytes in different orders. There's a second, deterministic reason: lib.rs:80-82 snapshots u_vec before any update, so the pos/neg item gradients use the pre-update user vector, whereas Python's _step (bpr.py:114-116) reuses the post-update u_vec via numpy view aliasing. The two would differ even with identical RNG. Worth naming — and it's a point in the kernel's favour (textbook simultaneous update; the Python aliasing is the quirk).

ADR accuracy + test gap — the fallback isn't exercised in CI. The Falling back to Python section claims CI runs "both with and without the extension." But every leg in ci.yml runs dtolnay/rust-toolchain@stable + pip install -e .[dev] (maturin backend), so the extension is always built and _fit_python is never hit in CI. Either add a leg that skips the Rust install so the fallback is actually tested, or correct the claim. The fallback is a real shipped path (sdist installs), so I'd lean toward testing it.

Also (non-blocking but expected for a perf phase): the ~30× headline isn't in a committed benchmark — benchmarks/profile.md still shows only the 4,503 ms Python number. The claim is real (I measured 51×), but please commit the evidence: update profile.md and/or extend the Phase 1.1 suite to cover the kernel path so the number is reproducible from the repo, not just the ADR.

Fix the test to clear CI, tighten those two ADR claims, commit the benchmark number, and I'll merge.

[Burton-David]

Re-requesting review — all three of the items from your 20:32 review are addressed in 7a94589 (pushed 20:42 UTC, ~10 minutes after the review). Concretely:

1. ADR — snapshot-vs-aliasing difference. The Bit-equivalence bullet in docs/evolution/02-rust-kernel-bpr.md now spells out both reasons the float arrays diverge: (a) RNG byte ordering and (b) the kernel using the pre-update user row (textbook simultaneous SGD) versus BPR._step reusing the post-update user via numpy view aliasing. Calls out that the kernel's behavior is the standard formulation; the Python aliasing is the quirk.

2. Fallback CI claim — addressed by adding a real test, not by softening the claim. New tests/test_bpr_fallback.py poisons sys.modules['recommender_systems._kernels'] before importing BPR, then runs the training contract end-to-end. The ADR's Falling back to Python section now correctly says CI exercises the kernel on every leg and the fallback through this test.

3. Benchmark number committed. benchmarks/profile.md has a new post-Phase-2 section with the 4,503 ms → 88 ms / ~51× result you measured, hardware caveat noted. The pre-existing test_bpr_fit benchmark in tests/benchmarks/test_fit_speed.py already exercises the kernel path, so the suite records a fresh number every run — no separate kernel-only benchmark needed. ADR's Expected impact table updated to match.

CI green (3.10/3.11/3.12) on the new HEAD.

[JohnJacob-coder]

Re-reviewed 7a94589 — all three items addressed, and I verified each against the rebuilt branch.

1. ADR divergence — fixed. The Equivalence discussion now names both reasons: RNG ordering and the kernel's pre-update snapshot vs _step's post-update view aliasing ("the kernel's formulation is the standard one; the Python aliasing is a quirk"). Accurate.
2. Fallback test — done and genuinely exercises the path. tests/test_bpr_fallback.py poisons sys.modules['recommender_systems._kernels'] before importing BPR. I checked this actually forces the ImportError (with the kernel installed, the suppression still routes fit through _fit_python) — so it's a real test, not a false positive. And __init__.py doesn't eagerly import _kernels, so no-kernel installs work. The ADR's CI claim is corrected to match ("same CI run, no extra job").
3. Benchmark committed. profile.md now has the After-Phase-2 table (4,503 ms → ~88 ms, ~51×), honestly flagged hardware-dependent.

Verified locally: rebuilt with maturin develop --release; ruff + mypy clean; pytest 131 passed, 1 skipped (torch), 7 deselected (benchmarks). CI green on all three legs. Kernel correctness + the ~51× speedup confirmed on the prior pass.

Clean, honest Phase 2 — the measurement-driven retarget from ALS to BPR is carried all the way through. LGTM. (v0.2.0 tag is the phase marker once this lands.)