-
Notifications
You must be signed in to change notification settings - Fork 212
Add hybrid search support with overrideSearchType parameter #316
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?
Add hybrid search support with overrideSearchType parameter #316
Conversation
ff55c11 to
ede5652
Compare
- Add overrideSearchType parameter to tool input schema with HYBRID/SEMANTIC enum validation - Implement parameter validation and configuration in retrieve function - Add comprehensive tests for hybrid search functionality including error cases - Support both HYBRID and SEMANTIC search types as per AWS Bedrock API documentation - Fix line length violations for code quality compliance - Preserve all existing tests including test_retrieve_via_agent_with_enable_metadata
bac2016 to
44940db
Compare
| ), | ||
| "default": False, | ||
| }, | ||
| "overrideSearchType": { |
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.
curious about the naming here?
-
Are you using the override prefix to so the agent sets the field less frequently?
-
Is the usage different if you just name this searchType - the default behavior is still the same?
-
Are all knowledge bases able to support both types or can this break older knowledge bases?
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.
Great questions. I should have attached the relevant documentation:
https://docs.aws.amazon.com/bedrock/latest/APIReference/API_agent-runtime_KnowledgeBaseVectorSearchConfiguration.html
In answer to your questions:
- The naming is derived as is from the parameter used in the bedrock API call. This is also what you see in the bedrock console.
- I can, but I am following the convention of keeping the name same as the underlying bedrock API search param. I see the rest of the class follows the same convention.
- The documentation says right now only Amazon Opensearch based stores support it with support for other KB types coming soon. I have not tested the API with other KB types, but I have another KB backed by Kendra and the override search option does show up for it, so I am assuming it doesn't break older KBs. That being said, this is an optional parameter and should the API invalidate the request for other KBs, the user may simply opt to not set this field.
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.
Sounds good on the name.
But for the compatibility, the user doesn't set the field at all (in most cases), the Agent is the one setting the field. The agent can recover with an error message (if its good) but I don't want to waste customer tokens if we know this will happen a lot
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.
I validated an API call with a KB that doesn't support hybrid search and it outputs the following:
An error occurred (ValidationException) when calling the Retrieve operation: HYBRID search type is not supported for search operation on index NF4WREDULM. Retry your request with a different search type.
What would you suggest we do here? Should additional documentation over the tool that explicitly states "only supported by amazon opensearch and kendra knowledge bases, do not use with other sources" suffice 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.
Alternatively I can add a fallback in cases of ValidationException to remove this parameter, but that doesn't seem right to me.
Summary
This PR adds support for hybrid search functionality to the retrieve tool by implementing the
overrideSearchTypeparameter as requested in the AWS Bedrock Knowledge Base API.Changes
overrideSearchTypeparameter to the tool input schema with enum validation forHYBRIDandSEMANTICvaluesAPI Reference
This implementation follows the AWS Bedrock Knowledge Base API specification:
Usage Example
Testing
All existing tests pass, and new tests have been added to ensure:
Backward Compatibility
This change is fully backward compatible - existing code will continue to work without modification, and the new parameter is optional.