[Enhancement] Fetch system index mappings from json file instead of string constants #3153
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,15 +18,15 @@ | |
| package org.opensearch.ml.common.conversation; | ||
|
There was a problem hiding this comment. @austintlee @HenryL27 I have also modified this class to use the MLIndex enum, please review the same. I didn't want to do a full refactor for now as it may impact functionality. Added as a todo on top of the class. |
||
|
|
||
| import org.opensearch.common.settings.Setting; | ||
| import org.opensearch.ml.common.MLIndex; | ||
|
|
||
| /** | ||
| * Class containing a bunch of constant defining how the conversational indices are formatted | ||
| * ToDo: use MLIndex.MEMORY_MESSAGE and MLIndex.MEMORY_META directly for index names and mappings rather than constants | ||
| */ | ||
| public class ConversationalIndexConstants { | ||
| /** Name of the conversational metadata index */ | ||
| public final static String META_INDEX_NAME = MLIndex.MEMORY_META.getIndexName(); | ||
| /** Name of the metadata field for initial timestamp */ | ||
| public final static String META_CREATED_TIME_FIELD = "create_time"; | ||
| /** Name of the metadata field for updated timestamp */ | ||
|
|
@@ -41,38 +41,10 @@ public class ConversationalIndexConstants { | |
| public final static String META_ADDITIONAL_INFO_FIELD = "additional_info"; | ||
|
|
||
| /** Mappings for the conversational metadata index */ | ||
| public final static String META_MAPPING = MLIndex.MEMORY_META.getMapping(); | ||
|
|
||
| /** Name of the conversational interactions index */ | ||
| public final static String INTERACTIONS_INDEX_NAME = MLIndex.MEMORY_MESSAGE.getIndexName(); | ||
| /** Name of the interaction field for the conversation Id */ | ||
| public final static String INTERACTIONS_CONVERSATION_ID_FIELD = "memory_id"; | ||
| /** Name of the interaction field for the human input */ | ||
|
|
@@ -92,42 +64,7 @@ public class ConversationalIndexConstants { | |
| /** The trace number of an interaction */ | ||
| public final static String INTERACTIONS_TRACE_NUMBER_FIELD = "trace_number"; | ||
| /** Mappings for the interactions index */ | ||
| public final static String INTERACTIONS_MAPPINGS = MLIndex.MEMORY_MESSAGE.getMapping(); | ||
|
|
||
| /** Feature Flag setting for conversational memory */ | ||
| public static final Setting<Boolean> ML_COMMONS_MEMORY_FEATURE_ENABLED = Setting | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| { | ||
| "_meta": { | ||
| "schema_version": 2 | ||
| }, | ||
| "properties": { | ||
| "name": { | ||
| "type": "text", | ||
| "fields": { | ||
| "keyword": { | ||
| "type": "keyword", | ||
| "ignore_above": 256 | ||
| } | ||
| } | ||
| }, | ||
| "type": { | ||
| "type": "keyword" | ||
| }, | ||
| "description": { | ||
| "type": "text" | ||
| }, | ||
| "llm": { | ||
| "type": "flat_object" | ||
| }, | ||
| "tools": { | ||
| "type": "flat_object" | ||
| }, | ||
| "parameters": { | ||
| "type": "flat_object" | ||
| }, | ||
| "memory": { | ||
| "type": "flat_object" | ||
| }, | ||
| "is_hidden": { | ||
| "type": "boolean" | ||
| }, | ||
| "created_time": { | ||
| "type": "date", | ||
| "format": "strict_date_time||epoch_millis" | ||
| }, | ||
| "last_updated_time": { | ||
| "type": "date", | ||
| "format": "strict_date_time||epoch_millis" | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "_meta": { | ||
| "schema_version": 4 | ||
| }, | ||
| "properties": { | ||
| "master_key": { | ||
| "type": "keyword" | ||
| }, | ||
| "config_type": { | ||
| "type": "keyword" | ||
| }, | ||
| "ml_configuration": { | ||
| "type": "flat_object" | ||
| }, | ||
| "create_time": { | ||
| "type": "date", | ||
| "format": "strict_date_time||epoch_millis" | ||
| }, | ||
| "last_updated_time": { | ||
| "type": "date", | ||
| "format": "strict_date_time||epoch_millis" | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
implementation group: 'org.json', name: 'json', version: '20231013'
I think this serves as both line 28 + 29 so no needs to declare twice here for the same dependency. Also if you want to go the extra mile, using "api group: 'org.json', name: 'json', version: '20231013'" can remove the duplicate of this dependency in other modules.
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 wanted to keep the dependency strictly based on the requirement,
implementationwill add it during runtime as well and here we only needed it during testing runtime. Wdyt?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.
Fair enough and fine with this new line. But if you check this (https://github.com/opensearch-project/ml-commons/blob/main/ml-algorithms/build.gradle#L76), it's already added in runtime in several module too.
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 I actually tried setting it as
implementationinitially but i received a lot of JarHell errorsCaused by: java.lang.IllegalStateException at JarHell.java:316due to multiple implementations of this package and which is why i left it astestImplementation, I remember attempting to resolve the dependencies but it became a bit convolutedThere 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.
Ref PR: https://github.com/opensearch-project/ml-commons/pull/1062/files