Skip to content
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

Add visualization tool #41

Merged
merged 8 commits into from
Dec 27, 2023
Merged

Add visualization tool #41

merged 8 commits into from
Dec 27, 2023

Conversation

Hailong-am
Copy link
Contributor

Description

Visualization Tool, coming from opensearch-project/ml-commons#1649

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed 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: Hailong Cui <[email protected]>
@Hailong-am
Copy link
Contributor Author

please help to add backport 2.x label also

@Hailong-am Hailong-am changed the title Visualization Tool Add visualization tool Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (7e0ea54) 44.41% compared to head (4c7399d) 50.35%.

Files Patch % Lines
src/main/java/org/opensearch/agent/ToolPlugin.java 0.00% 7 Missing ⚠️
...org/opensearch/agent/tools/VisualizationsTool.java 91.52% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #41      +/-   ##
============================================
+ Coverage     44.41%   50.35%   +5.94%     
- Complexity       30       38       +8     
============================================
  Files             6        7       +1     
  Lines           358      423      +65     
  Branches         42       49       +7     
============================================
+ Hits            159      213      +54     
- Misses          178      187       +9     
- Partials         21       23       +2     

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

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some nits.

Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
Signed-off-by: Hailong Cui <[email protected]>
@dhrubo-os
Copy link
Collaborator

Need to resolve the conflict


public static final String SAVED_OBJECT_TYPE = "visualization";
private static final String DEFAULT_DESCRIPTION =
"Use this tool to find user created visualizations. This tool takes the visualization name as input and returns the first 3 matching visualizations";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does 3 needs to be static? Can't we do like 3 by default but the input is dynamic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good suggestion, update the number to dynamic setting.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be update the description as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the specific number in description has been removed. now it's "Use this tool to find user created visualizations. This tool takes the visualization name as input and returns the matching visualizations"

@Hailong-am
Copy link
Contributor Author

Need to resolve the conflict

resolved

@zane-neo zane-neo merged commit 3774eb9 into opensearch-project:main Dec 27, 2023
12 of 13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 27, 2023
* Visualization Tool

Signed-off-by: Hailong Cui <[email protected]>

* fix build failure due to forbiddenApis

Signed-off-by: Hailong Cui <[email protected]>

* Address review comments

Signed-off-by: Hailong Cui <[email protected]>

* spotlessApply

Signed-off-by: Hailong Cui <[email protected]>

* update default tool name

Signed-off-by: Hailong Cui <[email protected]>

* update number of visualization be dynamic

Signed-off-by: Hailong Cui <[email protected]>

---------

Signed-off-by: Hailong Cui <[email protected]>
(cherry picked from commit 3774eb9)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhichao-aws pushed a commit that referenced this pull request Dec 27, 2023
* Visualization Tool



* fix build failure due to forbiddenApis



* Address review comments



* spotlessApply



* update default tool name



* update number of visualization be dynamic



---------


(cherry picked from commit 3774eb9)

Signed-off-by: Hailong Cui <[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>
yuye-aws pushed a commit to yuye-aws/skills that referenced this pull request Apr 26, 2024
* Visualization Tool

* fix build failure due to forbiddenApis

* Address review comments

* spotlessApply

* update default tool name

* update number of visualization be dynamic

---------

(cherry picked from commit 3774eb9)

Signed-off-by: Hailong Cui <[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>
Signed-off-by: yuye-aws <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants