-
Notifications
You must be signed in to change notification settings - Fork 85
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
Implement Optimized embedding generation in text and image embedding processor #1249
Implement Optimized embedding generation in text and image embedding processor #1249
Conversation
5e8c376
to
b9b21d0
Compare
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
for (Map.Entry<String, String> entry : knnMap.entrySet()) { | ||
String key = entry.getKey(); | ||
String value = entry.getValue(); | ||
if (existingDocument.containsKey(key) == false || existingDocument.get(key).equals(value) == 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.
I think we need to compare both the text and image here and this can be done by just checking one key?
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.
knnMap contains two keys: one for text and one for value. For each entry, it will be compared with text and image values of the existing document
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.
Synced offline that this code will work because currently we only allow user to define one image and one text field. So the knnMap only contains the text and image fields and both of them should be the same to reuse the existing embedding. We should add a comment to call it out.
Besides also thinking we may want to allow users to define multiple text and image fields in the processor. Probably we can create a RFC to see if there is a user need.
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.
Besides also thinking we may want to allow users to define multiple text and image fields in the processor. Probably we can create a RFC to see if there is a user need.
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Show resolved
Hide resolved
…processor Signed-off-by: will-hwang <[email protected]>
b9b21d0
to
109954a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
============================================
- Coverage 82.23% 0 -82.24%
============================================
Files 106 0 -106
Lines 5078 0 -5078
Branches 864 0 -864
============================================
- Hits 4176 0 -4176
+ Misses 569 0 -569
+ Partials 333 0 -333 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR implements embedding optimization for text/image embedding processor.
Notable differences from previous optimization in text embedding/sparse encoding:
batch_size
option for_bulk
update, and will process document one by one for batch operationRelated PRs:
Text Embedding Processor Optimization PR: link
Sparse Encoding Processor Optimization PR: link
Related RFC: #1138
Benchmark Results
Inference Model: amazon.titan-embed-image-v1
Dataset: flickr image dataset (https://www.kaggle.com/datasets/hsankesara/flickr-image-dataset?resource=download)
Pipeline:
Index:
Ingest Latency
The following table presents latency measurements of initial ingest operation (in milliseconds) with skip_existing feature enabled and disabled. The Percent difference columns show the relative performance impact between the two.
Update Latency
The following table presents the latency measurements of update operation after identical ingest operation (in milliseconds) with skip_existing feature enabled and disabled. The Percent difference columns show the relative performance impact between the two.
Related Issues
Resolves #1138
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.