feat: add reranker support to Python SDK#119
Conversation
|
|
||
|
|
||
| async def setup_index(client): | ||
| """Delete old index, create fresh one with all FAQ data.""" |
There was a problem hiding this comment.
Is the delete old index step missing ?
There was a problem hiding this comment.
darn, I think I removed it for a test and then forgot to add, let me do it real quick
|
|
||
|
|
||
| async def main(): | ||
| client = MossClient(os.getenv("MOSS_PROJECT_ID"), os.getenv("MOSS_PROJECT_KEY")) |
There was a problem hiding this comment.
can you please create .env.example
| top_n=5, # return top 5 after reranking | ||
| api_key=os.getenv("COHERE_API_KEY"), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
would reranking definition return type searchresult ?
There was a problem hiding this comment.
yes it would be of this type
| QueryOptions(filter={"$and": [ | ||
| {"field": "city", "condition": {"$eq": "NYC"}}, | ||
| {"field": "price", "condition": {"$lt": "50"}}, | ||
| {"field ": "price", "condition": {"$lt": "50"}}, |
There was a problem hiding this comment.
Please keep the changes minimal, the space here is not necessory.
| model: Optional[str] = None | ||
| top_n: Optional[int] = None | ||
| api_key: Optional[str] = None | ||
| options: Dict[str, Any] = field(default_factory=dict) |
There was a problem hiding this comment.
can you please use **kwargs, adding options will make the args complex
There was a problem hiding this comment.
so I can't use kwargs with a dataclass by default, I have two options
- use a regular class
- use dataclass(init=false) and write init separately
which one do you think should be done here?
|
|
||
| if is_loaded: | ||
| return await self._query_local(name, query, options) | ||
| result = await self._query_local(name, query, options) |
There was a problem hiding this comment.
reranking should be optional not necessory path
There was a problem hiding this comment.
I am not sure if I get it correctly but query function already has rerank as optional and defaults to none
async def query(
self,
name: str,
query: str,
options: Optional[QueryOptions] = None,
*,
rerank: Optional[RerankOptions] = None,
| model: str = "rerank-v3.5", | ||
| **kwargs: Any, | ||
| ): | ||
| """Initialize the Cohere reranker. |
There was a problem hiding this comment.
can we please use sdk instead of http api
Code Review SummaryOverall this is a solid feature addition — the architecture (Protocol-based Unresolved review comments from @yatharthk2Several items the author acknowledged but haven't been addressed in the current diff:
New issues
|
| from __future__ import annotations | ||
| import os | ||
| from typing import Any, List, Optional | ||
| import httpx |
There was a problem hiding this comment.
This still uses raw httpx — the switch to the Cohere SDK hasn't been made yet.
| QueryOptions(top_k=20, alpha=0.8), # fetch 10 candidates | ||
| rerank=RerankOptions( | ||
| provider="cohere", | ||
| model="rerank-v3.5", |
There was a problem hiding this comment.
Nit: comment says # fetch 10 candidates but top_k=20. Should be # fetch 20 candidates.
|
can you please resolve the comments |
85a603d to
4309523
Compare
fa63a1b to
8d4cec3
Compare
f24c01e to
56b42b3
Compare
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
why is this file required ?
There was a problem hiding this comment.
I guess I added it because of some CI tests were failing
|
|
||
| from dotenv import load_dotenv | ||
| from moss import MossClient, DocumentInfo, QueryOptions, RerankOptions | ||
| from moss.rerankers import CohereReranker |
There was a problem hiding this comment.
this import is not necessary, user should only be able to pass Cohere string as options and sdk should internally take care of the reranker object from cohere
There was a problem hiding this comment.
cool thing
let me add the registry again.
820238f to
e9a0002
Compare
| print("\nWith Cohere Reranking") | ||
| results = await client.query( | ||
| INDEX_NAME, | ||
| "How to get discount?", | ||
| QueryOptions(top_k=20, alpha=0.8), | ||
| rerank=RerankOptions( | ||
| provider="cohere", | ||
| api_key=os.getenv("COHERE_API_KEY"), | ||
| top_n=5, |
There was a problem hiding this comment.
reranking needs to be part of the query options. Please refer to the #84
There was a problem hiding this comment.
I have to modify the binding file to do this cause as of now sdks/python/bindings/src/models.rs this only supports
#[pyclass(name = "QueryOptions")]
pub struct PyQueryOptions {
pub embedding: Option<Vec<f32>>,
pub top_k: Option<usize>,
pub alpha: Option<f32>,
pub filter: Option<Py<PyAny>>,
}
There was a problem hiding this comment.
no need to go to bindings, you can do something roughly like below in the file moss_client.py :
async def _query_local(
self,
name: str,
query: str,
options: Optional[QueryOptions],
) -> SearchResult:
top_k = getattr(options, "top_k", None) or 5
alpha = getattr(options, "alpha", None) or 0.8
query_embedding = getattr(options, "embedding", None)
filter = getattr(options, "filter", None)
rerank = getattr(options, "rerank", None)
fetch_k = top_k * 4 if rerank else top_k
if query_embedding is None:
try:
result = await asyncio.to_thread(
self._manager.query_text, name, query, fetch_k, alpha, filter,
)
except RuntimeError as e:
if "requires explicit query embeddings" in str(e):
raise ValueError(
"This index uses custom embeddings. "
"Query embeddings must be provided via QueryOptions.embedding."
) from e
raise
else:
result = await asyncio.to_thread(
self._manager.query, name, query, list(query_embedding), fetch_k, alpha, filter,
)
if rerank:
from inferedge_moss.services.reranker import get_reranker
api_key = rerank.api_key or self._rerank_api_key
reranker = get_reranker(rerank.provider, api_key)
final_n = rerank.top_n or top_k
result.docs = reranker.rerank(query, result.docs, rerank.model, final_n)
return result
| print("\nWith Cohere Reranking") | ||
| results = await client.query( | ||
| INDEX_NAME, | ||
| "How to get discount?", | ||
| QueryOptions(top_k=20, alpha=0.8), | ||
| rerank=RerankOptions( | ||
| provider="cohere", | ||
| api_key=os.getenv("COHERE_API_KEY"), | ||
| top_n=5, |
There was a problem hiding this comment.
no need to go to bindings, you can do something roughly like below in the file moss_client.py :
async def _query_local(
self,
name: str,
query: str,
options: Optional[QueryOptions],
) -> SearchResult:
top_k = getattr(options, "top_k", None) or 5
alpha = getattr(options, "alpha", None) or 0.8
query_embedding = getattr(options, "embedding", None)
filter = getattr(options, "filter", None)
rerank = getattr(options, "rerank", None)
fetch_k = top_k * 4 if rerank else top_k
if query_embedding is None:
try:
result = await asyncio.to_thread(
self._manager.query_text, name, query, fetch_k, alpha, filter,
)
except RuntimeError as e:
if "requires explicit query embeddings" in str(e):
raise ValueError(
"This index uses custom embeddings. "
"Query embeddings must be provided via QueryOptions.embedding."
) from e
raise
else:
result = await asyncio.to_thread(
self._manager.query, name, query, list(query_embedding), fetch_k, alpha, filter,
)
if rerank:
from inferedge_moss.services.reranker import get_reranker
api_key = rerank.api_key or self._rerank_api_key
reranker = get_reranker(rerank.provider, api_key)
final_n = rerank.top_n or top_k
result.docs = reranker.rerank(query, result.docs, rerank.model, final_n)
return result
|
this is still being worked upon ? |
Yes will push a PR today sorry for the delay |
e9a0002 to
8f40c0c
Compare
|
@staru09 is attempting to deploy a commit to the Moss Team Team on Vercel. A member of the Team first needs to authorize it. |
8f40c0c to
ba073b2
Compare
| SearchResult, | ||
| ) | ||
|
|
||
| from .client.models import QueryOptions, RerankOptions |
There was a problem hiding this comment.
📝 Info: QueryOptions replaced from Rust class to Python dataclass — backward compatibility
The PR replaces QueryOptions imported from moss_core (a Rust-backed class) with a new Python @dataclass in models.py:39-54. This is a significant type change. However, it is safe because MossClient never passes the QueryOptions object directly to Rust functions — it always extracts individual fields via getattr() (e.g., moss_client.py:230-238). All existing callers (test_hot_reload.py, test_cloud_fallback.py, test_search.py, examples, packages) use from moss import QueryOptions and construct it with keyword arguments like QueryOptions(top_k=5), which works identically with the new dataclass since it's a superset of the old fields.
Was this helpful? React with 👍 or 👎 to provide feedback.
ba073b2 to
7836beb
Compare
| rerank = getattr(options, "rerank", None) | ||
| if rerank: | ||
| top_k = getattr(options, "top_k", None) | ||
| result = await self._apply_rerank(query, result, rerank, top_k) |
There was a problem hiding this comment.
🔴 Reranking over-fetch multiplier leaks into final result count when top_k/top_n not explicitly set
When reranking is enabled but the user doesn't explicitly set top_k or top_n, the 4x internal over-fetch multiplier leaks into the final result count. At moss_client.py:217, top_k = getattr(options, "top_k", None) reads the raw user value (None when unset) and passes it to _apply_rerank as default_top_k. Meanwhile, _query_local (line 230-232) and _query_cloud (line 300-302) both independently resolve None → 5 and then compute fetch_k = 5 * 4 = 20. In _apply_rerank at line 282, final_n = rerank_opts.top_n or default_top_k evaluates to None or None = None, so the reranker receives top_k=None and returns all 20 over-fetched documents. The user gets 20 results instead of the expected default of 5.
Example triggering the bug
result = await client.query("idx", "search",
QueryOptions(rerank=RerankOptions(provider="cohere", api_key="..."))
)
len(result.docs) # Returns 20 instead of 5| rerank = getattr(options, "rerank", None) | |
| if rerank: | |
| top_k = getattr(options, "top_k", None) | |
| result = await self._apply_rerank(query, result, rerank, top_k) | |
| rerank = getattr(options, "rerank", None) | |
| if rerank: | |
| top_k = getattr(options, "top_k", None) | |
| if top_k is None: | |
| top_k = 5 | |
| result = await self._apply_rerank(query, result, rerank, top_k) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| top_k = getattr(options, "top_k", None) | ||
| if top_k is None: | ||
| top_k = 5 |
There was a problem hiding this comment.
🚩 Cloud fallback default top_k changed from 10 to 5
The old _query_cloud used top_k = getattr(options, "top_k", None) or 10, defaulting to 10 results. The new code at moss_client.py:300-302 uses if top_k is None: top_k = 5, changing the default to 5. This aligns with the local path's default of 5 (moss_client.py:231-232), making behavior consistent across both paths. This is likely intentional but is a behavioral change for users who relied on the implicit cloud default of 10 results. Additionally, the old or 10 pattern would treat top_k=0 as falsy (falling back to 10), while the new is None check correctly preserves explicit top_k=0.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| from .cohere import CohereReranker | ||
|
|
||
| register_reranker("cohere", CohereReranker) | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
🚩 Cohere reranker silently swallowed when package not installed
In rerankers/__init__.py:41-46, the CohereReranker import is wrapped in try/except ImportError: pass. If a user specifies provider="cohere" but doesn't have the cohere package installed, the registration silently skips, and they'll get a ValueError: Unknown reranker provider: 'cohere' at query time rather than a clear missing-dependency message. The error message at line 31-35 lists available providers but doesn't hint that the provider might exist but require an optional dependency. This could confuse users since the SDK documentation suggests cohere is a built-in provider.
Was this helpful? React with 👍 or 👎 to provide feedback.
7836beb to
fb2201b
Compare
Pull Request Checklist
Please ensure that your PR meets the following requirements:
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
Fixes #84
Type of Change