microsoft / semantic-kernel

Integrate cutting-edge LLM technology quickly and easily into your apps
https://aka.ms/semantic-kernel
MIT License
21.55k stars 3.19k forks source link

.Net: Microsoft.SemanticKernel.Connectors.Google,not implementing the ITextGenerationService interface #6373

Open LinzhouWang opened 4 months ago

LinzhouWang commented 4 months ago

Describe the bug Microsoft.SemanticKernel.Connectors.Google,not implementing the ITextGenerationService interface

Screenshots image

Platform

dmytrostruk commented 4 months ago

@LinzhouWang I think this is by design. Initial version of Google connector had ITextGenerationService, but the implementation was the same as IChatCompletionService, so we decided to avoid duplication. Please let us know if you have any scenarios where ITextGenerationService will work for you better than IChatCompletionService. Thanks!

LinzhouWang commented 4 months ago

@LinzhouWang I think this is by design. Initial version of Google connector had ITextGenerationService, but the implementation was the same as IChatCompletionService, so we decided to avoid duplication. Please let us know if you have any scenarios where ITextGenerationService will work for you better than IChatCompletionService. Thanks!

Firstly, thank you for your reply, The API interfaces provided by the Gemini big model are indeed the same, but as a standard for the application layer, I think we should implement the ITextGenerationService interface, such as Connectors OpenAI, Connectors. HuggingFace, and more

JonathanVelkeneers commented 4 months ago

@LinzhouWang I think this is by design. Initial version of Google connector had ITextGenerationService, but the implementation was the same as IChatCompletionService, so we decided to avoid duplication. Please let us know if you have any scenarios where ITextGenerationService will work for you better than IChatCompletionService. Thanks!

Kernel-memory has functionality to re-use semantic-kernel's ITextGenerationService connectors to broaden the available LLMs (example).

This is very useful and currently works fine for Huggingface, but the google connector needs a custom ITextGenerationService wrapper to be used in this scenario.

It would be great if the google connectors (and any future connectors) expose an ITextGenerationService and IChatCompletionService.

matthewbolanos commented 4 months ago

ITextGenerationService should only be specific to models that perform text generation (not chat completion). Do we know if Google provides a text generation model (like how OpenAI has gpt-3.5-turbo-instruct). If there isn't a text generation model, it might not make sense.

@dluc, Is there a reason why Kernel-memory doesn't support chat completion?

stephentoub commented 4 months ago

@matthewbolanos, what's the downside to also implementing ITextGenerationService? (Genuine question)

RogerBarreto commented 4 months ago

What's the downside to also implementing ITextGenerationService? (Genuine question)

Is not necessary, as we have Chat Completion extensions where you can pass the prompt as string instead of ChatHistory.

I also agree with @matthewbolanos on letting a clear message that our interfaces should be used against an AI Model for that modality.

stephentoub commented 4 months ago

we have Chat Completion extensions where you can pass the prompt as string instead of ChatHistory

Where? There are some for Kernel... where are those for IChatCompletionService?

I also agree with @matthewbolanos on letting a clear message that our interfaces should be used against an AI Model for that modality.

That's a reasonable position. To play devil's advocate, though, if someone is using AddXxTextGeneration for one service and wants to switch to a different one, it's an extra hurdle to switch to (and know you can switch to) AddXxChatCompletion, and then also update any code that was using GetRequiredService<ITextGenerationService> to switch to the other interface, change call sites to work with the other interface, etc. And it's not clear to me that there's any technical downside.

dmytrostruk commented 4 months ago

I remember my comment in initial Gemini text generation client implementation: https://github.com/microsoft/semantic-kernel/pull/5463#discussion_r1525331671 image

My main concern was that text generation client relies on chat completion client and works as adapter only without any additional logic, so I was wondering if we need it at all.

But if users want to use chat completion service as text generation service, it's always possible to create that adapter on user side, correct? I'm just not sure if we need to provide that out-of-the-box.

LinzhouWang commented 4 months ago

@dmytrostruk As a business process coordination program and an application layer, I believe that the ITextGenerationService interface should be implemented to achieve out of the box use