feat: Implement and enhance remote document retrieval functionality #5487
Conversation
Signed-off-by: jyejare <jyejare@redhat.com>
Signed-off-by: jyejare <jyejare@redhat.com>
Signed-off-by: jyejare <jyejare@redhat.com>
Signed-off-by: jyejare <jyejare@redhat.com>
Signed-off-by: jyejare <jyejare@redhat.com>
jyejare
changed the title
jyejare
changed the title
jyejare
force-pushed
the
remote_retrieve_docs
branch
from
7d92b04 to
48f27f7
Compare
Signed-off-by: jyejare <jyejare@redhat.com>
jyejare
force-pushed
the
remote_retrieve_docs
branch
from
48f27f7 to
eb77f31
Compare
| query_embedding: Optional[List[float]] = None | ||
|
|
||
|
|
||
| class GetOnlineDocumentsRequest(BaseModel): |
There was a problem hiding this comment.
Since we are not passing entities dataframe, yes, everything is optional.
We can make features as non optional though.
| top_k: int, | ||
| distance_metric: Optional[str] = None, | ||
| query_string: Optional[str] = None, | ||
| api_version: Optional[int] = 1, |
There was a problem hiding this comment.
I havent tested version 2 in depth for all online stores. BTW Do we have v2 implemented for all stores and confirm stability ?
If not, I think we can go with v1 as stable with opt-in to v2.
| response = await run_in_threadpool( | ||
| lambda: store.retrieve_online_documents_v2(**read_params) # type: ignore | ||
| ) | ||
| if request.api_version == 2: |
There was a problem hiding this comment.
this shouldn't be necessary. v2 vs v1 should be approximately the same where v2 was meant to incorporate more fields. If there is a difference, it's a bug.
There was a problem hiding this comment.
we should remove v1 and move to v2 for all of the existing usage.
There was a problem hiding this comment.
@franciscojavierarceo Yes, I have seen atleast with postgres online store that v1 and v2 are retrieving different dataframes as the logic is completely different for both versions.
I agree for keeping one solid logic across all stores but only after identifying which is the best version AND THEN we should adapt the best logic having more fields. Finally I think we can remove the bad one and keep single retrieval version.
But I think the investigatin and removal from all stores should not be part of this PR, we should handle that in spearate PR.
PR includes: