-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
.Net: Text Search Service ADR #6012
base: feature-text-search-service
Are you sure you want to change the base?
.Net: Text Search Service ADR #6012
Conversation
13b1f6f
to
2bae9e8
Compare
…nto users/markwallace/text-search-service-adr
…nto users/markwallace/text-search-service-adr
var stringPlugin = new TextSearchPlugin<string>(searchService); | ||
kernel.ImportPluginFromObject(stringPlugin, "StringSearch"); | ||
var pagePlugin = new TextSearchPlugin<BingWebPage>(searchService); | ||
kernel.ImportPluginFromObject(pagePlugin, "PageSearch"); | ||
|
||
// Invoke the plugin to perform a text search and return string values | ||
var question = "What is the Semantic Kernel?"; | ||
var function = kernel.Plugins["StringSearch"]["Search"]; | ||
var result = await kernel.InvokeAsync(function, new() { ["query"] = question }); | ||
|
||
Console.WriteLine(result); | ||
|
||
// Invoke the plugin to perform a text search and return BingWebPage values | ||
function = kernel.Plugins["PageSearch"]["Search"]; | ||
result = await kernel.InvokeAsync(function, new() { ["query"] = question, ["count"] = 2 }); |
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.
SearchAsync
method is generic, and each implementation has these checks: typeof(T) == typeof(string)
or typeof(T) == typeof(TextSearchResult)
. TextSearchPlugin
class is also generic, and for each specific type you need to create separate instance (e.g. TextSearchPlugin<string>
and TextSearchPlugin<BingWebPage>
).
My point during initial discussion was - with generics and type checking it's already a sacrifice, but at the end of the day we still can get only string
from kernel.InvokeAsync
, and if it's object, it will be serialized. I think it will be really useful to get the actual search result type like result.GetValue<BingWebPage>
after InvokeAsync
method. Otherwise, we can probably avoid using generics, work just with object
, and the result of this sample will still be the same.
Just some ideas what we can do in theory:
- Make
ITextSearchService<T>
generic. - For specific service (e.g.
BingTextSearchService
), implement interface with all possible types that can be produced by this service:BingTextSearchService: ITextSearchService<string>, ITextSearchService<BingWebPage>
- this should allow to avoidtypeof(T) == typeof(string)
check inSearchAsync
implementation. - Based on that, we don't need
TextSearchPlugin
class, because we can mark allSearchAsync
methods inBingTextSearchService
asKernelFunction
and name them as you did in line 27 and 29 -StringSearch
andPageSearch
. We will end up withBingPlugin-StringSearch
andBingPlugin-PageSearch
in kernel. If we share it with LLM, it will be able to decide which function to use based on user prompt. After specific function is invoked, it will return specific type, which will be possible to get withresult.GetValue<T>
.
Another way for point 3, we can keep TextSearchPlugin
as non-generic, which will accept ITextSearchService<string>
and/or ITextSearchService<WebPage>
in constructor. Then, it will be possible to inject any service that implements ITextSearchService<string>
and/or ITextSearchService<WebPage>
(e.g. Bing or Google). In this case, you won't need to initialize TextSearchPlugin
multiple times, DI can resolve all dependencies for you, there will be no need in type checking in search methods and complex type should be available as result of InvokeAsync
method.
Let me know what you think about it.
CancellationToken cancellationToken = default) | ||
{ | ||
var results = await this._service.SearchAsync<T>(query, new() { Count = count, Offset = offset }, null, cancellationToken).ConfigureAwait(false); | ||
var resultList = await results.Results.ToListAsync(cancellationToken).ConfigureAwait(false); |
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 think it will crash if used with AzureAITextSearchService
and T
is string
. There is no way to provide ValueField
.
snippetField
will be null in this line: https://github.com/microsoft/semantic-kernel/pull/6012/files#diff-a0f85f8bc652ca0b820565601ad9168994f1b8efee02118c0d1494bcb0d7e01bR143
Motivation and Context
This is a POC for the new text search service abstraction. Sharing very early for team collaboration.
Description
Contribution Checklist