-
Notifications
You must be signed in to change notification settings - Fork 183
[Enhancements] Sparse encoding/tokenize support TOKEN_ID format embedding #3963
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
Conversation
@zhichao-aws The code looks OK to me. Can you provide some example calls and expected results so that other code reviewers can better understand the context? |
...rg/opensearch/ml/common/input/parameter/textembedding/AsymmetricTextEmbeddingParameters.java
Show resolved
Hide resolved
...rg/opensearch/ml/common/input/parameter/textembedding/AsymmetricTextEmbeddingParameters.java
Outdated
Show resolved
Hide resolved
.../main/java/org/opensearch/ml/engine/algorithms/sparse_encoding/SparseEncodingTranslator.java
Show resolved
Hide resolved
.../main/java/org/opensearch/ml/engine/algorithms/sparse_encoding/SparseEncodingTranslator.java
Outdated
Show resolved
Hide resolved
...orithms/src/main/java/org/opensearch/ml/engine/algorithms/tokenize/SparseTokenizerModel.java
Show resolved
Hide resolved
...rg/opensearch/ml/common/input/parameter/textembedding/AsymmetricTextEmbeddingParameters.java
Show resolved
Hide resolved
Here are some example calls/response: (using sparse tokenizer)
If parameters not set, it has the same behavior as before |
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/TextEmbeddingModel.java
Show resolved
Hide resolved
LGTM. |
reran flaky test
|
related to this issue #2560 @Hailong-am |
@mingshl This is a new issue. I have checked the logs from https://github.com/opensearch-project/ml-commons/actions/runs/16163806560/job/45694480886, the actual error is
checking the code of this, this happens when execute the agent, it will decrypt the credentials for connector. My first assumption is the master key has changed or not synced between different nodes, that will need further investigation. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3963 +/- ##
============================================
+ Coverage 80.64% 80.66% +0.01%
- Complexity 7976 7994 +18
============================================
Files 694 695 +1
Lines 34938 35002 +64
Branches 3899 3919 +20
============================================
+ Hits 28177 28233 +56
- Misses 5030 5035 +5
- Partials 1731 1734 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
See #3865 for more details
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.