-
Notifications
You must be signed in to change notification settings - Fork 29
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
Enhance RagTool to choose neural sparse query type #140
Conversation
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
known failure on CI GitHub Actions to run on JDK 21.0.1 due to opensearch-project/flow-framework#426 |
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.
Thanks for your work! But I think it still need some changes.
Besides the comments for this PR, Since RAGTool doesn't call getQueryBody, and doesn't use the logic in AbstractRetrieverTool, I think we don't need to extend the AbstractRetrieverTool. Now the retrieval part has been done in RAGTool's component, i.e. VectorDBTool and NeuralSparseTool. It doesn't make sense to extend the AbstractRetrieverTool for RAGTool itself.
I am more leaning towards keeping RAGTool extending from AbstractRetrieverTool, because it uses similar logic of creating a tool and run the parameters, it's also retrieving data through the sub tools(VectorDBTool and NeuralSparseTool). From the purpose of RAGTool, it retrieves data from subtool and use model for inference, so it should count towards a RetrieverTool as well. |
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #140 +/- ##
============================================
+ Coverage 80.83% 81.59% +0.75%
- Complexity 195 199 +4
============================================
Files 13 13
Lines 1002 1027 +25
Branches 133 133
============================================
+ Hits 810 838 +28
+ Misses 141 139 -2
+ Partials 51 50 -1 ☔ View full report in Codecov by Sentry. |
fixing the JDK issue in the commit Work around JDK 21.0.2 bug impacting scaling executors |
The core logic of AbstractRetrieverTool is There is also another implementation if we do want to extend AbstractRetrieverTool. In getQueryBody, we generate |
I also notice we still don't have integ test for RAGTool now. We can use
I don't think this is a blocker for this PR, but we need to add integ test in follow up PRs. |
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
.queryTool(neuralSparseSearchTool) | ||
.build(); | ||
case "neural": | ||
VectorDBTool.Factory.getInstance().init(client, xContentRegistry); | ||
params.put(VectorDBTool.MODEL_ID_FIELD, EMBEDDING_MODEL_ID_FIELD); | ||
params.put(VectorDBTool.MODEL_ID_FIELD, embeddingModelId); |
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 should also do this for NeuralSparseTool
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 don't need to call VectorDBTool.Factory.getInstance().init(client, xContentRegistry)
or NeuralSparseSearchTool.Factory.getInstance().init(client, xContentRegistry)
here. This has been done during the initialization of skills plugin
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.
added the initiation to the test to simulate the initiations of tools in ToolPlugins.java
mapped the inference_model_id to model_id for NeuralSparseTool
public String getVersion() { | ||
return null; | ||
} | ||
|
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 can also remove the extends logic in below lines. public static class Factory extends AbstractRetrieverTool.Factory<RAGTool>
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.
used Factory implements Tool.Factory
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
Signed-off-by: Mingshi Liu <mingshl@amazon.com>
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 can get PR this merged first, but may need to alter the prompt handling logic of RAGTool in the future based on e2e test result.
Need to fix the flaky test and add integration test for RAGTool in follow up PR.
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/skills/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/skills/backport-2.x
# Create a new branch
git switch --create backport/backport-140-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d7945d55dc28f95126891de8d8ca4ac2ea26e280
# Push it to GitHub
git push --set-upstream origin backport/backport-140-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/skills/backport-2.x Then, create a pull request where the |
* enhance RagTool to choose neural sparse query type Signed-off-by: Mingshi Liu <mingshl@amazon.com> * Work around JDK 21.0.2 bug impacting scaling executors Signed-off-by: Mingshi Liu <mingshl@amazon.com> * Modify RAGTool factory create to initiate subtools Signed-off-by: Mingshi Liu <mingshl@amazon.com> * Add enableContentGeneration to RAGTool Signed-off-by: Mingshi Liu <mingshl@amazon.com> * Map embedding_model_id to model_id for NeuralSparseTool Signed-off-by: Mingshi Liu <mingshl@amazon.com> * Map embedding_model_id to model_id for NeuralSparseTool Signed-off-by: Mingshi Liu <mingshl@amazon.com> --------- Signed-off-by: Mingshi Liu <mingshl@amazon.com> (cherry picked from commit d7945d5) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* enhance RagTool to choose neural sparse query type * Work around JDK 21.0.2 bug impacting scaling executors * Modify RAGTool factory create to initiate subtools * Add enableContentGeneration to RAGTool * Map embedding_model_id to model_id for NeuralSparseTool * Map embedding_model_id to model_id for NeuralSparseTool --------- (cherry picked from commit d7945d5) Signed-off-by: Mingshi Liu <mingshl@amazon.com> 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>
…t#140) (opensearch-project#152) * enhance RagTool to choose neural sparse query type * Work around JDK 21.0.2 bug impacting scaling executors * Modify RAGTool factory create to initiate subtools * Add enableContentGeneration to RAGTool * Map embedding_model_id to model_id for NeuralSparseTool * Map embedding_model_id to model_id for NeuralSparseTool --------- (cherry picked from commit d7945d5) Signed-off-by: Mingshi Liu <mingshl@amazon.com> 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 <yuyezhu@amazon.com>
Description
semantic search is based on text embedding models, and neural sparse search is based on sparse embedding models. We are now enhancing RagTool with a parameter
query_type
, to enable both options.now that users can set parameter
query_type=neural
to run neural query ,or set parameter
query_type=neural_sparse
to run neural_sparse queryThere are two phases of RAGTool, first is through retrieval phase, to query, and second phase is content generation through inference mode.
The newly added parameter
enable_Content_Generation
, the default is true, When it sets to false, it will return search retrieval result directly. When it sets to true, the RAGTool will continue trigger inference model to generate content.Also fixing the build failure related to JDK 21
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.