Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add index for storing template models #34

Merged
merged 21 commits into from
Jun 14, 2021
Merged

Add index for storing template models #34

merged 21 commits into from
Jun 14, 2021

Conversation

jmazanec15
Copy link
Member

@jmazanec15 jmazanec15 commented Jun 1, 2021 •
edited
Loading

Description

This PR adds a hidden model index that will be used to stored serialized template indices used during index creation. Some of faiss's index types require a training step before indexing can begin. In order to support these index types, we need a way for a user to train a model index template and serialize it. This template will then be retrieved during segment creation to initialize the faiss index.

The mapping is fairly straightforward:

{
  "properties": {
    "engine": {
      "type": "keyword"
    },
    "model_blob": {
      "type": "binary"
    }
  }
}

The engine is required to identify which engine the model is intended to be used with. The model_blob stores the binary representation of the model.

1 model maps to 1 OpenSearch document in this index. models are identified by the OpenSearch id field. The index allows users to provide a custom id or allow OpenSearch to generate one for them. Documents cannot be updated in this index, however, they can be removed.

For implementing the index, I referred to:

  1. Anomaly Detection plugin
  2. Index Management plugin

Additionally, I added test cases for each operation.

Note -- changes related to jni can be ignored as they are reviewed in #28. The files to be reviewed are:

  1. ModelIndex.java
  2. KNNSettings.java
  3. KNNConstants.java
  4. KNNPlugin.java
  5. model-mapping.json
  6. ModelIndexTests.java

Issues Resolved

#27

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as per the DCO using --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.

jmazanec15 added 15 commits May 24, 2021 15:42
@jmazanec15
Initial commit to add faiss to jni
8845b9d 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
add faiss submodule
e6690a5 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Minor changes to get library working
d8c299a 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Minor style improvements
ddc331c 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Fix parameter null checks
498b47d 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Add util to get object from map or throw
276cc32 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Simplify faiss result parsing logic
a5d1a19 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Improve memory safety
92d70c3 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Remove parameters parameter from query index
9d1e958 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Add algorithm to fix build failure
4013600 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Minor cleanup to address comments
e5a1cb5 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
fix flaky test
2041807 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Free vector after call completes
9ffc046 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Refactor jni utils into interface
199d5f2 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Model index init commit
ee89aff 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15 jmazanec15 requested review from VijayanB and vamshin June 1, 2021 19:53
jmazanec15 added 2 commits June 1, 2021 12:57
@jmazanec15
Merge branch 'faiss-develop' of github.com:opensearch-project/k-NN in…
e0aa7fa 
…to model-index
@jmazanec15
Fix license
9bc647c 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Address comments
aa6bef6 
Signed-off-by: John Mazanec <jmazane@amazon.com>
@jmazanec15
Split put tests
e696cd6 
Signed-off-by: John Mazanec <jmazane@amazon.com>
vamshin
vamshin reviewed Jun 9, 2021
View reviewed changes
src/main/java/org/opensearch/knn/common/KNNConstants.java Outdated
public static final String MODEL_BLOB_PARAMETER = "model_blob";

public static final String MODEL_INDEX_MAPPING_PATH = "mappings/model-index.json";
public static final String MODEL_INDEX_NAME = ".knn-model-index";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the file name start with opensearch? May be confirm other plugins system indices naming convention with openSearch changes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Conventions, the only requirement is prefixed with ".".

  1. AD is still using "opendistro-"
  2. ISM also prefixes with ".opendistro".

That being said, I think ".opensearch-knn-model-index" is good. I will update.

src/main/java/org/opensearch/knn/index/KNNSettings.java
Setting.Property.NodeScope,
Setting.Property.Dynamic);

public static final Setting<Integer> MODEL_INDEX_NUMBER_OF_REPLICAS_SETTING = Setting.intSetting(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default should be 1 replica?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will update.

src/main/java/org/opensearch/knn/indices/ModelIndex.java
* @throws IOException thrown when get mapping fails
*/
public void create(ActionListener<CreateIndexResponse> actionListener) throws IOException {
if (isCreated()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log message here and return?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little worried this may blow up the logs if isCreated is not called by caller. Id prefer not to log here.

src/main/java/org/opensearch/knn/indices/ModelIndex.java Outdated

private String getMapping() throws IOException {
URL url = ModelIndex.class.getClassLoader().getResource(MODEL_INDEX_MAPPING_PATH);
assert url != null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asserts are sometimes disabled on production hosts. Can we do manual check and throw exception?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will update.

src/main/java/org/opensearch/knn/indices/ModelIndex.java Outdated
Object blob = getResponse.getSourceAsMap().get(KNNConstants.MODEL_BLOB_PARAMETER);

if (blob == null) {
throw new IllegalArgumentException("ModelID: \"" + modelId + "\" is not present in index");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this message be more user focused? Something like There is no model available with the provided Id?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can update.

@jmazanec15
Address comments
a143e87 
Signed-off-by: John Mazanec <jmazane@amazon.com>
vamshin
vamshin approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@vamshin vamshin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@jmazanec15 jmazanec15 merged commit 1e4e176 into opensearch-project:faiss-develop Jun 14, 2021
@jmazanec15 jmazanec15 mentioned this pull request Jun 29, 2021
Merged
5 tasks
jmazanec15 added a commit to jmazanec15/k-NN-1 that referenced this pull request Oct 22, 2021
@jmazanec15
Add index for storing template models (opensearch-project#34)
26e90eb 
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
jmazanec15 added a commit that referenced this pull request Oct 22, 2021
@jmazanec15
Add index for storing template models (#34)
dae7b23 
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
@jmazanec15 @martin-gaievski
Add index for storing template models (opensearch-project#34)
c400281 
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 7, 2022
@jmazanec15 @martin-gaievski
Add index for storing template models (opensearch-project#34)
59e53b7 
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
Signed-off-by: Martin Gaievski <gaievski@amazon.com>
martin-gaievski pushed a commit to martin-gaievski/k-NN that referenced this pull request Mar 30, 2022
@jmazanec15
Add index for storing template models (opensearch-project#34)
cca4af7 
Signed-off-by: Jack Mazanec <jmazane1@nd.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vamshin vamshin vamshin approved these changes

@VijayanB VijayanB VijayanB approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmazanec15 @VijayanB @vamshin