new: support pluggable stemmer and inline stopwords for Bm25 - #655
new: support pluggable stemmer and inline stopwords for Bm25#655Mohith26 wants to merge 2 commits into
Conversation
Allow passing a custom stemmer (any object with a stem_word(word) -> str method, per the new Stemmer protocol) to Bm25, overriding the default SnowballStemmer. When a custom stemmer is provided, the supported-languages check is skipped, enabling languages without a Snowball algorithm such as Polish, Czech, Ukrainian, Slovak, Bulgarian, or Vietnamese. Also allow passing stopwords inline, overriding the per-language stopwords file shipped with the model. Both parameters are forwarded to parallel workers. Default behavior is unchanged when neither parameter is given. Fixes qdrant#654
- custom stemmer callable is applied to tokens - default Snowball path is unchanged - unsupported language (Polish) works with a custom stemmer and still raises without one - inline stopwords override the file-based stopwords
📝 WalkthroughWalkthroughBM25 now exposes a typed custom Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@fastembed/sparse/bm25.py`:
- Around line 130-131: Update the stopword initialization in the BM25
constructor to lowercase each supplied stopword before storing it in
_custom_stopwords, matching the normalization performed by _stem(); preserve
None handling and add a mixed-case stopword test.
- Around line 125-126: Update the constructor’s stopwords-loading logic so
provided stemmers cannot bypass language validation when fallback files are
used. Only open model_dir / f"{language}.txt" for supported languages; when
language is unsupported and stopwords are not inline, use an empty stopwords set
instead, while preserving inline stopwords behavior.
- Around line 222-223: Make the custom stemmer path safe for parallel execution:
document on Bm25’s stemmer configuration that values passed through
Bm25Worker.start must be pickleable and importable, and add a parallel test
using a module-level stemmer. Alternatively, replace direct stemmer instances
with an importable factory or configuration payload that Bm25 can reconstruct in
worker processes.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7486b40-d261-444f-834a-b33549119cd5
📒 Files selected for processing (2)
fastembed/sparse/bm25.pytests/test_sparse_embeddings.py
| if stemmer is None and language not in supported_languages: | ||
| raise ValueError(f"{language} language is not supported") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline fastembed/sparse/bm25.py --view expanded || true
echo "== relevant source =="
sed -n '1,220p' fastembed/sparse/bm25.py | cat -n
echo "== occurrences of load_stopwords and language validation =="
rg -n "_load_stopwords|supported_languages|stopwords|language" fastembed/sparse/bm25.pyRepository: qdrant/fastembed
Length of output: 12445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Path traversal probe in Python =="
python3 - <<'PY'
from pathlib import Path
tmp = Path("/tmp")
for language in ["english", "../../../etc/"], ["custom", tmp / ".." / "tmp"]:
# Avoid creating files; only inspect normalized paths and whether a fake model dir root can contain the result.
path = tmp / f"{language}.txt" if isinstance(language, str) else tmp / f"{language}.txt"
print(language, "=>", path, "=>", path.resolve(), "inside_tmp", path.resolve().is_relative_to(tmp))
PY
echo "== Worker pass-through =="
sed -n '366,430p' fastembed/sparse/bm25.py | cat -n
echo "== search for custom stemmer usage / tests =="
rg -n "Bm25\\(|stemmer=|disable_stemmer=|stopwords=" --glob '*.py' . | head -200Repository: qdrant/fastembed
Length of output: 2748
Prevent unsupported language values from being used as stopwords file names.
When stemmer is provided, the constructor skips supported_languages, but fallback stopwords still open model_dir / f"{language}.txt" and path traversal values can read arbitrary readable files outside the model directory. Load stopwords only for supported languages or when stopwords is inline, otherwise default to an empty set.
[low_effort and high_reward]
🤖 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 `@fastembed/sparse/bm25.py` around lines 125 - 126, Update the constructor’s
stopwords-loading logic so provided stemmers cannot bypass language validation
when fallback files are used. Only open model_dir / f"{language}.txt" for
supported languages; when language is unsupported and stopwords are not inline,
use an empty stopwords set instead, while preserving inline stopwords behavior.
| self._custom_stemmer = stemmer | ||
| self._custom_stopwords = set(stopwords) if stopwords is not None else None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize inline stopwords to match token normalization.
_stem() lowercases tokens, but set(stopwords) preserves caller casing; stopwords=["The"] will not filter "the". Store lowercase stopwords and add a mixed-case test.
🤖 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 `@fastembed/sparse/bm25.py` around lines 130 - 131, Update the stopword
initialization in the BM25 constructor to lowercase each supplied stopword
before storing it in _custom_stopwords, matching the normalization performed by
_stem(); preserve None handling and add a mixed-case stopword test.
| "stemmer": self._custom_stemmer, | ||
| "stopwords": self._custom_stopwords, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate bm25.py =="
fd -a 'bm25.py$' . || true
echo "== git status/stat =="
git status --short || true
git diff --stat || true
echo "== relevant bm25.py excerpt =="
file="$(fd 'bm25.py$' . | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,280p' "$file" | cat -n
fi
echo "== search parallel/forkserver/spawn/pool/embed signatures =="
rg -n "parallel|forkserver|spawn|multiprocessing|Pool|Parallel|embed\(" -S . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' || true
echo "== tests referencing bm25/custom stemmer =="
rg -n "_FakePolishStemmer|custom_stemmer|custom stop|BM25|bm25|parallel=" -S . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**' || trueRepository: qdrant/fastembed
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect parallel_processor.py =="
wc -l fastembed/parallel_processor.py
sed -n '1,180p' fastembed/parallel_processor.py | cat -n
echo "== inspect bm25 worker/reconstructor and raw_embed tail =="
sed -n '280,390p' fastembed/sparse/bm25.py | cat -n
echo "== inspect tests for bm25 custom stemrer =="
rg -n "_FakePolishStemmer|custom stemmer|custom_stemmer|stemmer=|parallel=" tests -S --glob '!build/**' --glob '!dist/**' || true
fd -i 'bm25*test*.py|test*bm25*.py' tests | while read -r f; do echo "--- $f"; wc -l "$f"; sed -n '1,260p' "$f" | cat -n; doneRepository: qdrant/fastembed
Length of output: 18573
🌐 Web query:
Python multiprocessing Process pickle worker arguments forkserver spawn source documentation pickable arguments
💡 Result:
In Python's multiprocessing module, all arguments passed to a Process (specifically via the args and kwargs parameters) must be picklable [1][2][3]. This requirement exists because, regardless of the start method—'spawn', 'forkserver', or 'fork'—the multiprocessing module must be able to serialize these arguments to transmit them to the child process [1][4]. Start Method Overview: 1. 'spawn': The parent process starts a fresh Python interpreter [1][2]. Because it is a clean process, it cannot access the parent's memory, necessitating the pickling of target functions and all arguments [1][5]. 2. 'forkserver': A server process is started at the beginning of the program [1][5]. When a new process is requested, the parent communicates with this server to fork a new process [6][5]. Since the server is a separate, single-threaded process, arguments must also be picklable to be transferred [1][4]. 3. 'fork': The parent process forks the interpreter using os.fork [5]. While the child technically inherits the parent's memory space, consistency across different platforms and the potential for complex behavior often makes it best practice to treat arguments as needing to be picklable even when using 'fork' [1][4]. Key Practical Considerations: - Main Protection: You must wrap your process creation code in an if name == 'main': block [1][2]. This prevents the child process from recursively attempting to spawn new processes when it imports the main module [7]. - Picklability: If you attempt to pass non-picklable objects (such as locally defined functions, lambdas, or certain system handles like Locks directly) as arguments, you will encounter pickling errors [1][3]. - Recommendation: For robust, cross-platform code, always design your target functions and arguments to be serializable and define target functions at the module level rather than inside other functions [1][7].
Citations:
- 1: https://docs.python.org/3/library/multiprocessing.html
- 2: https://github.com/python/cpython/blob/main/Doc/library/multiprocessing.rst
- 3: https://www.digitalocean.com/community/tutorials/python-multiprocessing-example
- 4: https://discuss.python.org/t/switching-default-multiprocessing-context-to-spawn-on-posix-as-well/21868
- 5: https://github.com/python/cpython/blob/3.11/Doc/library/multiprocessing.rst
- 6: https://docs.python.org/3.11/library/multiprocessing.html
- 7: https://pymotw.com/2/multiprocessing/basics.html
🌐 Web query:
Python standard library snowballstemmer PyRustStemmer pickle
💡 Result:
The snowballstemmer library and related tools like PyRustStemmer are not part of the Python standard library. The Python standard library does not include built-in support for linguistic stemming algorithms. Here is a breakdown of the components you mentioned: 1. snowballstemmer: This is a third-party, pure-Python library that provides Snowball stemming algorithms [1][2]. It is available on PyPI and must be installed separately (e.g., via pip) [1][3]. It acts as a wrapper that can automatically detect and use the faster C-based PyStemmer library if it is installed [1][4]. 2. PyRustStemmer (py-rust-stemmers): This is also a third-party library [5]. It provides a high-performance Python wrapper around the Rust-based snowball-stemming algorithms [5]. It is designed for faster execution, including support for parallel processing, and is managed as an independent project [5][6]. 3. Pickle: Unlike the other items mentioned, the pickle module is part of the Python standard library. It is used for serializing and de-serializing Python object structures, allowing you to convert complex objects into a byte stream for storage or transmission and then reconstruct them later. For stemming tasks in Python, you typically use third-party libraries such as snowballstemmer, PyStemmer, or NLTK (which includes its own implementation of Snowball stemmers) [1][7][8].
Citations:
- 1: https://pypi.org/project/snowballstemmer/
- 2: https://doc.sagemath.org/html/en/reference/spkg/snowballstemmer.html
- 3: https://pypi.org/project/snowballstemmer/3.0.1/
- 4: https://github.com/snowballstem/pystemmer/
- 5: https://github.com/qdrant/py-rust-stemmers/blob/master/README.md
- 6: https://aur.archlinux.org/packages/python-py-rust-stemmers
- 7: https://www.nltk.org/api/nltk.stem.snowball.html
- 8: https://pypi.org/project/PyStemmer/
Make custom BM25 stemmers safe for parallel execution.
When parallel is enabled, Bm25Worker.start(...) re-constructs Bm25 from the constructor arguments, so stemmer is passed through multiprocessing process arguments. Local stemmers such as the test’s class _FakePolishStemmer are not pickleable and will fail parallel calls; document that stemmer must be pickleable/importable and add a parallel test, or replace it with an importable factory/configuration payload.
🤖 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 `@fastembed/sparse/bm25.py` around lines 222 - 223, Make the custom stemmer
path safe for parallel execution: document on Bm25’s stemmer configuration that
values passed through Bm25Worker.start must be pickleable and importable, and
add a parallel test using a module-level stemmer. Alternatively, replace direct
stemmer instances with an importable factory or configuration payload that Bm25
can reconstruct in worker processes.
1 participant
Closes #654, requested by @pjastrzebskiwelyo.
Adds a
stemmerparameter toBm25: any object with astem_word(word: str) -> strmethod (a smallStemmerprotocol, satisfied bypy_rust_stemmers.SnowballStemmeramong others). When a custom stemmer is provided, the language check is relaxed, which enables languages without a Snowball algorithm such as Polish. An optionalstopwordsparameter lets callers pass inline stopwords instead of the file shipped with the model, which those languages need too.No new dependencies, and defaults are unchanged: without the new parameters, behavior is byte-for-byte the same (there is a test pinning the default Snowball path).
New tests cover a custom stemmer being used, an unsupported language working with one, inline stopwords overriding the file-based set, and the default path staying unchanged. They fail without the change; the sparse embedding suite passes with it.