-
Notifications
You must be signed in to change notification settings - Fork 94
Add hybrid query and score/rank based normalization processor stats #1326
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
Add hybrid query and score/rank based normalization processor stats #1326
Conversation
966d125
to
6797f1d
Compare
@@ -70,6 +72,7 @@ <Result extends SearchPhaseResult> void hybridizeScores( | |||
Optional<FetchSearchResult> fetchSearchResult = getFetchSearchResults(searchPhaseResult); | |||
boolean explain = Objects.nonNull(searchPhaseContext.getRequest().source().explain()) | |||
&& searchPhaseContext.getRequest().source().explain(); | |||
EventStatsManager.increment(EventStatName.RRF_PROCESSOR_EXECUTIONS); |
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.
for rank based normalization you also need to increment counter for combination technique calls, currently we do support "rrf" https://docs.opensearch.org/docs/latest/search-plugins/search-pipelines/score-ranker-processor/
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.
Do we plan to add more combination techniques for rank based normalization in the future? We could also wait until then to add more granular breakdowns, otherwise the stats will just be duplicated right? I added anyways for now
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.
no in near future, my worry was more about consistent result format, because for RRF we have "rrf" as both normalization technique (hidden), and combination technique.
src/main/java/org/opensearch/neuralsearch/processor/NormalizationProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/stats/events/EventStatName.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/stats/info/InfoStatName.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/NormalizationProcessor.java
Outdated
Show resolved
Hide resolved
b9765c8
to
c6f1ace
Compare
c6f1ace
to
c7ea569
Compare
@@ -40,6 +49,24 @@ public class NormalizationProcessor extends AbstractScoreHybridizationProcessor | |||
private final ScoreCombinationTechnique combinationTechnique; | |||
private final NormalizationProcessorWorkflow normalizationWorkflow; | |||
|
|||
private final Map<String, Runnable> normTechniqueIncrementers = Map.of( |
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.
looks like we have switch/case
to increment for some processors, and runnable as map values for others. We should have one standard approach
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. I am more inclined towards using map.
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.
Sure, can refactor the others as maps.
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 map can be static, same for the second map combTechniqueIncrementers
for (QueryBuilder query : queries) { | ||
if (filter == null) { | ||
compoundQueryBuilder.add(query); | ||
} else { | ||
compoundQueryBuilder.add(query.filter(filter)); | ||
} | ||
|
||
// Check if children have inner hits for stats | ||
if (hasInnerHits == false) { |
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.
is this the right comparison?
if children have inner hits, shouldn't this be true?
|
||
// Check if children have inner hits for stats | ||
if (hasInnerHits == false) { | ||
Map<String, InnerHitContextBuilder> innerHits = new HashMap<>(); |
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.
shouldn't we emit stats for the inner hits? seems like we're checking for inner hits without doing anything to them. hasInnerHits
can be flipped between true/false in the for loop, but we're only emitting once after the loop finishes in line 295. Is this valid?
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.
The thought here is that we care about the stats at the level of the hybrid query. If at least one child query has inner hits then that means the hybrid query is an inner hits query. So we check each child, if at least one has inner hits, then we set hasInnerHits = true and we don't have to keep checking the rest.
@@ -40,6 +49,24 @@ public class NormalizationProcessor extends AbstractScoreHybridizationProcessor | |||
private final ScoreCombinationTechnique combinationTechnique; | |||
private final NormalizationProcessorWorkflow normalizationWorkflow; | |||
|
|||
private final Map<String, Runnable> normTechniqueIncrementers = Map.of( |
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. I am more inclined towards using map.
|
||
private void recordStats(ScoreCombinationTechnique combinationTechnique) { | ||
EventStatsManager.increment(EventStatName.RRF_PROCESSOR_EXECUTIONS); | ||
Optional.of(combTechniqueIncrementers.get(combinationTechnique.techniqueName())).ifPresent(Runnable::run); |
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.
RRF processor internally creates RRFNormalizationProcessor also. Check https://github.com/opensearch-project/neural-search/blob/main/src/main/java/org/opensearch/neuralsearch/processor/factory/RRFProcessorFactory.java#L51
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 added this since @martin-gaievski requested, but my opinion is if currently RRF processor only works works with RRF combination technique and RRF normalization technique, then functionally the stats will be identical, and I don't think there's a point to including a breakdown, it will just be duplicated. Combination is configurable with a single option, but normalization technique isn't a configurable in processor config: https://docs.opensearch.org/docs/latest/search-plugins/search-pipelines/score-ranker-processor/
If in the future there are more normalizaiton/score techniques added we can add this granularity then.
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.
Having RRF as processor and as technique gives more dimensions for reporting. For instance, with original version you can generate report with breakdown "by processor type". But report with breakdown on something like "by combination technique" will be harder because for RRF you raw data will have combination technique metric with empty 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.
I see your point, my concern is having duplicated information in the stats API makes it less readable and less performant. Especially as we add more features, we are concerned with possible response bloat, similar the issues core is facing with the size of _nodes/info
and _nodes/stats
responses causing slowdowns on large clusters. And adding stats is one-way door in the sense that it's difficult to justify removing them since it's a breaking change. Ideally we save granularity for when it is needed and don't add more stats unless necessary
From a report perspective, I'm thinking in terms of what kind of insight a breakdown would give you: If you are trying to determine which score combination techniques are seeing more usage, perhaps proportion of RRF to zscore or minmax isn't comparable since they're categorically different, e.g. used in different processors in different contexts. And if needed, the information is available implicitly from looking at RRF processor stats directly.
for (QueryBuilder query : queries) { | ||
if (filter == null) { | ||
compoundQueryBuilder.add(query); | ||
} else { | ||
compoundQueryBuilder.add(query.filter(filter)); | ||
} | ||
|
||
// Check if children have inner hits for stats | ||
if (hasInnerHits == false) { |
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.
Where are we changing this parameter 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.
It's set inside the loop, see my other comment for explanation. Basically my thought is we want to increment the stat once if at least one child query has inner hits, which means the hybrid query as a whole is an inner hits hybrid query.
@@ -40,6 +49,24 @@ public class NormalizationProcessor extends AbstractScoreHybridizationProcessor | |||
private final ScoreCombinationTechnique combinationTechnique; | |||
private final NormalizationProcessorWorkflow normalizationWorkflow; | |||
|
|||
private final Map<String, Runnable> normTechniqueIncrementers = Map.of( |
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 map can be static, same for the second map combTechniqueIncrementers
break; | ||
case RRFProcessor.TYPE: | ||
countRRFProcessorStats(stats, processorConfig); | ||
break; |
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.
please add default case and throw one of runtime exceptions in case we reach it
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.
Not sure if that makes sense here? Here we are iterating through all processors in the pipeline, which may contain processors from core or other plugins. If we encounter an MLInferenceRequestProcessor
for example, we would not want to record stats for it, and that would trigger the default case and throw an exception, breaking the API.
ebd6ef6
to
0eac607
Compare
c7f39e9
to
bfedad1
Compare
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.
Looks good to me, thanks!
LGTM |
Signed-off-by: Andy Qin <[email protected]> # Conflicts: # CHANGELOG.md # src/main/java/org/opensearch/neuralsearch/stats/events/EventStatName.java # Conflicts: # CHANGELOG.md # src/main/java/org/opensearch/neuralsearch/stats/events/EventStatName.java # src/main/java/org/opensearch/neuralsearch/stats/info/InfoStatName.java
Signed-off-by: Andy Qin <[email protected]> # Conflicts: # CHANGELOG.md
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
Signed-off-by: Andy Qin <[email protected]>
bfedad1
to
788edd7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1326 +/- ##
============================================
- Coverage 82.71% 0 -82.72%
============================================
Files 149 0 -149
Lines 7383 0 -7383
Branches 1192 0 -1192
============================================
- Hits 6107 0 -6107
+ Misses 821 0 -821
+ Partials 455 0 -455 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR to add initial stats for hybrid query, normalization processor, and RRF processor
Normalization processor event stats:
Normalization processor info stats:
RRF processor event stats:
RRF processor info stats:
Hybrid query event stats
Example response:
Related Issues
Resolves #1146
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.