Skip to content

feat(retrieval): flip use_bm25f_anchors default-on at v1.7.0 (#154) - #430

Merged
robotrocketscience merged 2 commits into
mainfrom
feat/issue-154-bm25f-default-on-v2
May 5, 2026
Merged

feat(retrieval): flip use_bm25f_anchors default-on at v1.7.0 (#154)#430
robotrocketscience merged 2 commits into
mainfrom
feat/issue-154-bm25f-default-on-v2

Conversation

@yoshi280

@yoshi280 yoshi280 commented May 5, 2026

Copy link
Copy Markdown
Collaborator

Lands the v1.7 default-on flip for use_bm25f_anchors. Stacked on PR #428 (Porter stemming) — that has to merge first; this PR's base is set to that branch.

What ships (2 commits on top of #428)

  1. perf(bm25): LRU-memoise Porter stem — 64K-entry lru_cache over _stem(token). Stemming added per-doc cost that pushed two 5s-timeout tests over the edge at 10k beliefs (test_ac4_median_latency_under_200ms_on_10k_belief_store, test_fire_cap_independent_per_session). Zipfian token distribution → high cache hit rate; both timeouts clear.

  2. feat(retrieval): flip use_bm25f_anchors default-on at v1.7.0resolve_use_bm25f_anchors default False → True. Three regression tests pinned to explicit flag values:

    • test_ac2_weight_zero_byte_identical_to_v10x — explicit use_bm25f_anchors=False to keep the v1.0.x byte-identity check.
    • test_retrieve_default_off_byte_identical_to_pre_v15_path → renamed test_retrieve_default_on_byte_identical_to_explicit_on; pinned to True.
    • test_lane_telemetry_records_fts5_lane_by_default → renamed test_lane_telemetry_records_fts5_lane_when_opted_out (pinned to False); new test_lane_telemetry_records_bm25f_lane_by_default asserts the new contract.

Bench evidence

Opt-out paths preserved

Callers that want the legacy FTS5 path can still:

  • AELFRICE_BM25F=0 env var
  • use_bm25f_anchors=False kwarg
  • [retrieval] use_bm25f_anchors = false in .aelfrice.toml

Test plan

  • uv run pytest --ignore=tests/bench_gate -q — 2457 passed, 23 skipped.
  • AELFRICE_CORPUS_ROOT=... uv run pytest tests/bench_gate/test_retrieve_uplift.py — PASS.
  • Reproduction: q="banana" against content "bananas" returns ['F1'] under default settings.
  • Latency tests under 5s timeout at 10k beliefs.

Out of scope

Summary by Sourcery

Flip BM25F anchor-based retrieval to be enabled by default while preserving opt-out paths and stabilizing performance with Porter stemming memoization.

New Features:

  • Enable BM25F anchor-based retrieval by default via the resolve_use_bm25f_anchors configuration flow.

Enhancements:

  • Add LRU-memoized Porter stemming to improve BM25 index build performance on large corpora.
  • Clarify and update regression and telemetry tests to reflect the new default-on BM25F behavior while keeping legacy FTS5 behavior explicitly test-covered.

Tests:

  • Adjust and extend telemetry, regression, and byte-identity tests to assert the new default-on BM25F behavior and ensure legacy FTS5 paths remain reachable.

Adding stemming to tokenize_stemmed() raised per-doc tokenisation
cost enough to push two 5s-timeout tests over the line at 10k
beliefs:

- test_ac4_median_latency_under_200ms_on_10k_belief_store
- test_fire_cap_independent_per_session

Real corpora are Zipfian — the same tokens recur frequently across
documents — so a 64K-entry LRU on stemWord() has very high hit
rate after warm-up. Cache is module-global; reset on process exit.
Both timeouts clear after this commit.

This is needed before the default-on flip (which makes BM25F the
hot path on every retrieve() call) to keep retrieve() perf
characteristic similar to the v1.5/v1.6 FTS5 baseline.
resolve_use_bm25f_anchors default flipped False → True per the bench
evidence at #154:

- comment 4380842909: pre-stem +0.6010 NDCG@k uplift on the v0.1
  retrieve_uplift fixture (30 rows, 6 categories).
- comment 4380967901: post-stem +0.6650 NDCG@k uplift after the
  Porter stemmer addition closed the q="banana" vs content
  "bananas" gap.
- bench-gate test_retrieve_per_flag_no_regression: PASS — no per-row
  regression vs all-flags-off baseline.

Three regression tests pinned to explicit flag values to assert the
contract (default-on, opt-out path intact):

- test_ac2_weight_zero_byte_identical_to_v10x: explicit
  use_bm25f_anchors=False to keep the v1.0.x byte-identity check.
- test_retrieve_default_off_byte_identical_to_pre_v15_path renamed
  to test_retrieve_default_on_byte_identical_to_explicit_on; pinned
  to use_bm25f_anchors=True since the default flipped.
- test_lane_telemetry_records_fts5_lane_by_default renamed to
  test_lane_telemetry_records_fts5_lane_when_opted_out (pinned to
  False); new test_lane_telemetry_records_bm25f_lane_by_default
  asserts the new default contract.

Opt-out remains via AELFRICE_BM25F=0, kwarg use_bm25f_anchors=False,
or [retrieval] use_bm25f_anchors = false in .aelfrice.toml.

Closes the v1.7 default-on flip portion of #154.
@coderabbitai

coderabbitai Bot commented May 5, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)
  • main

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

Run ID: 8979f883-8083-4982-af2d-9c0acb527257

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
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/issue-154-bm25f-default-on-v2

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 and usage tips.

@sourcery-ai

sourcery-ai Bot commented May 5, 2026

Copy link
Copy Markdown

Reviewer's Guide

Implements a performance optimization for Porter stemming in the BM25 index via an LRU cache and flips the retrieval default to use BM25F anchors at v1.7.0, updating the resolution logic and regression tests to enforce and document the new default while preserving explicit opt-out paths and legacy behavior checks.

Sequence diagram for resolve_use_bm25f_anchors decision order

sequenceDiagram
    participant caller
    participant resolve_use_bm25f_anchors
    participant env_bm25f_override
    participant toml_reader

    caller->>resolve_use_bm25f_anchors: call(explicit, start)
    resolve_use_bm25f_anchors->>env_bm25f_override: env = _env_bm25f_override()
    env_bm25f_override-->>resolve_use_bm25f_anchors: env or None
    alt env is not None
        resolve_use_bm25f_anchors-->>caller: return env
    else env is None
        alt explicit is not None
            resolve_use_bm25f_anchors-->>caller: return explicit
        else explicit is None
            resolve_use_bm25f_anchors->>toml_reader: toml_value = _read_toml_flag_for(BM25F_FLAG, start)
            toml_reader-->>resolve_use_bm25f_anchors: toml_value or None
            alt toml_value is not None
                resolve_use_bm25f_anchors-->>caller: return toml_value
            else toml_value is None
                resolve_use_bm25f_anchors-->>caller: return True
            end
        end
    end
Loading

Class diagram for BM25 stemming cache and retrieval flag resolution

classDiagram
    class Bm25Module {
        +snowballstemmer stemmer porter_stemmer
        +_stem(token str) str
        +tokenize_stemmed(text str) list_str
    }

    class PorterStemmerLRUCache {
        +int maxsize
        +_stem_cache
        +_stem(token str) str
    }

    class RetrievalConfigResolver {
        +resolve_use_bm25f_anchors(explicit bool, start Path) bool
        +_env_bm25f_override() bool
        +_read_toml_flag_for(flag str, start Path) bool
    }

    Bm25Module ..> PorterStemmerLRUCache : uses
    Bm25Module : tokenize_stemmed(text) calls _stem(token)
    PorterStemmerLRUCache : _stem(token) uses porter_stemmer

    RetrievalConfigResolver : resolve_use_bm25f_anchors uses default True
    RetrievalConfigResolver : resolve_use_bm25f_anchors prefers env
    RetrievalConfigResolver : then explicit kwarg
    RetrievalConfigResolver : then toml flag
Loading

File-Level Changes

Change Details Files
Optimize Porter stemming in BM25 tokenization using an LRU cache to recover latency lost by stemming at larger corpus sizes.
  • Introduce a module-global _stem() helper that wraps snowballstemmer.stemWord with an lru_cache of size 65,536.
  • Refactor tokenize_stemmed() to call the cached _stem() helper instead of invoking stemWord directly on each token.
  • Document the performance rationale (Zipfian token distribution, pure-Python stemmer cost, cache lifetime) in the _stem() docstring.
src/aelfrice/bm25.py
Flip resolve_use_bm25f_anchors default from False to True at v1.7.0 and update documentation to reference bench evidence and remaining opt-out mechanisms.
  • Change resolve_use_bm25f_anchors to return True when no env, explicit flag, or TOML config is provided.
  • Update the resolve_use_bm25f_anchors docstring to describe the v1.7.0 default-on contract, reference the uplift benchmark, and explicitly list opt-out paths.
  • Keep environment variable and TOML override precedence unchanged so only the final fallback default is flipped.
src/aelfrice/retrieval.py
Adjust LaneTelemetry tests to reflect BM25F as the default lane while explicitly testing the legacy FTS5 opt-out path.
  • Rename the previous default-lane test to assert that use_bm25f_anchors=False yields bm25f_used=False, confirming the FTS5 path is still reachable.
  • Add a new test that retrieve() with no flag set produces bm25f_used=True in LaneTelemetry, enforcing the default-on behavior.
  • Clarify test docstrings to tie behavior to the v1.7.0 default flip and bench evidence.
tests/test_composition_tracker.py
Update retrieval byte-identity regression tests to pin expectations to the new default-on BM25F contract while preserving explicit checks for the legacy BM25 path.
  • Change the BM25 index regression test to assert that default retrieve() output matches the explicit use_bm25f_anchors=True path, replacing the previous default-off vs explicit False equality check.
  • Extend the Bayesian ranking regression test to pass use_bm25f_anchors=False when checking posterior_weight=0.0 byte-identity against the legacy search_beliefs() path, so it continues to validate the non-BM25F behavior.
  • Update test names and docstrings to describe the v1.7.0 default-on contract, reference the benchmarked uplift, and clarify which behavior each test is pinning (default vs legacy).
tests/test_bm25_index.py
tests/test_bayesian_ranking.py

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

robotrocketscience added a commit that referenced this pull request May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and
the actual default-on flip:

- BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154
  bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift
  fixture under Porter stemming. PR #428 added the stemmer; PR #430
  flipped the default; bench-gate test_retrieve_per_flag_no_regression
  PASS.
- Other v1.7 components (use_signed_laplacian, use_heat_kernel,
  use_hrr_structural) remain opt-in — placeholder lanes pending
  wiring into retrieve(). The v1.7 wave is shipped; the
  remaining-flags flip waits on those lanes landing.
- v2.0 row drops the "default-on flip is a prereq" note since
  v1.7 is now shipped.

Replaces the prior intermediate framing ("shipped (opt-in)";
deferred default-on flip).

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@yoshi280

yoshi280 commented May 5, 2026

Copy link
Copy Markdown
Collaborator Author

[claim:review:Kulili:2026-05-05T16:17:27Z]

Base automatically changed from feat/issue-154-bm25f-stemming to main May 5, 2026 16:33
@yoshi280 yoshi280 closed this May 5, 2026
@yoshi280 yoshi280 reopened this May 5, 2026
@robotrocketscience
robotrocketscience merged commit 95e563b into main May 5, 2026
22 of 28 checks passed
@robotrocketscience
robotrocketscience deleted the feat/issue-154-bm25f-default-on-v2 branch May 5, 2026 16:36
robotrocketscience added a commit that referenced this pull request May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and
the actual default-on flip:

- BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154
  bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift
  fixture under Porter stemming. PR #428 added the stemmer; PR #430
  flipped the default; bench-gate test_retrieve_per_flag_no_regression
  PASS.
- Other v1.7 components (use_signed_laplacian, use_heat_kernel,
  use_hrr_structural) remain opt-in — placeholder lanes pending
  wiring into retrieve(). The v1.7 wave is shipped; the
  remaining-flags flip waits on those lanes landing.
- v2.0 row drops the "default-on flip is a prereq" note since
  v1.7 is now shipped.

Replaces the prior intermediate framing ("shipped (opt-in)";
deferred default-on flip).
robotrocketscience added a commit that referenced this pull request May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and
the actual default-on flip:

- BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154
  bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift
  fixture under Porter stemming. PR #428 added the stemmer; PR #430
  flipped the default; bench-gate test_retrieve_per_flag_no_regression
  PASS.
- Other v1.7 components (use_signed_laplacian, use_heat_kernel,
  use_hrr_structural) remain opt-in — placeholder lanes pending
  wiring into retrieve(). The v1.7 wave is shipped; the
  remaining-flags flip waits on those lanes landing.
- v2.0 row drops the "default-on flip is a prereq" note since
  v1.7 is now shipped.

Replaces the prior intermediate framing ("shipped (opt-in)";
deferred default-on flip).
robotrocketscience added a commit that referenced this pull request May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and
the actual default-on flip:

- BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154
  bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift
  fixture under Porter stemming. PR #428 added the stemmer; PR #430
  flipped the default; bench-gate test_retrieve_per_flag_no_regression
  PASS.
- Other v1.7 components (use_signed_laplacian, use_heat_kernel,
  use_hrr_structural) remain opt-in — placeholder lanes pending
  wiring into retrieve(). The v1.7 wave is shipped; the
  remaining-flags flip waits on those lanes landing.
- v2.0 row drops the "default-on flip is a prereq" note since
  v1.7 is now shipped.

Replaces the prior intermediate framing ("shipped (opt-in)";
deferred default-on flip).
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.

2 participants