added top_k argument in the run function of ElasticSearcBM25Retriever #130
Conversation
|
|
| @@ -48,12 +48,12 @@ def from_dict(cls, data: Dict[str, Any]) -> "ElasticsearchBM25Retriever": | |||
| return default_from_dict(cls, data) | |||
|
|
|||
| @component.output_types(documents=List[Document]) | |||
| def run(self, query: str): | |||
| def run(self, query: str, top_k: int=None): | |||
There was a problem hiding this comment.
top_k should be typed as Optional
| docs = self._document_store._bm25_retrieval( | ||
| query=query, | ||
| filters=self._filters, | ||
| fuzziness=self._fuzziness, | ||
| top_k=self._top_k, | ||
| top_k=self._top_k if top_k == None else top_k, |
There was a problem hiding this comment.
you can simplify this statement with top_k = top_k or self.top_k
| @@ -64,17 +64,18 @@ def from_dict(cls, data: Dict[str, Any]) -> "ElasticsearchEmbeddingRetriever": | |||
| return default_from_dict(cls, data) | |||
|
|
|||
| @component.output_types(documents=List[Document]) | |||
| def run(self, query_embedding: List[float]): | |||
| def run(self, query_embedding: List[float], top_k:int = None): | |||
There was a problem hiding this comment.
Same as above, top_k is optional
| """ | ||
| Retrieve documents using a vector similarity metric. | ||
|
|
||
| :param query_embedding: Embedding of the query. | ||
| :param top_k: Maximum number of Documents to return |
There was a problem hiding this comment.
Thanks for fixing the docs! It's out of the scope of this PR, but would you mind adding a similar docstring to the run method of the other retriever component?
There was a problem hiding this comment.
The changes you suggested will be done soon.
Question: When you mentioned other retriever components you mean in the main haystack right?
There was a problem hiding this comment.
@sahusiddharth no I mean the bm25 retriever in this integration, see https://github.com/deepset-ai/haystack-core-integrations/blob/main/integrations/elasticsearch/src/elasticsearch_haystack/bm25_retriever.py#L51
masci
changed the title
…-ElasticSearshBM25Retriever
|
@masci All done |
The top_k can only be defined at initialization, It would allow users to change the top_k at the pipeline runtime too.