-
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
Added Multimodal semantic search feature #359
Added Multimodal semantic search feature #359
Conversation
8811b48
to
2192bb5
Compare
Codecov Report
@@ Coverage Diff @@
## 2.x #359 +/- ##
============================================
+ Coverage 80.51% 80.69% +0.18%
- Complexity 447 496 +49
============================================
Files 39 41 +2
Lines 1411 1554 +143
Branches 210 237 +27
============================================
+ Hits 1136 1254 +118
- Misses 187 200 +13
- Partials 88 100 +12
|
791d792
to
88b65d9
Compare
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
...n/java/org/opensearch/neuralsearch/processor/factory/TextImageEmbeddingProcessorFactory.java
Outdated
Show resolved
Hide resolved
...n/java/org/opensearch/neuralsearch/processor/factory/TextImageEmbeddingProcessorFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/neuralsearch/processor/TextImageEmbeddingProcessor.java
Outdated
Show resolved
Hide resolved
@martin-gaievski Haven't reviewed the code completely. Will try to do it by tonight |
src/test/java/org/opensearch/neuralsearch/common/BaseNeuralSearchIT.java
Outdated
Show resolved
Hide resolved
@martin-gaievski overall code looks good to me. Lets make sure that we add enough tests to make the github action pass. This file seems missing UTs. TextImageEmbeddingProcessor |
Sure, let me work backwards from the unit test coverage report |
src/main/java/org/opensearch/neuralsearch/ml/MLCommonsClientAccessor.java
Outdated
Show resolved
Hide resolved
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
src/test/java/org/opensearch/neuralsearch/processor/NormalizationProcessorIT.java
Show resolved
Hide resolved
768a024
to
b3bf6b4
Compare
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
…ferences Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
b3bf6b4
to
21f356b
Compare
Signed-off-by: Martin Gaievski <[email protected]>
Signed-off-by: Martin Gaievski <[email protected]>
import com.google.common.collect.ImmutableList; | ||
|
||
/** | ||
* Testing text_and_image_embedding ingest processor. We can only test text in integ tests, none of pre-built models |
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.
How are we confirming that this works with images if we dont have model? Has offline testing been done?
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.
We have tested that offline by setting up one of BedRock titan models and registering it using remote connector feature in ml-commons.
Signed-off-by: Martin Gaievski <[email protected]>
* Adding inference processor and factory, register that in plugin class Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 841f280)
* Adding inference processor and factory, register that in plugin class Signed-off-by: Martin Gaievski <[email protected]> (cherry picked from commit 841f280)
* Added Multimodal semantic search feature (#359) Signed-off-by: Martin Gaievski <[email protected]>
Description
Adding support for multimodal search.
This PR covers phase 1 of the implementation that is a simplified version that supports only models with text and image (text, image or both).
Main parts involved:
That is done in 2.x to be in sync with ml-commons that are also doings changes, and to be align better with 2.11 release that is coming soon. It will be upported to
main
later.Issues Resolved
#318
#362
Check List
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.