Skip to content
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

Consider changing IChatClient/IEmbeddingGenerator.GetService to be non-generic #5607

Closed
stephentoub opened this issue Nov 7, 2024 · 9 comments · Fixed by #5608
Closed

Consider changing IChatClient/IEmbeddingGenerator.GetService to be non-generic #5607

stephentoub opened this issue Nov 7, 2024 · 9 comments · Fixed by #5608

Comments

@stephentoub
Copy link
Member

stephentoub commented Nov 7, 2024

We currently have:

TService? GetService<TService>(object? key = null)
    where TService : class;

That prohibits its use if you only have a Type. If we instead had:

object? GetService(Type serviceType, object? key = null)

then you could use it with either a generic or non-generic, and we could provide a generic wrapper, e.g.

public static TService GetService<TService>(this IChatClient client, object? key = null) where TService : class =>
    (TService?)client.GetService(typeof(TService), key);

ala what's done with IServiceProvider.

I ran into this while experimenting while prototyping implementing a delegating chat client with delegates for all of its members. There isn't a good way to have a strongly-typed delegate that parameterizes an instance and provides an implementation for GetService<TService> today, because the TService can change with every call. If it were instead based on Type, that wouldn't be an issue.

cc: @SteveSandersonMS

@SteveSandersonMS
Copy link
Member

Agreed, accepting a Type seems very reasonable and inline with our APIs elsewhere.

@JohnGalt1717
Copy link

It appears that this borke a ton of stuff. Now I can use OllamaEmbeddingGenerator with Micrsoft.Extensions.AI.Ollama because you get the following: Method 'GetService' in type 'Microsoft.Extensions.AI.OllamaEmbeddingGenerator' from assembly 'Microsoft.Extensions.AI.Ollama, Version=9.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.'

doesn't matter what combination of versions I try.

But here's what I have:

<PackageVersion Include="Microsoft.Extensions.AI" Version="9.0.0-preview.9.24507.7" />
<PackageVersion Include="Microsoft.Extensions.AI.Ollama" Version="9.0.0-preview.9.24507.7" />
<PackageVersion Include="Microsoft.Extensions.AI.OpenAI" Version="9.0.0-preview.9.24507.7" />

@stephentoub
Copy link
Member Author

Those are the versions from two months ago. This change isn't in those, so something else you're using is pulling in a newer version. Change your version numbers to be 9.0.1-preview.1.24570.5.

@JohnGalt1717
Copy link

JohnGalt1717 commented Nov 27, 2024

It also does it with that version.

And I also get this:

Method 'GetService' in type 'Microsoft.Extensions.AI.OllamaChatClient' from assembly 'Microsoft.Extensions.AI.Ollama, Version=9.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35' does not have an implementation.

So it's happening with ALL versions of Ollama (at least)

I have Kernel Memory also installed if that would matter and the Aspire Ollama stuff if that matters.

Edit: Yes, it's a conflict with: CommunityToolkit.Aspire.OllamaSharp

@SteveSandersonMS
Copy link
Member

There was an update to OllamaSharp last week: awaescher/OllamaSharp#144

It may be that explicitly referencing OllamaSharp 4.0.8 will fix this.

@stephentoub
Copy link
Member Author

stephentoub commented Nov 27, 2024

It may be that explicitly referencing OllamaSharp 4.0.8 will fix this.

If you're using OllamaSharp, yes, but you said you're using OllamaChatClient, which doesn't use OllamaSharp. (EDIT: I didn't see that the last response was from Steve rather than John)

(Are you using SK? SK needs to ship updated nuget packages to ship the changes that fold this in. cc: @RogerBarreto)

@JohnGalt1717
Copy link

I'm using KernelMemory not Semantic Kernel. Same problem though I would assume?

@JohnGalt1717
Copy link

FWIW, removing the Ollama stuff for Aspire and doing the connection info manually, solves the issue with 9.0.0-preview.9.24556.5

It doesn't solve the issue with: 9.0.1-preview.1.24570.5 which has the same error back again.

Major version issues here to get this to work across the entire ecosystem of aspire, SK, (or KM) and M.E.AI.

@stephentoub
Copy link
Member Author

Can you share a standalone repro? I'm not sure what dependency is causing the problem, but to my knowledge everything on nuget that depended on the old version has an updated version that depends on the newest.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants