-
Notifications
You must be signed in to change notification settings - Fork 138
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
Implement search alerts tool #1629
Implement search alerts tool #1629
Conversation
Relevant added tests pass:
|
ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/SearchAlertsTool.java
Outdated
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/SearchAlertsTool.java
Outdated
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/SearchAlertsTool.java
Outdated
Show resolved
Hide resolved
final int tableSize = parameters.containsKey("size") ? Integer.parseInt(parameters.get("size")) : 20; | ||
final String searchString = parameters.getOrDefault("searchString", null); | ||
|
||
// not exposing "missing" or "startIndex" from the table, using defaults of null/0, respectively |
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 would need to expose the start index to paginate the responses.
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.
Is pagination something feasible from the agent? It would need to process and combine multiple alert results after making multiple calls to the tools with different start index values. Is that possible?
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.
Well one call will get a list of alerts. I think this is a good question where we need to understand the limit of data we should be fetching, so we don't overwhelm the LLM. @jngz-es, do you have any recommendations here?
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.
With CoT agent it will be feasible to let LLM/agent to paginate the request to fetch more data. However, as part of tool output it must need to specify if it is a partial or complete output.
I would prefer to make the tool configurable to specify the max result size so that the agent configuration will have flexibility to tweak tool usage based on underlying model.
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.
Max results size will already be covered from tableSize
value. I'll change to expose startIndex
. My reasoning was to try to limit / scope what we expose and expand later when needed to try to keep things simple.
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.
Updated.
ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/SearchAlertsTool.java
Outdated
Show resolved
Hide resolved
ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/SearchAlertsTool.java
Outdated
Show resolved
Hide resolved
ml-algorithms/build.gradle
Outdated
@@ -47,6 +47,7 @@ dependencies { | |||
implementation("ai.djl.onnxruntime:onnxruntime-engine:0.21.0") { | |||
exclude group: "com.microsoft.onnxruntime", module: "onnxruntime" | |||
} | |||
implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.3.50" |
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.
Why need to add this dependency?
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.
For pulling in kotlin code from AlertingPluginInterface
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.
Does just using common-utils not provide this?
api "org.opensearch:common-utils:${common_utils_version}@jar"
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.
@ohltyler Could you please update the response here?
b8f5c6e
to
1641fa3
Compare
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
9689368
to
455ec8e
Compare
Rebased, confirmed all tests still passing. |
@ohltyler , I see Github CI still failed. Have you ran |
Yes, relevant tests pass, the error is unrelated to any of these changes. I'm just merging this to the dev branch. From my understanding this branch is not prioritized to have passing CI, and most changes are being directly merged, which is probably why there are failures. I'm still ensuring these changes aren't causing new failures. |
Check the CI error, and find these
From the error , seems opensearch can't start up. If that's the case, that's not just unit test issue, that mean the OS can't startup, that will block other guys to work on this dev branch. |
Thanks for the inputs, confirmed this dependency is causing some problems now, somehow only has started happening after the rebases. I've been testing different versions and using alerting plugin for reference on the kotlin integration. I will push a change after some more testing. |
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
I've changed to 1.8.21 to match that of alerting and ensure this isn't pulling in new/unknown dependencies. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/agent_framework_dev #1629 +/- ##
=================================================================
+ Coverage 67.13% 67.20% +0.06%
- Complexity 2488 2496 +8
=================================================================
Files 225 226 +1
Lines 12113 12163 +50
Branches 1249 1257 +8
=================================================================
+ Hits 8132 8174 +42
- Misses 3436 3437 +1
- Partials 545 552 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
742f920
into
opensearch-project:feature/agent_framework_dev
* Add search alerts tool Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Clean up test params; change validate() Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Add kotlin dependency Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * remove spotless Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * revert one dep Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Remove clusterservice; simplify param parsing Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Add TYPE; cast o/p to String Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Support null params passed Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Expose startIndex Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> * Bump kotlin to 1.8.21 Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com> --------- Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Description
Adds a search alerts tool. Utilizes getAlerts() exposed by the alerting interface in common utils.
Exposes all of the parameters offered by the interface, besides
missing
since that doesn't seem relevant (defaults tonull
). All parameters have defaults which match that of the get alerts API for now.The amount of parameters and/or the default values may be changed later on depending on agent performance and accuracy. For example, too much parameter tuning may lead to messy or invalid results.
Additionally, further integration testing will need to be done to validate different alerting-related questions input to the agent that will have this tool associated to it.
Issues Resolved
Closes #1521
Check List
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.