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

Create model client adapter for Semantic kernel ChatCompletionClientBase connectors #4741

Open
jackgerrits opened this issue Dec 17, 2024 · 5 comments
Assignees
Milestone

Comments

@jackgerrits
Copy link
Member

In order to make it easier to consume semantic kernel connects and expand our ecoystem we should create a wrapper that adapts ChatCompletionClientBase to ChatCompletionClient

https://github.com/microsoft/semantic-kernel/blob/main/python/semantic_kernel/connectors/ai/README.md

@lspinheiro
Copy link
Collaborator

@jackgerrits , I can try to help with this if needed

@jackgerrits
Copy link
Member Author

Sounds good!

@lspinheiro
Copy link
Collaborator

@jackgerrits I have an initial draft in #4851 . It is missing that streaming version of the methods but I want to ask some feedback before proceeding.

  1. I added an adapter from tools to sk kernel functions as well, should I move it to autogen_ext.tools.semantic_kernel?
  2. In SK, the relationship between tools and model clients in dependent on the kernel. Functions are added to the kernel which injects them into the client. We could artificially try to replicate this behavior if we don't want to kernel to be dependency but I'm not sure if this would have any real application or would be just for the purpose of trying to decouple them. Overall ,I think an adapter directly at the agent level could make more since.. Keen to hear your thoughts.
  3. In case we want to keep the kernel/client dependency, how do we want to handle function execution? It seems the natural behavior in semantic kernel is for functions to be automatically executed but we don't have this behavior in autogen at the client level.

@ekzhu
Copy link
Collaborator

ekzhu commented Dec 31, 2024

My thinking:

  1. We provide adapters like this one for SK tools and clients for component level integration. So yes to (1).
  2. We provide a functionally equivalent implementation of AssistantAgent, i.e., SemanticKernelAssistantAgent that takes an SK kernel as dependency. So to your (2) and (3) points, I think we want to make the default behavior consistent across our "Assistant" agent classes.

@jackgerrits
Copy link
Member Author

jackgerrits commented Dec 31, 2024

  1. That sounds great and a good value add. You should go ahead and get that merged on it its own.
  2. The kernel being a dependency seems fine and expected in my opinion. I like what you've done in terms of dynamically functions based on the create args. However, see below on what I think about functions in the kernel vs injected as you've done.
  3. I think since we are adapting to AutoGen, we need to follow the contract we have in place. So that means the kernel should not invoke tools and it should be left to the process we have in place for tool execution. It looks like that's how you have it in your PR?

Yeah so I think the SK assistant agent can follow more closely what a user would expect the kernel to do in terms of function calling. However, I think for a model client only component we need to follow the expected contract of the model client so that means no function execution, and so it seems that functions added directly to the kernel do not make sense in this context.

So ultimately there are two things that need to fit the expected contract in their context:

  1. A ChatCompletionClient adapter
  2. An AssistantAgent SK based impl

@jackgerrits jackgerrits modified the milestones: 0.4.1, 0.4.x Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants