Skip to content

Harden Candle::Reranker for Benchmate: revision pinning, bounded batching, concurrency safety - #115

Draft
cpetersen wants to merge 1 commit into
mainfrom
feature/reranker-benchmate-hardening
Draft

cpetersen wants to merge 1 commit into
mainfrom
feature/reranker-benchmate-hardening

Conversation

@cpetersen

Copy link
Copy Markdown
Member

Summary

Hardens the existing Candle::Reranker (no new wrapper, Benchmate untouched) for
Benchmate's in-process cross-encoder reranking use case: cross-encoder/ms-marco-MiniLM-L6-v2
pinned to 233902d25c440f23af6f7d6e94d2946bac0bee0a, CPU-first, up to 100 candidates per
call, as a memoized singleton shared by concurrent Puma threads.

  • revision: on from_pretrained — pins config/tokenizer/weights to an exact HF
    revision via Repo::with_revision; never falls back to the default branch when supplied.
    Surfaced in options/inspect; included in load/inference error messages.
  • batch_size: (default 16) — documents are processed in bounded micro-batches instead
    of one tensor for every candidate. Results are merged and sorted once globally (descending
    score, stable tie-break on original index); doc_id is preserved across batches.
  • Concurrency: a single reranker instance is safe for concurrent calls from multiple
    Ruby threads for BERT/XLM-RoBERTa/DeBERTa/ModernBERT (the Benchmate target architecture) —
    candle_core::Tensor is Arc<RwLock<Storage>> by design for concurrent reads, and these
    variants have no interior mutability, so no locking is added for them (true parallel
    throughput). Qwen3 had a real bug: its KV cache lived in a RefCell, mutated during
    compute_rerank(&self, ...) — since the GVL is released for compute, concurrent Ruby
    threads could genuinely race on that RefCell's non-atomic borrow counter (real UB, not
    just a Ruby-level race). Fixed with a narrowly-scoped Mutex<Qwen3Model> (only that
    variant pays the cost).

Two findings from investigation (both fixed, both in scope)

  1. Model id typo in the original spec: cross-encoder/ms-marco-MiniLM-L-6-v2 (hyphen
    before 6) 404s. The real repo is cross-encoder/ms-marco-MiniLM-L6-v2 — confirmed via
    the HF API, and its current main SHA matches the pinned revision exactly. All code/tests
    use the corrected id.
  2. Real latent correctness bug, now fixed: candle-transformers' BertEmbeddings assigns
    position ids as flat 0..seq_len regardless of padding (see its own
    // TODO: Proper absolute positions?). Red Candle padded left, so any batch with
    documents of different token lengths gave the shorter documents wrong position embeddings
    — silently, no error. This predates this PR and already affected the existing
    (single-tensor) reranker whenever it processes >1 document of differing length. Fixed by
    padding right instead, scoped to the reranker only (EmbeddingModel/NER unchanged).

Regression proof (old fails, new passes)

Built an old-implementation worktree from main, reranked two same-query documents of very
different length (43 vs 21 tokens) in one batch, compared the shorter document's raw logit
against the Python reference (computed padding-independently, batch-of-one):

short-doc raw logit vs. Python reference (-11.200197)
old (left-padded, main) -10.434481 diff 0.7657 — bug reproduced
new (right-padded, this PR) -11.200198 diff 0.0000009 — matches

Correctness: parity with Python Sentence Transformers

  • scripts/generate_reranker_parity_fixtures.py loads the pinned revision via
    transformers, asserts its state_dict has exactly the keys RerankerModel::Bert
    expects (bert.pooler.dense.*, classifier.*), and scores 7 query/doc pairs (clear
    match, clear mismatch, near-duplicates, one forcing truncation at 512 tokens) one at a
    time (batch-of-one, no padding — isolates model correctness from the padding fix above).
  • Fixture committed at spec/fixtures/reranker_parity/ms_marco_minilm_l6.json
    (torch==2.13.0, transformers==5.16.1, sentence-transformers==6.0.0).
  • spec/reranker_parity_spec.rb asserts agreement within 1e-3 (raw logit) / 1e-4
    (sigmoid) — ~100x the measured max deviation (~8e-6 raw, ~0 sigmoid) on this machine, to
    absorb backend/platform float differences without masking a real regression. See
    docs/RERANKER_PARITY.md.

Benchmarks

bench/reranker_bench.rb sweeps candidates × [10,25,50,100] × max_length ×
[128,256,512] × batch_size × [8,16,32]; bench/reranker_memory_growth.rb isolates the
repeated-call memory check. Exact commands and full results (including a concurrency
1-vs-4-callers table) are in docs/RERANKER_BENCHMARKS.md.

Run on macOS arm64 (no Linux amd64 machine available) — labeled as such throughout,
not a substitute for Linux amd64 numbers. Peak RSS across the whole sweep: ~0.76 GiB,
comfortably under the 3 GiB budget at every batch_size tested, so the default of 16
stands (no memory-driven reason to change it from this data). Please re-run
bundle exec ruby bench/reranker_bench.rb on Linux amd64 CPU for the authoritative
numbers — command documented in docs/RERANKER_BENCHMARKS.md.

Tests

bundle exec rspec spec/reranker*.rb spec/gvl_release_spec.rb — 57 examples, 0 failures.
New: reranker_revision_spec.rb, reranker_batching_spec.rb (incl. a forced-tie
stable-sort test), reranker_concurrency_spec.rb (8 threads × 10 calls each on one
singleton, deterministic-result assertion, deadlock-timeout guard), reranker_parity_spec.rb.
Registered in .github/workflows/build.yml's per-file model-spec job so they actually run
in CI (that job lists spec files explicitly rather than globbing).

Full default suite (bundle exec rake spec, excludes LLM specs): 481 examples, 2 failures
— both pre-existing LLM timing/generation-length flakiness (tinyllama_spec.rb,
llama_spec.rb), unrelated to this change and not reproduced when run in isolation from a
concurrent benchmark process.

Not done / known gaps

  • cargo fmt --check needs the project's nightly-only rustfmt options (unrelated,
    pre-existing across the whole crate — not run).
  • cargo clippy has pre-existing warnings/an unrelated compile error elsewhere in the
    crate (vlm.rs) blocking a full clean run; reranker.rs itself has one pre-existing-style
    large_enum_variant warning (cosmetic, not a correctness issue — the enum was already
    variant-size-imbalanced before this PR).
  • Linux amd64 CPU benchmark numbers — command documented, not run by me (no Linux amd64
    machine).

🤖 Generated with Claude Code

https://claude.ai/code/session_01STHtRNA8VsM9ZMrGF1c6iW

…ching, concurrency safety

Adds revision:/batch_size: to Reranker.from_pretrained, fixes a real Qwen3 concurrency
bug (RefCell -> Mutex; a genuine data race under GVL-released concurrent calls), and
fixes a latent BERT correctness bug where left-padding + candle-transformers' unoffset
position ids gave wrong scores to shorter documents in a mixed-length batch (now
right-padded, scoped to the reranker only). Verified against Python Sentence
Transformers for cross-encoder/ms-marco-MiniLM-L6-v2 (parity fixture + spec), with a
regression proof that the old implementation diverges by ~0.77 raw-logit on a
mixed-length batch while the new one matches Python within ~1e-6. Benchmarked (arm64
sample data; Linux amd64 command documented) across candidate count x max_length x
batch_size — peak RSS ~0.76 GiB, well under the 3 GiB budget at every batch_size tested.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STHtRNA8VsM9ZMrGF1c6iW

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