Persist model definition in model metadata #1527

ryanbogan · 2024-03-12T20:40:05Z

Description

Adds MethodComponentContext to ModelMetadata. This allows developers and users to more easily access the model definition after the model has been created.

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.

Signed-off-by: Ryan Bogan <[email protected]>

codecov · 2024-03-12T21:00:18Z

Codecov Report

Attention: Patch coverage is 82.00000% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 84.86%. Comparing base (b7bdda4) to head (f3a5521).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...g/opensearch/knn/index/MethodComponentContext.java	80.00%	8 Missing and 11 partials ⚠️
...java/org/opensearch/knn/indices/ModelMetadata.java	82.97%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1527      +/-   ##
============================================
- Coverage     85.20%   84.86%   -0.34%     
- Complexity     1299     1327      +28     
============================================
  Files           171      171              
  Lines          5293     5432     +139     
  Branches        505      527      +22     
============================================
+ Hits           4510     4610     +100     
- Misses          571      595      +24     
- Partials        212      227      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

CHANGELOG.md

src/main/java/org/opensearch/knn/index/KNNMethodContext.java

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

jmazanec15 · 2024-03-12T22:25:14Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+        return stringBuilder.toString();
+    }
+
+    public static MethodComponentContext fromString(String in) {


This method will need a few comments describing what it will do. Also, lets add java doc

Added a java doc with a small description. Let me know if I need to add more

name this fromClusterStateString for consistency.

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

jmazanec15 · 2024-03-12T22:27:55Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+    public static MethodComponentContext fromString(String in) {
+        int index = 0;
+
+        String[] outerMethodComponentContextArray = in.split("\\{", -1);


Why do we need "\"?

The split method takes a regex as an input. { is a special character in regex so the \ escapes it to just split by {

Can we avoid this by just confirming the first and last character are {} and then building a substring from then which is processed.

The split still needs to be there for the recursion to work. Otherwise, I can't pass the whole substring of a nested MethodComponentContext because it will be split by ;

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

jmazanec15 · 2024-03-12T22:29:24Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+
+        String[] outerMethodComponentContextArray = in.split("\\{", -1);
+
+        if (outerMethodComponentContextArray[index].isEmpty()) {


Can we split this into a few methods? maybe parse name and parse parameters?

Sure, I'll try to see if it works.

Added a parse parameters method. Didn't add a parse name method because it's only two lines

jmazanec15 · 2024-03-12T22:31:23Z

@ryanbogan In PR description, can you add longer description of the change?

jmazanec15 · 2024-03-12T22:46:10Z

Also @ryanbogan can we update https://github.com/opensearch-project/k-NN/blob/main/src/main/resources/mappings/model-index.json

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan · 2024-03-12T23:33:40Z

Also @ryanbogan can we update https://github.com/opensearch-project/k-NN/blob/main/src/main/resources/mappings/model-index.json

Added

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

src/test/java/org/opensearch/knn/indices/ModelMetadataTests.java

jmazanec15 · 2024-03-13T15:39:02Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+    public static MethodComponentContext fromString(String in) {
+        int index = 0;
+
+        String[] outerMethodComponentContextArray = in.split("\\{", -1);


Can we avoid this by just confirming the first and last character are {} and then building a substring from then which is processed.

VijayanB · 2024-03-13T18:21:29Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+     *
+     * Provides a String representation of MethodComponentContext
+     * Sample return:
+     * {name=ivf;parameters=[nlist=4;type=fp16;encoder={name=sq;parameters=[nprobes=2;clip=false;]};]}


why do we need { and } at beginning and end? can we not skip it?

I added those to help with the recursion if there is a nested MethodComponentContext object. Otherwise, there is no way to differentiate the object from a normal field.

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 · 2024-03-14T18:27:10Z

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

+        return stringBuilder.toString();
+    }
+
+    public static MethodComponentContext fromString(String in) {


name this fromClusterStateString for consistency.

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

Signed-off-by: Ryan Bogan <[email protected]>

src/main/java/org/opensearch/knn/index/MethodComponentContext.java

src/main/java/org/opensearch/knn/indices/ModelMetadata.java

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15

lgtm thanks!

opensearch-trigger-bot · 2024-03-15T18:24:31Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1527-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4734d88b8b12ff3edd676e3a1f337fc814c8932f
# Push it to GitHub
git push --set-upstream origin backport/backport-1527-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-1527-to-2.x.

* Add MethodComponentContext to ModelMetadata Signed-off-by: Ryan Bogan <[email protected]> * Add changelog Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Change fromString Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Fix spotless Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit 4734d88)

* Persist model definition in model metadata (#1527) * Add MethodComponentContext to ModelMetadata Signed-off-by: Ryan Bogan <[email protected]> * Add changelog Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Change fromString Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Address PR Comments Signed-off-by: Ryan Bogan <[email protected]> * Fix spotless Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit 4734d88) * Fix compile errors Signed-off-by: Ryan Bogan <[email protected]> * Fix compile error Signed-off-by: Ryan Bogan <[email protected]> * Fix imports Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan added 2 commits March 12, 2024 13:39

Add MethodComponentContext to ModelMetadata

98ef3dd

Signed-off-by: Ryan Bogan <[email protected]>

Add changelog

0ba728e

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan marked this pull request as ready for review March 12, 2024 21:23

ryanbogan requested review from heemin32, navneet1v, VijayanB, vamshin, jmazanec15, naveentatikonda, junqiu-lei and martin-gaievski as code owners March 12, 2024 21:23

jmazanec15 reviewed Mar 12, 2024

View reviewed changes

jmazanec15 mentioned this pull request Mar 12, 2024

Add Range Validation for SQFP16 #1493

Merged

5 tasks

ryanbogan changed the title ~~Add MethodComponentContext to ModelMetadata~~ Persist model definition in model metadata Mar 12, 2024

Address PR Comments

d24baad

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan requested a review from jmazanec15 March 12, 2024 23:48

jmazanec15 reviewed Mar 13, 2024

View reviewed changes

VijayanB reviewed Mar 13, 2024

View reviewed changes

src/main/java/org/opensearch/knn/index/MethodComponentContext.java Outdated Show resolved Hide resolved

ryanbogan added 2 commits March 14, 2024 10:24

Address PR Comments

70a2f31

Signed-off-by: Ryan Bogan <[email protected]>

Rebase with main

cf049ea

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan requested review from VijayanB and jmazanec15 March 14, 2024 17:26

jmazanec15 mentioned this pull request Mar 14, 2024

[FEATURE] Enable setting ef_search parameter as part of k-NN Query #1537

Closed

jmazanec15 reviewed Mar 14, 2024

View reviewed changes

src/main/java/org/opensearch/knn/indices/ModelMetadata.java Outdated Show resolved Hide resolved

ryanbogan added 2 commits March 14, 2024 15:46

Change fromString

1bf49c2

Signed-off-by: Ryan Bogan <[email protected]>

Address PR Comments

a1f9e81

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 reviewed Mar 15, 2024

View reviewed changes

src/main/java/org/opensearch/knn/index/MethodComponentContext.java Outdated Show resolved Hide resolved

src/main/java/org/opensearch/knn/index/MethodComponentContext.java Outdated Show resolved Hide resolved

Address PR Comments

bd723e2

Signed-off-by: Ryan Bogan <[email protected]>

ryanbogan requested a review from jmazanec15 March 15, 2024 01:56

jmazanec15 reviewed Mar 15, 2024

View reviewed changes

src/main/java/org/opensearch/knn/indices/ModelMetadata.java Outdated Show resolved Hide resolved

ryanbogan added 2 commits March 14, 2024 19:18

Address PR Comments

4dcdaee

Signed-off-by: Ryan Bogan <[email protected]>

Fix spotless

f3a5521

Signed-off-by: Ryan Bogan <[email protected]>

jmazanec15 approved these changes Mar 15, 2024

View reviewed changes

VijayanB approved these changes Mar 15, 2024

View reviewed changes

ryanbogan merged commit 4734d88 into opensearch-project:main Mar 15, 2024
50 of 51 checks passed

ryanbogan deleted the model_definition branch March 15, 2024 18:24

ryanbogan added backport 2.x v2.13.0 labels Mar 15, 2024

This was referenced Mar 15, 2024

Persist model definition in model metadata #1547

Closed

[Backport 2.x] Persist model definition in model metadata #1548

Merged

ryanbogan mentioned this pull request Mar 18, 2024

Potential flaky bwc test failure around model management #1556

Closed

jmazanec15 mentioned this pull request Mar 18, 2024

Persist model definition for k-NN indices #1342

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist model definition in model metadata #1527

Persist model definition in model metadata #1527

ryanbogan commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

jmazanec15 Mar 12, 2024

ryanbogan Mar 12, 2024

jmazanec15 Mar 14, 2024

jmazanec15 Mar 12, 2024

ryanbogan Mar 12, 2024

jmazanec15 Mar 13, 2024

ryanbogan Mar 14, 2024

jmazanec15 Mar 12, 2024

ryanbogan Mar 12, 2024

ryanbogan Mar 12, 2024

jmazanec15 commented Mar 12, 2024

jmazanec15 commented Mar 12, 2024

ryanbogan commented Mar 12, 2024

jmazanec15 Mar 13, 2024

VijayanB Mar 13, 2024

ryanbogan Mar 13, 2024

jmazanec15 Mar 14, 2024

jmazanec15 left a comment

opensearch-trigger-bot bot commented Mar 15, 2024


		String[] outerMethodComponentContextArray = in.split("\\{", -1);

		if (outerMethodComponentContextArray[index].isEmpty()) {

Persist model definition in model metadata #1527

Persist model definition in model metadata #1527

Conversation

ryanbogan commented Mar 12, 2024 • edited Loading

Description

Check List

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 commented Mar 12, 2024

jmazanec15 commented Mar 12, 2024

ryanbogan commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmazanec15 left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Mar 15, 2024

ryanbogan commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading