deprecate: completely replace fastembed bm25 with qdrant core bm25 in hosted qdrant#1166
Conversation
❌ Deploy Preview for poetic-froyo-8baba7 failed.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe BuiltinEmbedder class and its module were removed. Embedder gained a new constructor flag Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qdrant_client/embed/model_embedder.py`:
- Around line 438-441: The error message raised in the ValueError uses two
adjacent f-strings which concatenate without a space producing "supported
models.Check"; update the raise so the message is a single properly spaced
string—either merge into one f-string or add a leading/trailing space between
the two f-strings—at the ValueError that references model in model_embedder.py
(the raise ValueError(...) that mentions `{model}` and
`cloud_inference`/`fastembed`).
- Around line 55-59: The constructor is passing the wrong keyword to Embedder,
so change the call that creates self.embedder to pass
use_core_bm25=self._is_builtin_embedder_available (instead of use_bm25_core) so
Embedder.__init__ receives the correct flag; locate the Embedder(...)
instantiation in model_embedder.py where self._is_builtin_embedder_available is
used and replace the incorrect keyword so the Embedder class (in
qdrant_client/embed/embedder.py) correctly toggles _use_core_bm25 and allows
falling back to FastEmbedMisc when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 583e043d-213f-477d-9096-7bf4608569f8
📒 Files selected for processing (3)
qdrant_client/embed/builtin_embedder.pyqdrant_client/embed/embedder.pyqdrant_client/embed/model_embedder.py
💤 Files with no reviewable changes (1)
- qdrant_client/embed/builtin_embedder.py
| raise ValueError( | ||
| f"{model} is not found among supported models." | ||
| f"Check if `cloud_inference` is set to True or `fastembed` is installed (for local inference)?" | ||
| ) |
There was a problem hiding this comment.
Fix the concatenated exception text.
Lines 438-441 currently render supported models.Check because the two f-strings are joined without a space.
✏️ Proposed fix
raise ValueError(
- f"{model} is not found among supported models."
+ f"{model} is not found among supported models. "
f"Check if `cloud_inference` is set to True or `fastembed` is installed (for local inference)?"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ValueError( | |
| f"{model} is not found among supported models." | |
| f"Check if `cloud_inference` is set to True or `fastembed` is installed (for local inference)?" | |
| ) | |
| raise ValueError( | |
| f"{model} is not found among supported models. " | |
| f"Check if `cloud_inference` is set to True or `fastembed` is installed (for local inference)?" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 438-441: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qdrant_client/embed/model_embedder.py` around lines 438 - 441, The error
message raised in the ValueError uses two adjacent f-strings which concatenate
without a space producing "supported models.Check"; update the raise so the
message is a single properly spaced string—either merge into one f-string or add
a leading/trailing space between the two f-strings—at the ValueError that
references model in model_embedder.py (the raise ValueError(...) that mentions
`{model}` and `cloud_inference`/`fastembed`).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qdrant_client/embed/embedder.py (1)
243-246:⚠️ Potential issue | 🔴 CriticalCritical:
embed()bypasses the BM25 pathway because it usesFastEmbedMiscclass method directly instead of the instance method.The
embed()method checksFastEmbedMisc.is_supported_sparse_model(model_name)at line 243, but when_use_core_bm25is enabled and the model is"Qdrant/bm25", this model is not in FastEmbed's sparse model list. The code falls through to theelsebranch (line 256) and raises"Unsupported embedding model"without calling_embed_sparse_text().The instance method
is_supported_sparse_model()(lines 445-454) correctly handles the BM25 shortcut, butembed()bypasses it by calling the class method directly.Proposed fix - use the instance method for sparse model check
- elif FastEmbedMisc.is_supported_sparse_model(model_name): + elif self.is_supported_sparse_model(model_name): embeddings = self._embed_sparse_text( texts, model_name, options, is_query, batch_size )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/embed/embedder.py` around lines 243 - 246, The embed() method incorrectly calls the class method FastEmbedMisc.is_supported_sparse_model(model_name) which bypasses the instance-level BM25 shortcut; change the check to call the instance method (self.is_supported_sparse_model or the instance method defined at lines 445-454) so that when self._use_core_bm25 is enabled and model_name == "Qdrant/bm25" the code routes into self._embed_sparse_text(texts, model_name, options, is_query, batch_size) instead of falling through to the unsupported-model branch.
♻️ Duplicate comments (1)
qdrant_client/embed/model_embedder.py (1)
440-443:⚠️ Potential issue | 🟡 MinorMissing space in concatenated error message.
The two f-strings are joined without a space, producing
"supported models.Check". This was flagged in a prior review.✏️ Proposed fix
raise ValueError( - f"{model} is not found among supported models." + f"{model} is not found among supported models. " f"Check if `cloud_inference` is set to True or `fastembed` is installed (for local inference)?" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/embed/model_embedder.py` around lines 440 - 443, The ValueError message raised in model_embedder.py concatenates two f-strings without a separating space, causing "...supported models.Check..."; update the raise ValueError in the block that references model to produce a single properly spaced message (either merge into one f-string or add a leading/trailing space between the two parts) so the final error reads "...supported models. Check if `cloud_inference`..." and still includes the {model} interpolation and the guidance about cloud_inference/fastembed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@qdrant_client/embed/embedder.py`:
- Around line 243-246: The embed() method incorrectly calls the class method
FastEmbedMisc.is_supported_sparse_model(model_name) which bypasses the
instance-level BM25 shortcut; change the check to call the instance method
(self.is_supported_sparse_model or the instance method defined at lines 445-454)
so that when self._use_core_bm25 is enabled and model_name == "Qdrant/bm25" the
code routes into self._embed_sparse_text(texts, model_name, options, is_query,
batch_size) instead of falling through to the unsupported-model branch.
---
Duplicate comments:
In `@qdrant_client/embed/model_embedder.py`:
- Around line 440-443: The ValueError message raised in model_embedder.py
concatenates two f-strings without a separating space, causing "...supported
models.Check..."; update the raise ValueError in the block that references model
to produce a single properly spaced message (either merge into one f-string or
add a leading/trailing space between the two parts) so the final error reads
"...supported models. Check if `cloud_inference`..." and still includes the
{model} interpolation and the guidance about cloud_inference/fastembed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 867fd866-cd1d-4061-a5d1-81a5e196e749
📒 Files selected for processing (2)
qdrant_client/embed/embedder.pyqdrant_client/embed/model_embedder.py
joein
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qdrant_client/embed/embedder.py (1)
445-456:⚠️ Potential issue | 🟡 MinorFix redundant
.lower()comparison.The
.lower()call on the string literal is unnecessary. Change line 454:- if self._use_core_bm25 and model_name.lower() == "Qdrant/bm25".lower(): + if self._use_core_bm25 and model_name.lower() == "qdrant/bm25":Regarding the conversion from
@classmethodto instance method: while this is technically a breaking change for external callers, no usages ofEmbedder.is_supported_sparse_modelas a class method were found in the codebase. All current internal calls already use instance method syntax (self.is_supported_sparse_modelorembedder_instance.is_supported_sparse_model), so this change aligns with the actual usage patterns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/embed/embedder.py` around lines 445 - 456, The equality check in is_supported_sparse_model unnecessarily calls .lower() on the string literal; update the comparison in Embedder.is_supported_sparse_model to compare model_name.lower() to the lowercase literal "qdrant/bm25" (keep the existing _use_core_bm25 guard and the fallback to FastEmbedMisc.is_supported_sparse_model(model_name)), so the method reads: if self._use_core_bm25 and model_name.lower() == "qdrant/bm25": return True.
🧹 Nitpick comments (3)
tests/embed_tests/test_local_inference.py (2)
1736-1778: Missing collection cleanup for remote client.The test creates a collection on the remote client but doesn't delete it at the end. Other tests in this file consistently clean up collections. Consider adding cleanup to avoid leaving test state.
♻️ Proposed fix to add cleanup
if not local_client._FASTEMBED_INSTALLED: with pytest.raises(ImportError): local_client.upsert(COLLECTION_NAME, points) else: local_client.upsert(COLLECTION_NAME, points) + + remote_client.delete_collection(COLLECTION_NAME)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/embed_tests/test_local_inference.py` around lines 1736 - 1778, The test_bm25_core leaves the remote_client collection (COLLECTION_NAME) behind; wrap the remote_client collection creation/upsert in a try/finally (or add explicit cleanup at the end of the test) and call remote_client.delete_collection(COLLECTION_NAME) in the finally/cleanup block to ensure the remote collection is removed even if assertions fail; reference the existing remote_client and COLLECTION_NAME identifiers when adding the cleanup.
1775-1777: FragileImportErrorassertion lacks specificity.Using
pytest.raises(ImportError)without amatchparameter will pass for anyImportError, not just the expected fastembed import error. Based on the error message infastembed_common.py, consider adding a match pattern.♻️ Proposed fix to add match pattern
if not local_client._FASTEMBED_INSTALLED: - with pytest.raises(ImportError): + with pytest.raises(ImportError, match="fastembed is not installed"): local_client.upsert(COLLECTION_NAME, points)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/embed_tests/test_local_inference.py` around lines 1775 - 1777, The test currently asserts a generic ImportError when fastembed isn't installed; update the pytest.raises in tests/embed_tests/test_local_inference.py to assert the specific fastembed import error by adding a match pattern that matches the message raised in fastembed_common.py (referencing local_client._FASTEMBED_INSTALLED and the local_client.upsert call); change the pytest.raises(ImportError) to pytest.raises(ImportError, match="...") using the exact substring from fastembed_common.py so only the intended import failure passes.qdrant_client/embed/embedder.py (1)
300-305: Redundant.lower()call on string literal.The expression
"Qdrant/bm25".lower()evaluates to"qdrant/bm25"at runtime. Using the lowercase literal directly improves clarity and avoids unnecessary computation.♻️ Proposed fix
- if self._use_core_bm25 and model_name.lower() == "Qdrant/bm25".lower(): + if self._use_core_bm25 and model_name.lower() == "qdrant/bm25":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/embed/embedder.py` around lines 300 - 305, The comparison in the embedder method redundantly calls .lower() on the string literal; change the condition to compare model_name.lower() == "qdrant/bm25" (lowercase literal) so it reads if self._use_core_bm25 and model_name.lower() == "qdrant/bm25": and keep the existing return constructing models.Document for each text (referencing _use_core_bm25, model_name, and models.Document).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qdrant_client/embed/embedder.py`:
- Around line 30-32: Embedder.__init__ currently accepts an unused **kwargs
which should be removed; update the constructor signature in class Embedder to
drop **kwargs (leaving threads: int | None = None, use_core_bm25: bool = True)
and remove any references to that parameter in the body, and update any call
sites that were passing extra keyword args to instead pass model options
directly to get_or_init_model (or forward them explicitly where appropriate) so
no model options are silently discarded.
---
Outside diff comments:
In `@qdrant_client/embed/embedder.py`:
- Around line 445-456: The equality check in is_supported_sparse_model
unnecessarily calls .lower() on the string literal; update the comparison in
Embedder.is_supported_sparse_model to compare model_name.lower() to the
lowercase literal "qdrant/bm25" (keep the existing _use_core_bm25 guard and the
fallback to FastEmbedMisc.is_supported_sparse_model(model_name)), so the method
reads: if self._use_core_bm25 and model_name.lower() == "qdrant/bm25": return
True.
---
Nitpick comments:
In `@qdrant_client/embed/embedder.py`:
- Around line 300-305: The comparison in the embedder method redundantly calls
.lower() on the string literal; change the condition to compare
model_name.lower() == "qdrant/bm25" (lowercase literal) so it reads if
self._use_core_bm25 and model_name.lower() == "qdrant/bm25": and keep the
existing return constructing models.Document for each text (referencing
_use_core_bm25, model_name, and models.Document).
In `@tests/embed_tests/test_local_inference.py`:
- Around line 1736-1778: The test_bm25_core leaves the remote_client collection
(COLLECTION_NAME) behind; wrap the remote_client collection creation/upsert in a
try/finally (or add explicit cleanup at the end of the test) and call
remote_client.delete_collection(COLLECTION_NAME) in the finally/cleanup block to
ensure the remote collection is removed even if assertions fail; reference the
existing remote_client and COLLECTION_NAME identifiers when adding the cleanup.
- Around line 1775-1777: The test currently asserts a generic ImportError when
fastembed isn't installed; update the pytest.raises in
tests/embed_tests/test_local_inference.py to assert the specific fastembed
import error by adding a match pattern that matches the message raised in
fastembed_common.py (referencing local_client._FASTEMBED_INSTALLED and the
local_client.upsert call); change the pytest.raises(ImportError) to
pytest.raises(ImportError, match="...") using the exact substring from
fastembed_common.py so only the intended import failure passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e5201d6-990f-40d2-aff1-ba1376a6f0a4
📒 Files selected for processing (3)
qdrant_client/embed/embedder.pyqdrant_client/fastembed_common.pytests/embed_tests/test_local_inference.py
✅ Files skipped from review due to trivial changes (1)
- qdrant_client/fastembed_common.py
| def __init__( | ||
| self, threads: int | None = None, use_core_bm25: bool = True, **kwargs: Any | ||
| ) -> None: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Embedder instantiation to see if kwargs are being passed
rg -n "Embedder\(" --type py -A 3 | head -50Repository: qdrant/qdrant-client
Length of output: 1326
🏁 Script executed:
cat -n qdrant_client/embed/embedder.py | sed -n '1,70p'Repository: qdrant/qdrant-client
Length of output: 2886
🏁 Script executed:
cat -n qdrant_client/embed/model_embedder.py | sed -n '45,75p'Repository: qdrant/qdrant-client
Length of output: 1605
🏁 Script executed:
#!/bin/bash
# Check for Embedder subclasses
rg -n "class.*\(.*Embedder.*\)" --type py
# Check all Embedder instantiations to see what kwargs might be passed
echo "=== Embedder instantiations ==="
rg -n "Embedder\(" --type py -B 2 -A 2Repository: qdrant/qdrant-client
Length of output: 1791
Remove unused **kwargs parameter from Embedder.__init__.
The **kwargs parameter is accepted but never used in the constructor body. While get_or_init_model() accepts its own **kwargs for model options, the ones passed to __init__ are silently discarded, creating unnecessary confusion for maintainers and callers.
♻️ Proposed fix
def __init__(
- self, threads: int | None = None, use_core_bm25: bool = True, **kwargs: Any
+ self, threads: int | None = None, use_core_bm25: bool = True
) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__( | |
| self, threads: int | None = None, use_core_bm25: bool = True, **kwargs: Any | |
| ) -> None: | |
| def __init__( | |
| self, threads: int | None = None, use_core_bm25: bool = True | |
| ) -> None: |
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 31-31: Unused method argument: kwargs
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qdrant_client/embed/embedder.py` around lines 30 - 32, Embedder.__init__
currently accepts an unused **kwargs which should be removed; update the
constructor signature in class Embedder to drop **kwargs (leaving threads: int |
None = None, use_core_bm25: bool = True) and remove any references to that
parameter in the body, and update any call sites that were passing extra keyword
args to instead pass model options directly to get_or_init_model (or forward
them explicitly where appropriate) so no model options are silently discarded.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration-tests.sh`:
- Line 71: The pip uninstall command "pip3 uninstall fastembed" is interactive
and will hang CI; make it non-interactive by adding the confirmation flag (e.g.,
change to "pip3 uninstall -y fastembed" or "pip3 uninstall --yes fastembed") so
the uninstall proceeds without prompting; update the command in
tests/integration-tests.sh where "pip3 uninstall fastembed" appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d865c8b3-909b-481d-80e9-acab9e708cfb
📒 Files selected for processing (2)
.github/workflows/integration-tests.ymltests/integration-tests.sh
💤 Files with no reviewable changes (1)
- .github/workflows/integration-tests.yml
… hosted qdrant (#1166) * deprecate: completely replace fastembed bm25 with qdrant core bm25 * fix: fix param name in embedder, accept lower case bm25 model name * tests: enable skipped tests, add bm25 core usage tests * fix: fix ci, run fastembed tests in integration tests * fix: fix pip uninstall prompt * tests: log message when running tests wo fastembed
2 participants
No description provided.