-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Optionally allow text similarity reranking to fail #121784
Conversation
Hi @thecoop, I've created a changelog YAML for you. |
@@ -181,6 +182,11 @@ private void onPhaseDone( | |||
RankFeaturePhaseRankCoordinatorContext rankFeaturePhaseRankCoordinatorContext, | |||
SearchPhaseController.ReducedQueryPhase reducedQueryPhase | |||
) { | |||
RankFeatureDoc[] docs = rankPhaseResults.getSuccessfulResults() |
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.
This changes the thread this happens in - not sure if this matters or not?
20c5735
to
1ac18fe
Compare
...rg/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankRetrieverBuilder.java
Outdated
Show resolved
Hide resolved
65aac03
to
505ae8b
Compare
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
f43701f
to
bc81bd0
Compare
...test/java/org/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankTests.java
Outdated
Show resolved
Hide resolved
Hi @thecoop, I've updated the changelog YAML for you. |
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 @thecoop , really nice :) Left some minor comments, with the error handling/logging pending, as well as how to propagate to the user that the results are not reranked (prob something in CompoundRetrieverBuilder
? )
@@ -51,35 +43,26 @@ public class TextSimilarityRankBuilder extends RankBuilder { | |||
License.OperationMode.ENTERPRISE | |||
); | |||
|
|||
static final ConstructingObjectParser<TextSimilarityRankBuilder, Void> PARSER = new ConstructingObjectParser<>(NAME, args -> { |
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.
+1 on the cleanup :)
...test/java/org/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankTests.java
Outdated
Show resolved
Hide resolved
.../org/elasticsearch/xpack/inference/rank/textsimilarity/TextSimilarityRankMultiNodeTests.java
Outdated
Show resolved
Hide resolved
...pack/inference/rank/textsimilarity/TextSimilarityRankFeaturePhaseRankCoordinatorContext.java
Outdated
Show resolved
Hide resolved
.../main/java/org/elasticsearch/search/rank/context/RankFeaturePhaseRankCoordinatorContext.java
Show resolved
Hide resolved
context.onPhaseFailure(NAME, "Computing updated ranks for results failed", e); | ||
if (rankFeaturePhaseRankCoordinatorContext.failuresAllowed()) { | ||
// TODO: handle the exception somewhere | ||
// don't want to log the entire stack trace, it's not helpful here |
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.
++
...pack/inference/rank/textsimilarity/TextSimilarityRankFeaturePhaseRankCoordinatorContext.java
Show resolved
Hide resolved
339acc2
to
7b81006
Compare
...estTest/resources/rest-api-spec/test/rrf/800_rrf_with_text_similarity_reranker_retriever.yml
Outdated
Show resolved
Hide resolved
Thanks for all the work and the iterations so far @thecoop ! Will documentation and propagating the non-reranked flag to the user be part of a second PR or will you append them on this one? |
Yes, they'll be in a second PR to add the user-visible sections |
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 @thecoop ! Really nice ❤️ Left a couple of minor comments but no need for another review round :)
First part of #116796
Add a
allow_reranker_failures
option to reranker configuration, allowing errors to be ignored and the search to carry on. The option will be documented as part of the second stage PR