-
Notifications
You must be signed in to change notification settings - Fork 36
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
Adding default use cases #583
Adding default use cases #583
Conversation
c679732
to
26b5eb2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
============================================
- Coverage 72.55% 72.15% -0.40%
- Complexity 659 670 +11
============================================
Files 80 81 +1
Lines 3399 3462 +63
Branches 268 278 +10
============================================
+ Hits 2466 2498 +32
- Misses 816 842 +26
- Partials 117 122 +5 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Amit Galitzky <[email protected]>
26b5eb2
to
74f58bc
Compare
Signed-off-by: Amit Galitzky <[email protected]>
src/main/java/org/opensearch/flowframework/util/ParseUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java
Outdated
Show resolved
Hide resolved
Some nit comments on this draft pr, will review again once the pr is ready for review. |
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.
Overall looks good to me, thanks @amitgalitz for this, definitely improves ease of use. Just had a few comments
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
Signed-off-by: Amit Galitzky <[email protected]>
Signed-off-by: Amit Galitzky <[email protected]>
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.
Have some design questions and code/style comments to discuss. Without a frontend, this functionality might appear somewhat complex to users. However, we are steadily advancing towards enhancing the framework's capabilities.
src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.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/transport/WorkflowRequest.java
Show resolved
Hide resolved
Signed-off-by: Amit Galitzky <[email protected]>
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.
Please create those 2 issues to either continue the discussion over the design or have a follow up PR. Rest looks good. Thanks for addressing the comments. @dbwiddis can you take a look and we can merge this in?
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! This is an awesome addition.
* initial default use case addition Signed-off-by: Amit Galitzky <[email protected]> * adding IT and UT Signed-off-by: Amit Galitzky <[email protected]> * addresing comments and adding more tests Signed-off-by: Amit Galitzky <[email protected]> * addressing more comments and adding more UT Signed-off-by: Amit Galitzky <[email protected]> * addressed more comments and more UT Signed-off-by: Amit Galitzky <[email protected]> --------- Signed-off-by: Amit Galitzky <[email protected]> (cherry picked from commit b148eb5) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Adding default use cases (#583) * initial default use case addition * adding IT and UT * addresing comments and adding more tests * addressing more comments and adding more UT * addressed more comments and more UT --------- (cherry picked from commit b148eb5) 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>
Description
The reason I went with a separate defaults file for now is so we don't make breaking changes on our template if customers gives us feedback for how to this differently.
Some concerns I have:
Customers might have a hard time knowing what to substitute, for example if they want to change connector post functions they need to know this key:
"create_connector.actions.pre_process_function": "<content>"
or for sparse search:
"register_local_sparse_encoding_model.function_name"
If we want to make these more flexibly its going to be hard to do substitution on things like the index mapping unless user gives a whole string version of there mapping, settings and alias and we make the entire configuration a substitution.
Currently you can see I have two versions of create connector - deploy - register. This is due that some connectors have more optional parameters then the others. One solution is having many files for each variation, another is to potentially have the connectors made with empty fields (for example
truncate
is only needed in cohere default so if we have that for an OpenAI connector it will just be there and unused).I only added 3 fully working defaults here for now, will have lots more to add if this approach seems fine. I want to also add a good amount of local models so that at least for fast POC or demo users can have a one click set up of an entire use case with 0 additional input like:
another example with current use cases in PR:
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
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.