Fix WithReference extension return type #3449
Merged
Conversation
This file contains hidden or 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
ghost
added
the
area-integrations
eerhardt
approved these changes
Apr 6, 2024
Member
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
Thanks! I'll cherry pick this into the release/8.0 PR
Member
|
This should be a generic method with a constraint IResourceWithEnviroment. See |
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]>
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.
Not caught in PR, but just discovered in use, the
WithReferencechanged the shape of the type fromProjectResourcetoIResourceWithEnvironment-- not making it compatible with other components.Microsoft Reviewers: Open in CodeFlow