-
Notifications
You must be signed in to change notification settings - Fork 174
Fix missing RAG response from generative_qa_parameters #4118
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
base: main
Are you sure you want to change the base?
Fix missing RAG response from generative_qa_parameters #4118
Conversation
@rithin-pullela-aws nice way to compatible with search templates!!! can you add unit tests to verify your fix? also @junqiu-lei can you help review this fix? |
7570fba
to
cf2badc
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4118 +/- ##
============================================
- Coverage 81.81% 81.80% -0.01%
- Complexity 8847 8848 +1
============================================
Files 761 761
Lines 38099 38103 +4
Branches 4250 4250
============================================
+ Hits 31170 31172 +2
- Misses 5109 5110 +1
- Partials 1820 1821 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Agree, please also add integ test for template search with generative_qa_parameters. |
@rithin-pullela-aws if we are taking this approach, we need all custom search responses with search extensions to have the same fix. can you add the fix for this search response class too? ml-commons/plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchResponse.java Line 46 in 71d47e9
|
@mingshl, already included in this PR :) |
the fix LGTM, is it difficult to add in UT and IT to verify the fix? you need create a search template and create a ml inference processor and rag processor and then verify in the search response has extension. Maybe you can add the new IT in here https://github.com/opensearch-project/ml-commons/blob/main/plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchResponseProcessorIT.java |
@mingshl, was busy with other tasks. I will add an IT as you suggested |
Signed-off-by: rithin-pullela-aws <[email protected]>
cf2badc
to
321c32c
Compare
Description
Please refer to #4018 for detailed description of the issue.
Problem:
When using search templates with generative_qa_parameters, the RAG answer is missing from the response. This happens because:
The core SearchTemplateResponse calls response.innerToXContent() instead of response.toXContent() to preserve the status field, but GenerativeSearchResponse only adds the ext block in toXContent().
Solution:
Override innerToXContent() in GenerativeSearchResponse to include the ext block, ensuring both direct and template searches return RAG responses.
Alternative Considered:
Modify core SearchTemplateResponse to use toXContent(), but this would lose the status field that templates require.
Related Issues
Resolves #4018
Check List
--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.