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 IT for VectorDBTool and NeuralSparseTool #177

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

mingshl
Copy link
Contributor

@mingshl mingshl commented Feb 3, 2024

Description

Add IT for VectorDBTool and NeuralSparseTool

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.

@mingshl
Copy link
Contributor Author

mingshl commented Feb 3, 2024

IT test failed due to the new merged PR in mlcommon

opensearch-project/ml-commons#1949

{"error":{"root_cause":[{"type":"illegal_state_exception","reason":"Agent Framework is currently disabled. To enable it, update the setting \"plugins.ml_commons.agent_framework_enabled\" to true."}],"type":"illegal_state_exception","reason":"Agent Framework is currently disabled. To enable it, update the setting \"plugins.ml_commons.agent_framework_enabled\" to true."},"status":500}

Copy link
Member

@zhichao-aws zhichao-aws left a comment

Choose a reason for hiding this comment

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

missing integ test with enable_content_generation enabled

@mingshl
Copy link
Contributor Author

mingshl commented Feb 19, 2024

missing integ test with enable_content_generation enabled

added tests for with enable_content_generation
-testRAGToolWithNeuralQueryAndLLM
-testRAGToolWithNeuralSparseQueryAndLLM
-testRAGToolWithNeuralSparseQueryAndLLM_withIllegalSourceField_thenGetEmptySource
-testRAGToolWithNeuralQueryAndLLM_withIllegalSourceField_thenGetEmptySource
-testRAGToolWithNeuralSparseQueryAndLLM_withIllegalEmbeddingField_thenThrowException
-testRAGToolWithNeuralQueryAndLLM_withIllegalEmbeddingField_thenThrowException
-testRAGToolWithNeuralSparseQueryAndLLM_withIllegalIndexField_thenThrowException
-testRAGToolWithNeuralQueryAndLLM_withIllegalIndexField_thenThrowException
-testRAGToolWithNeuralSparseQueryAndLLM_withIllegalModelIdField_thenThrowException
-testRAGToolWithNeuralQueryAndLLM_withIllegalModelIdField_thenThrowException

@mingshl mingshl requested a review from zhichao-aws February 20, 2024 17:23
PromptHandler RAGToolHandler = new PromptHandler() {
@Override
String response(String prompt) {
if (prompt.contains("RAGTool")) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some lines to verify the full prompt after concatenation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mingshl
Copy link
Contributor Author

mingshl commented Mar 5, 2024

not able to reproduce the build failure locally

./gradlew ':integTest' --tests "org.opensearch.integTest.RAGToolIT.testRAGToolWithNeuralQuery" -Dtests.seed=E4F502402E2AE0C1 -Dtests.security.manager=false -Dtests.locale=es-ES -Dtests.timezone=Asia/Jerusalem                 

> Task :extractAdJar UP-TO-DATE
> Task :extractJsJar UP-TO-DATE
> Task :extractSqlJar UP-TO-DATE
> Task :addJarsToClasspath UP-TO-DATE
> Task :processResources UP-TO-DATE
> Task :generateNotice
> Task :copyPluginPropertiesTemplate
> Task :pluginProperties UP-TO-DATE
> Task :processTestResources UP-TO-DATE
> Task :generateEffectiveLombokConfig
> Task :compileJava UP-TO-DATE
> Task :classes UP-TO-DATE
> Task :jar
> Task :bundlePlugin
> Task :generateTestEffectiveLombokConfig
> Task :compileTestJava UP-TO-DATE
> Task :testClasses UP-TO-DATE
> Task :integTest

Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

For more on this, please refer to https://docs.gradle.org/8.6/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.

BUILD SUCCESSFUL in 1m 21s
17 actionable tasks: 8 executed, 9 up-to-date

wondering why the integ test running on git CI is having one less digit in the scores.

  2> org.junit.ComparisonFailure: The agent execute response not equal with expected. expected:<...","_score":0.5671488[6]}
    {"_index":"test_ne...> but was:<...","_score":0.5671488[]}```

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.57%. Comparing base (8b279f3) to head (e0483d9).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #177   +/-   ##
=========================================
  Coverage     81.57%   81.57%           
  Complexity      211      211           
=========================================
  Files            13       13           
  Lines          1069     1069           
  Branches        146      146           
=========================================
  Hits            872      872           
  Misses          140      140           
  Partials         57       57           

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

@zhichao-aws
Copy link
Member

How about using regex to match the first valid digits and add comments here

@mingshl
Copy link
Contributor Author

mingshl commented Mar 6, 2024

How about using regex to match the first valid digits and add comments here

I already used string.contains to match the scores prefix the avoid the digits rounding issues. Can you please help rerun that Mac CI? That one is failing because model is not ready yet. Rerunning should fix it m. Thanks!

@zhichao-aws zhichao-aws merged commit cbe5fa2 into opensearch-project:main Mar 6, 2024
18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 6, 2024
* Add IT for VectorDBTool and NeuralSparseTool

Signed-off-by: Mingshi Liu <[email protected]>

* changed to smaller models and refine IT tests

Signed-off-by: Mingshi Liu <[email protected]>

* add match full prompt

Signed-off-by: Mingshi Liu <[email protected]>

* fix digits rounding differences in CI

Signed-off-by: Mingshi Liu <[email protected]>

---------

Signed-off-by: Mingshi Liu <[email protected]>
(cherry picked from commit cbe5fa2)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhichao-aws pushed a commit that referenced this pull request Mar 6, 2024
* Add IT for VectorDBTool and NeuralSparseTool



* changed to smaller models and refine IT tests



* add match full prompt



* fix digits rounding differences in CI



---------


(cherry picked from commit cbe5fa2)

Signed-off-by: Mingshi Liu <[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
…opensearch-project#255)

* Add IT for VectorDBTool and NeuralSparseTool

* changed to smaller models and refine IT tests

* add match full prompt

* fix digits rounding differences in CI

---------

(cherry picked from commit cbe5fa2)

Signed-off-by: Mingshi Liu <[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.

2 participants