Initial work for Qdrant resource and component #3131
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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)
- change how dashboard is removed in Publish (per docs) - add 'qdrant' to spelling.dic - fix tests
- Add WithReference overload (setting cn string + key) - Added tests for named parameter on manifest - Changed playground app
- adds component (using Qdrant.QdrantClient) - Change playground to use DI component
- changed to 'rest' as named endpoint
- added to reference as a ConnectionString_{name}_rest (for support for SK)
- fixed tests
dotnet-issue-labeler
bot
added
the
area-integrations
davidfowl
reviewed
Mar 24, 2024
davidfowl
reviewed
Mar 24, 2024
davidfowl
reviewed
Mar 24, 2024
davidfowl
reviewed
Mar 24, 2024
davidfowl
reviewed
Mar 24, 2024
- Changed parameters to component config - change playground app to keyed services - changed playground to use shared servicedefaults
|
Really nice to see this!
FWIW reimplementing the SK connector over the SDK is something that's on my TODO list... that would make the SK connector more efficient thanks to gRPC etc.
Can you point out what trace/metrics in e.g. the Python client look like? This may be something we can improve. |
- matching sample Qdrant/.NET workbook sample
This is great to hear @roji! For now the Resource will expose both endpoints so the SK SDK as-is can get the |
eerhardt
reviewed
Mar 27, 2024
eerhardt
reviewed
Mar 28, 2024
- 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
davidfowl
reviewed
Mar 28, 2024
Change to ReferenceExpression.Create
davidfowl
reviewed
Mar 29, 2024
davidfowl
reviewed
Mar 29, 2024
eerhardt
reviewed
Mar 29, 2024
|
|
||
| // REST endpoint | ||
| var restEndpoint = qdrantResource.Resource.GetEndpoint("rest"); | ||
| context.EnvironmentVariables[$"ConnectionStrings__{qdrantResource.Resource.Name}_rest"] = ReferenceExpression.Create($"Endpoint={restEndpoint};Key={qdrantResource.Resource.ApiKeyParameter}"); |
There was a problem hiding this comment.
Who reads this _rest environment variable?
There was a problem hiding this comment.
As noted in the original PR, Qdrant exposes two endpoints for access: gRPC (which the client component uses by default), and a REST endpoint (btw, I'm using their terms). The second is currently what you'd use to compose with SK (although Shay is hoping to change that but not there yet) and also is where the Qdrant management web UI is exposed (note: this is disabled in publish mode).
eerhardt
reviewed
Mar 29, 2024
- Rename component correctly - Add component client tests - Add component logging (and document) - Add property for 2nd endpoint on resource directly
|
@eerhardt I think I addressed most of your feedback (thanks for the pointers on logging). |
|
On the hosting side this is looking pretty good. I'm wondering whether when we deploy to ACA we should favor the use of the dev services (it supports quadrant). If that is the case then perhaps there should be an Aspire.Hosting.Azure.Qdrant package? |
|
We don’t favor any of the aca services anymore so I would day no. Not as part of this PR and we’d do something like redis first (which requires container app cdk support). Might be worth spiking PublishAsAcaRedis or something similar |
glennc
reviewed
Apr 2, 2024
eerhardt
reviewed
Apr 4, 2024
There was a problem hiding this comment.
This is looking really great. Thanks for the contribution @timheuer!
I pushed some minor changes and fix ups to the PR.
If you are ready to take this out of "Draft", I think it can be approved. The one remaining comment I have can be addressed in a follow up PR if we think we should do it.
| /// The API Key of the Qdrant server to connect to. | ||
| /// </summary> | ||
| public string? Key { get; set; } | ||
|
|
There was a problem hiding this comment.
I see that the QdrantClient also supports a grpcTimeout option. Should we allow for configuring that on our Settings object?
cc @roji
| Vectors = new [] {0.106827796f, 0.21676421f}, | ||
| Payload = | ||
| { | ||
| ["title"] = "Pulp Function" |
… other components. Add class level summary docs.
eerhardt
approved these changes
Apr 4, 2024
eerhardt
added a commit
to eerhardt/aspire
that referenced
this pull request
Apr 4, 2024
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]>
davidfowl
reviewed
Apr 5, 2024
davidfowl
reviewed
Apr 5, 2024
eerhardt
added a commit
that referenced
this pull request
Apr 8, 2024
…3416) * Initial work for Qdrant resource and component (#3131) 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]> * Addressing port/endpoint issues on Qdrant (#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]> * Fix WithReference extension return type (#3449) * Fix Qdrant WithReference to allow any IResourceWithEnvironment. Also fix the same pattern in AWS. --------- Co-authored-by: Tim Heuer <[email protected]>
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Initial proposal for #795
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 secred 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.
The component doesn't seem to be rich in trace/metrics so those are not implemented...ideas?
Little runtime demo with playground+dashboard apps:
runtimedemo.mp4
Microsoft Reviewers: Open in CodeFlow