-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(reranker): support omitting top_n #7199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Mikhail Khludnev <[email protected]>
✅ Deploy Preview for localai ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Signed-off-by: Mikhail Khludnev <[email protected]>
Signed-off-by: Mikhail Khludnev <[email protected]>
| request = backend_pb2.RerankRequest( | ||
| query="I love you", | ||
| documents=["I hate you", "I really like you"], | ||
| top_n=0 # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise we can better handle top_n REST API param
notice https://api.jina.ai/redoc#tag/rerank/operation/rank_v1_rerank_post
defaults to the length of documents
Yes, that would make def. sense! |
mudler
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up!
Description
This PR fixes the issue introduced in #7025
Notes for Reviewers
#7025 introduced handling result cropping by
top_n. However, I believe the most users just omittop_n, just because don't bother to countlen(documents). So, I'm afraid releasing #7025 causes a trouble for many users. @mudler, beg your pardon.This PR let users to omit
top_nor sendtop_n=0meaning all docs.One thing about tests, since tey bring up the backend every time, isn't it worth to loop three alt requests in the single test method?
Signed commits