-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fixing create index and use case input parsing bugs #600
Conversation
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
a9f1146
to
583ec86
Compare
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
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.
Let's wait for opensearch-project/ml-commons#2211 to be merged before merging this
You went with the default way of creating the memory id. I see
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.
LGTM with a few suggestions.
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateIndexStep.java
Outdated
Show resolved
Hide resolved
Yes had a conversation with Yaliang on this, for now users will have to create there own conversation and put into the rag processor, conversation as user created isn't too bad as these are basically per user conversation and should be created for each new one. If we created a conversation here and user got it from state index it's fairly similar amount of calls for the users, Yaliang suggested we auto create this conversation idea in RAG processor if processor doesn't get one in the search request. I overall agree with that, it is probably too tight for 2.13 though. |
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 are doing so much of regex. I am worried it might fail if user has some complex mappings or payload. Is there any parsing library we can use to achieve the same?
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/CreateIndexStep.java
Show resolved
Hide resolved
Can we have a follow up issue to handle it later? |
src/main/java/org/opensearch/flowframework/workflow/CreateIndexStep.java
Show resolved
Hide resolved
Signed-off-by: Amit Galitzky <[email protected]>
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Show resolved
Hide resolved
src/main/resources/defaults/conversational-search-defaults.json
Outdated
Show resolved
Hide resolved
src/main/resources/defaults/conversational-search-defaults.json
Outdated
Show resolved
Hide resolved
src/main/resources/substitutionTemplates/conversational-search-with-cohere-model-template.json
Show resolved
Hide resolved
Signed-off-by: Amit Galitzky <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #600 +/- ##
============================================
- Coverage 72.75% 72.54% -0.22%
- Complexity 681 684 +3
============================================
Files 82 82
Lines 3491 3526 +35
Branches 279 285 +6
============================================
+ Hits 2540 2558 +18
- Misses 832 845 +13
- Partials 119 123 +4 ☔ View full report in Codecov by Sentry. |
6f8122f
to
cad55fa
Compare
@owaiskazi19 I changed method to what you suggested for ParseUtils.removingBackslashesAndQuotesInArrayInJsonString, I also made slight change to ParseUtils.parseStringToObjectMap() in latest commit |
src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java
Show resolved
Hide resolved
* fixing create index step and array input for processors Signed-off-by: Amit Galitzky <[email protected]> * fixing test and enhancing error message Signed-off-by: Amit Galitzky <[email protected]> * adding release notes change and some comments addressed Signed-off-by: Amit Galitzky <[email protected]> * addressed comments and cleaned up defaults/templates Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit 5018ebc) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…602) Fixing create index and use case input parsing bugs (#600) * fixing create index step and array input for processors * fixing test and enhancing error message * adding release notes change and some comments addressed * addressed comments and cleaned up defaults/templates --------- (cherry picked from commit 5018ebc) Signed-off-by: Amit Galitzky <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@amitgalitz we need to forward port this to main |
…ct#600) * fixing create index step and array input for processors Signed-off-by: Amit Galitzky <[email protected]> * fixing test and enhancing error message Signed-off-by: Amit Galitzky <[email protected]> * adding release notes change and some comments addressed Signed-off-by: Amit Galitzky <[email protected]> * addressed comments and cleaned up defaults/templates Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]>
…gs (#600) (#618) Fixing create index and use case input parsing bugs (#600) * fixing create index step and array input for processors * fixing test and enhancing error message * adding release notes change and some comments addressed * addressed comments and cleaned up defaults/templates --------- Signed-off-by: Amit Galitzky <[email protected]>
Description
Multiple bug fixes:
Address the inconsistency created by this issue: [BUG] Inconsistency for create index between REST and CreateIndex OpenSearch#12775, we want to have the same behavior as users today expect through the regular Create Index REST API. This means that we shouldn't expect _doc in the mapping and we should add it ourselves, this is exactly what core does on the rest layer but have missed doing on transport layer. https://github.com/opensearch-project/OpenSearch/blob/2.x/server/src/main/java/org/opensearch/rest/action/admin/indices/RestCreateIndexAction.java#L106
We know correctly expect arrays in the input fields for the default use cases, I make sure to correctly escape the strings weather we are expecting a numerical array or string array. For example:
this is done through parsing these arrays into strings in RestCreateWorkflowAction (we currently support inputs of string, array, map of strings of strings) and handleing the backlashes in createpipelinestep to correctly send the body to the transport action
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.