-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Text_similarity_reranker retriever rework to be evaluated during rewrite phase #114085
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
Text_similarity_reranker retriever rework to be evaluated during rewrite phase #114085
Conversation
@elasticmachine update branch |
…/elasticsearch into text_similarity_reranker_rework
...rg/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankRetrieverBuilder.java
Show resolved
Hide resolved
- length: { hits.hits: 2 } | ||
|
||
- match: { hits.hits.0._id: "doc_2" } | ||
- match: { hits.hits.0._rank: 1 } |
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.
_rank
has now been removed as with rrf
.
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
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.
TextSimilarityRankDoc.java
needs randomized serialization tests and we need to ensure whatever utilizes this named object (looks like RankDocsQueryBuilder
and RankDocsRetrieverBuilder
) exercise using this new named object some how.
We don't want to miss adding this new named object to the named serialization objects and such.
server/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankDoc.java
Show resolved
Hide resolved
…ion; converting rankdoc to versionednamedwriteable; and adding serialization tests for textsimilarityrankdoc
@elasticmachine update branch |
@elasticmachine update branch |
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.
I think we are getting close, I am wondering about some of the build.gradle changes & if they are actually necessary.
...rg/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankRetrieverBuilder.java
Show resolved
Hide resolved
testImplementation(testArtifact(project(':server'))) | ||
|
||
clusterModules project(xpackModule('rank-rrf')) | ||
clusterModules project(xpackModule('inference')) |
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.
Why is rrf
requiring inference
?
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.
I've added this to actually exercise different semantic_reranking and rrf combinations (in 800_rrf_with_text_similarity_reranker_retriever.yml
) . AFAICT clusterModules
is a test-only dependency, so there shouldn't be any changes in runtime dependencies.
However, I wasn't certain myself on what place would be the best for hosting such combined tests (needing code from both inference
and rank-rrf
) so I'm really open to any alternatives :)
run elasticsearch-ci/part-1 |
💔 Backport failed
You can use sqren/backport to manually backport by running |
Now that #112648 has been merged, in this PR we handle the
text_similarity_reranker
retriever next, so that it can be evaluated during the rewrite phase and be transformed to a flattenedRankDocsRetrieverBuilder
. This will enable us to support composition and many search features, including highlighting, aggregations, and collapsing.