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

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

Open
LinzhouWang opened this issue May 23, 2024 · 9 comments
Labels
ai connector Anything related to AI connectors .NET Issue or Pull requests regarding .NET code

Comments

@LinzhouWang
Copy link

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

Screenshots
image

Platform

  • OS: [e.g. Windows, Mac]
  • IDE: [e.g. Visual Studio, VS Code]
  • Language: C#
  • Source: [e.g. NuGet package version,Microsoft.SemanticKernel.Connectors.Google1.13.0-alpha, main branch of repository]
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code triage labels May 23, 2024
@github-actions github-actions bot changed the title Microsoft.SemanticKernel.Connectors.Google,not implementing the ITextGenerationService interface .Net: Microsoft.SemanticKernel.Connectors.Google,not implementing the ITextGenerationService interface May 23, 2024
@dmytrostruk
Copy link
Member

@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
Copy link
Author

@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
Copy link

JonathanVelkeneers commented May 24, 2024

@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
Copy link
Member

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
Copy link
Member

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

@RogerBarreto
Copy link
Member

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
Copy link
Member

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
Copy link
Member

I remember my comment in initial Gemini text generation client implementation:
#5463 (comment)
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
Copy link
Author

@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

@markwallace-microsoft markwallace-microsoft added ai connector Anything related to AI connectors and removed triage labels Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ai connector Anything related to AI connectors .NET Issue or Pull requests regarding .NET code
Projects
Status: No status
Development

No branches or pull requests

7 participants