-
Notifications
You must be signed in to change notification settings - Fork 72
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
Explainability in hybrid query #970
Explainability in hybrid query #970
Conversation
e8a374c
to
6761ac7
Compare
f590f46
to
a19de09
Compare
0f3813f
to
37cd3c1
Compare
Pushed commit that changes the format to a hierarchical structure, similar to other queries. This addresses Heemin's request Following is updated format, also put it to the PR description
|
I've added integ test for this scenario |
src/main/java/org/opensearch/neuralsearch/processor/explain/ExplanationDetails.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/explain/ExplanationUtils.java
Show resolved
Hide resolved
Signed-off-by: Martin Gaievski <[email protected]>
@martin-gaievski Can you help me understand what does field1 [0 to 500] and within top 12 mean in explanation response. Overall the code looks good me. One question, are we not suppose to show original scores prior to normalization in the search response? |
src/main/java/org/opensearch/neuralsearch/processor/ExplanationResponseProcessor.java
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/ExplanationResponseProcessor.java
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/NormalizationProcessorWorkflow.java
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/NormalizationProcessorWorkflow.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/combination/ScoreCombiner.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/explain/ExplanationUtils.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/neuralsearch/query/HybridQueryExplainIT.java
Show resolved
Hide resolved
Original scores are there. Please check the breakdown of explanation elements below, hope that will answer all questions
|
9ac81cd
to
d297e0f
Compare
Can you add one integ test with "multi_match" and "neural" subquery? Most of the customer use this combination and lately we have seen this query combination is creating issues. |
List<Pair<Float, String>> scoreDetails; | ||
|
||
public ExplanationDetails(List<Pair<Float, String>> scoreDetails) { | ||
this(-1, scoreDetails); |
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 hardcode this -1 value?
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 will match default docId in SearchHit https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/search/SearchHit.java#L170, we set it when we don't have access to real docId value
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.
Can you add this explanation at line 24? As by the look of it, it will very difficult to guess. Also please the code ref as well.
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.
yup, added
Signed-off-by: Martin Gaievski <[email protected]>
@ToString.Include | ||
public static final String TECHNIQUE_NAME = "geometric_mean"; | ||
public static final String PARAM_NAME_WEIGHTS = "weights"; |
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 we removed this?
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.
it is a duplicate, same constant is in ScoreCombinationUtil, we reference it from all technique classes https://github.com/martin-gaievski/neural-search/blob/poc/explain_for_hybrid_v2/src/main/java/org/opensearch/neuralsearch/processor/combination/GeometricMeanScoreCombinationTechnique.java#L14
d297e0f
to
9098781
Compare
I'm not sure if it's necessary in scope of explain, it's doesn't matter which query we pick, all we care about is the format of newly added sections. We can add it in later PR if needed, anyways code goes to feature branch |
The reason I asked for that is because it involes multiple scorer behind the scenes so it will good to test. You can add it in the actual PR. |
explain is one of those features that are implemented with two phase approach: first it retrieves data at the query phase and then compile final response in fetch phase. We need to enable explain in the query by collecting sub-query scores, and then scores will be set to SearchHits in the Fetch phase automatically. We do not call anything explicitly, just adding implementation for existing interface methods |
Got it, approving this PR now. Will review the final PR once again after the security clearence. |
Signed-off-by: Martin Gaievski <[email protected]>
I've added the test, just one change - I did multi-match + knn query to avoid model deployment |
a3fd3a2
into
opensearch-project:feature/explainability-in-hybrid-query
* Added Explainability support for hybrid query Signed-off-by: Martin Gaievski <[email protected]>
* Added Explainability support for hybrid query Signed-off-by: Martin Gaievski <[email protected]>
* Added Explainability support for hybrid query Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 393d49a)
* Added Explainability support for hybrid query Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 393d49a) Signed-off-by: Martin Gaievski <[email protected]>
* Added Explainability support for hybrid query (cherry picked from commit 393d49a) Signed-off-by: Martin Gaievski <[email protected]> Co-authored-by: Martin Gaievski <[email protected]>
Description
Adding support for
explain
flag in hybrid query, implementation follows design shared in the RFC #905.PR will be merged to the feature branch because some work regarding security still needs to be done.
Search pipeline need to have a new response processor in order to format the explanation payload. Example of search pipeline configuration:
Example of search request, no changes there just now we will support
explain
flag:example of response:
same query framed as
bool
will give following response when used withexplain
:we don't support the explain by doc id, as per design, but I checked if we're not breaking things, here is the response for such explain by id :
Related Issues
#658
Change request for documentation: opensearch-project/documentation-website#8645
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.