-
Notifications
You must be signed in to change notification settings - Fork 129
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
Enable pass query string to model_config in ml inference search response processor #2899
base: main
Are you sure you want to change the base?
Enable pass query string to model_config in ml inference search response processor #2899
Conversation
Add more details/examples to description ? |
|
299aefa
to
350ab60
Compare
CI failed
|
Signed-off-by: Mingshi Liu <[email protected]>
350ab60
to
c1d112a
Compare
common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java
Outdated
Show resolved
Hide resolved
common/src/main/java/org/opensearch/ml/common/utils/StringUtils.java
Outdated
Show resolved
Hide resolved
I also left a comment in your RFC. |
Signed-off-by: Mingshi Liu <[email protected]>
bbbdae0
to
5319937
Compare
common/src/test/java/org/opensearch/ml/common/utils/StringUtilsTest.java
Show resolved
Hide resolved
Signed-off-by: Mingshi Liu <[email protected]>
flaky test
|
@@ -316,22 +321,42 @@ private void rewriteResponseDocuments(SearchResponse response, ActionListener<Se | |||
* @param inputMapIndex the index of the input mapping to process | |||
* @param batchPredictionListener the listener to be notified when the predictions are processed | |||
* @param hitCountInPredictions a map to keep track of the count of hits that have the required input fields for each round of prediction | |||
* @param queryString |
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.
minor: add some explanation to this parameter
String modelConfigValue = entry.getValue(); | ||
if (StringUtils.isValidJSONPath(modelConfigValue)) { | ||
Object queryJson = JsonPath.parse(queryString).read("$"); | ||
Configuration configuration = Configuration |
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 efficient to construct same Configuration in a for loop. Move this out of for loop or create a static variable ?
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.
great catch, will move the reading the query string to line 341
String modelConfigKey = entry.getKey(); | ||
String modelConfigValue = entry.getValue(); | ||
if (StringUtils.isValidJSONPath(modelConfigValue)) { | ||
Object queryJson = JsonPath.parse(queryString).read("$"); |
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 need to read "$" first ? , Can we just read the json path directly?
JsonPath.parse(queryString).read(modelConfigValue)
Description
Enable pass query string to model_config in ml inference search response processor
setting cluster
register cross-encoders local model
get register task status
GET /_plugins/_ml/tasks/tQ5p1ZEB4iWlnHsIf2Xw
deploy cross-encoders local model
`POST /_plugins/_ml/models/tg5p1ZEB4iWlnHsIh2U9/_deploy
`
get deploy task status
GET /_plugins/_ml/tasks/tw5q1ZEB4iWlnHsIo2WX
wait until completed
test model predict
upload index
create search pipeline with query text passing in model_config
search with search pipeline, scores are added in the response
ToDo
currently ml inference processor only support single tensor for local model, need to support multiple tensor parsing as well.
Related Issues
#2897
#2878
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.