Allow kwargs in body_func #46
Conversation
| @@ -112,3 +129,18 @@ def _multi_field_mapper(self, hit: Mapping[str, Any]) -> Document: | |||
| field = self.content_field[hit["_index"]] | |||
| content = hit["_source"].pop(field) | |||
| return Document(page_content=content, metadata=hit) | |||
|
|
|||
| async def _aget_relevant_documents( | |||
There was a problem hiding this comment.
Is this method necessary? We do not currently have support for async in any other part of this library, and we would most likely not use an executor when we add it, since the Elasticsearch client does support async natively.
There was a problem hiding this comment.
I didn't realize that, sorry!
Anyway, these ainvoke calls work even while the underlying implementation being fully synchronous.
Leaving the retriever without this _aget_relevant_documents override may confuse users by allowing them to call the retriever in an async fashion (even when it is just running sychronously under the hood) and obtaining either a different result than they expect, if the body_func supports the omission of its keyword arguments, or a hard-to-debug error for the absence of said arguments.
Does that make sense to you?
There was a problem hiding this comment.
Yes, I do realize that this works, but it is not a great solution. When we add async support to this library we are going to do it properly. Adding this hack seems out of place, considering that no other function in the library does it. Of course you can subclass our implementation and add async methods in a subclass if this soluiton works for you.
There was a problem hiding this comment.
That makes sense! I will just remove the async bit, then. Thanks!
Closes #41
This PR enables the passing of auxiliary arguments when forming the
bodyin a elastic search retrieval.It seems to me that we need to override the
_aget_relevant_documentsbecauselangchain.coredoes not passkwargsto_aget_relevant_documentswhen it is called viaainvoke. It may be the case to open a PR inlangchaininstead, but for now I will leave it here.I will add tests and try to fix the
mypyerrors if the approach is validated by the maintainers.