-
Notifications
You must be signed in to change notification settings - Fork 510
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
[release/8.0] Initial work for Qdrant resource and component (#3131) #3416
Conversation
This PR adds a Resource and initial Component (plus playground and tests) Resource exposes two API access endpoints (REST + gRPC). The QdrantClient for .NET uses gRPC by default but Semantic Kernel does not use that client library so the other exposed endpoint is helpful. REST endpoint by default exposes a Web UI dashboard -- this is excluded in Publish (per documentation in source, confirmed with Qdrant contributors). Endpoints are secured non-optionally with an API Key (using ParameterResource -- will generate random if not provided). Component used QdrantClient .NET version (recommended from Qdrant). This component uses gRPC endpoint by default. Settings will read from standard env vars for ConnectionString + API key or from config for the component itself if provided. * Initial draft of QdrantServerResource - exposed gRPC endpoint as primary (c# client uses that) - exposes HTTP endpoint optionally (as dashboard is there) - defaults to secure the access via API key (mainly due to dashboard) * Fix-up after main rebase on removing InputAnnotation * Remove dashboard in Publish mode - change how dashboard is removed in Publish (per docs) - add 'qdrant' to spelling.dic - fix tests * Fixup code style violations * Change ApiKey reference - Add WithReference overload (setting cn string + key) - Added tests for named parameter on manifest - Changed playground app * Initial Qdrant component work - adds component (using Qdrant.QdrantClient) - Change playground to use DI component * Changed rest endpoint name - changed to 'rest' as named endpoint - added to reference as a ConnectionString_{name}_rest (for support for SK) - fixed tests * Addressing PR feedback - Changed parameters to component config - change playground app to keyed services - changed playground to use shared servicedefaults * Fixing volume mounts to correct target location * Add missing README to component * Adding logo usage comment to readme after permission from Andrey V from Qdrant * Changed playground sample - matching sample Qdrant/.NET workbook sample * PR Feedback: Name maps to client name used * PR feedback on connection sring - Moved to Endpoint/Key connection string - Renamed to Key - Modified schema to match/updated tests - Changed primary endpoint to use scheme instead of hardcode - removed env var for API key only - fixed renamed component proj location in sln * Updating readme/comments to match config * Endpoint property typo fix * PR Feedback - No need for endpoint null check - Fixed tests * Cleanup of ServiceDefaults Change to ReferenceExpression.Create * More PR Feedback - Rename component correctly - Add component client tests - Add component logging (and document) - Add property for 2nd endpoint on resource directly * PR feedback * Fix up the playground to run with latest changes. * Rename QdrantSettings to QdrantClientSettings to match the pattern in other components. Add class level summary docs. --------- Co-authored-by: Eric Erhardt <[email protected]>
Is there feedback on the original PR still to address? |
Yes, I left some comments. |
Can you cherry-pick into this the other feedback fixups in #3422 |
Marking as "no merge" for now, to wait for the changes in #3422. |
* Addressing port/endpoint issues on Qdrant * PR feedback (param name) * Fix up tests * Respond to PR feedback. Rename ports and endpoint names to be consistent. * PR feedback - moved to using grpc/http endpoint name consistent with config and prior art - fixed wrong link in readme - fixed/validated resource hosting test --------- Co-authored-by: Eric Erhardt <[email protected]>
The changes in #3422 are now in this PR as well. This PR is ready for review. |
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.
Approved in chat.
Need to also take #3487 into this PR. |
Also fix the same pattern in AWS.
Backport of #3131 to release/8.0
/cc @timheuer
Customer Impact
Being able to simply use a Qdrant vector database container and easily get a QdrantClient connected to it using Aspire makes it easier to be able to write smart applications.
Adding Qdrant Hosting and runtime component libraries makes it easy to work with this vector database.
Testing
Added new tests for hosting and the component.
Manually ran the playground app.
Risk
Very low. This only adds 2 new packages and doesn't touch anything in already shipping packages. If these packages aren't referenced by the user, it won't affect the user at all.
This PR adds a Resource and initial Component (plus playground and tests)
Resource exposes two API access endpoints (REST + gRPC). The QdrantClient for .NET uses gRPC by default but Semantic Kernel does not use that client library so the other exposed endpoint is helpful.
REST endpoint by default exposes a Web UI dashboard -- this is excluded in Publish (per documentation in source, confirmed with Qdrant contributors).
Endpoints are secured non-optionally with an API Key (using ParameterResource -- will generate random if not provided).
Component used QdrantClient .NET version (recommended from Qdrant). This component uses gRPC endpoint by default. Settings will read from standard env vars for ConnectionString + API key or from config for the component itself if provided.
Fix-up after main rebase on removing InputAnnotation
Remove dashboard in Publish mode
Fixup code style violations
Change ApiKey reference
Fixing volume mounts to correct target location
Add missing README to component
Adding logo usage comment to readme after permission from Andrey V from Qdrant
Changed playground sample
PR Feedback: Name maps to client name used
PR feedback on connection sring
Updating readme/comments to match config
Endpoint property typo fix
PR Feedback
Cleanup of ServiceDefaults Change to ReferenceExpression.Create
More PR Feedback
PR feedback
Fix up the playground to run with latest changes.
Rename QdrantSettings to QdrantClientSettings to match the pattern in other components.
Add class level summary docs.
Microsoft Reviewers: Open in CodeFlow