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: SK Agents #4740

Merged
merged 7 commits into from
Feb 4, 2024
Merged

.Net: SK Agents #4740

merged 7 commits into from
Feb 4, 2024

Conversation

SergeyMenshykh
Copy link
Member

Motivation and Context

This ADR outlines high-level ideas/design for SK agents.

@SergeyMenshykh SergeyMenshykh added PR: ready for review All feedback addressed, ready for reviews .NET Issue or Pull requests regarding .NET code labels Jan 25, 2024
@SergeyMenshykh SergeyMenshykh self-assigned this Jan 25, 2024
@shawncal shawncal added documentation and removed .NET Issue or Pull requests regarding .NET code labels Jan 25, 2024
@crickman crickman self-requested a review January 29, 2024 19:07
@crickman crickman modified the milestone: R4: Cycle 3 Jan 29, 2024
@crickman
Copy link
Contributor

I am also curious if "user-proxy" agent might need to be addressed to fully support agent collaboration/chat patterns?

@crickman
Copy link
Contributor

FWIW - ADR "31" is already overloaded

@crickman
Copy link
Contributor

Perhaps we can consider the upcoming Azure support: Azure/azure-sdk-for-net#40817

@joslat
Copy link
Contributor

joslat commented Jan 30, 2024

I am also curious if "user-proxy" agent might need to be addressed to fully support agent collaboration/chat patterns?

Continuing on @crickman reply, adding some thoughts:

Why not implementing all the Autogen agent types? as in https://github.com/microsoft/autogen/pull/924/files#diff-9860f01d79405521f2af05fe4040ed4f50498c9108956723f4bc5f89d88ab5ad :

  • ConversableAgent
  • AssistantAgent
  • GroupChatManager
  • UserProxyAgent...

IMHO this implementation is gold, and a basis for inspiration - the only issue I see is that is bound to OpenAI API.
I like the GroupChat & more streamlined SequentialGroupChat - this is very needed IMHO

But let me get to the core, I mean Kernel ;)
The other point I want to bring is a suggestion on the design which the ADR does not mention: kernel centricity.
There is no central kernel in the agent story. A new one is created on each Agent, that's why we see the configuration and the model passed on the agent builder. And then it is hidden from the developer, it is internal. IMHO this is not good.

Why not pass the "central" kernel on its builder, and then use it? in case that we need parallelized execution - the main reason I believe that triggered this design decision - each agent has its own kernel (heart) - why not create a pool of kernels that are a clone from the main, central kernel (no configuration needed) and then each agent can take the next available, free, kernel/heart? ;)

This way they can execute in parallel and this approach would simplify the architecture (read: reduce code) and increase performance by reusing ready-to-use kernels.

This pool of kernels, could be easily coordinated by the kernel itself, keeping the kernel centricity. Maybe with a thread safe collection of kernels that have a flag if it is used and by who, each with a semaphore to avoid race conditions, et voila... we have a better solution than creating all the kernels beforehand. It also would scale better and be more performant.

And also solve the multi-model compatability (sort of) - as the Kernel is supporting already different AI Models. So we could tell the agent what serviceId to use (as https://github.com/microsoft/semantic-kernel/blob/main/dotnet/samples/KernelSyntaxExamples/Example62_CustomAIServiceSelector.cs)

Thoughts?

@SergeyMenshykh
Copy link
Member Author

I am also curious if "user-proxy" agent might need to be addressed to fully support agent collaboration/chat patterns?

Yes, it may be necessary. Once the agents' abstraction is in place, we will begin adding concrete implementations for agents, and the "user-proxy" agent could be one of those implementations. There will be a user story to track the "user-proxy" agent for SK.

@SergeyMenshykh
Copy link
Member Author

FWIW - ADR "31" is already overloaded

Yes, thank you. I will change the number before submitting the PR to the main branch.

@SergeyMenshykh
Copy link
Member Author

Perhaps we can consider the upcoming Azure support: Azure/azure-sdk-for-net#40817

Yep, definitely, we should migrate our agent to the new API when it's available. I'll create an issue to track this.

@SergeyMenshykh
Copy link
Member Author

Why not implementing all the Autogen agent types? as in https://github.com/microsoft/autogen/pull/924/files#diff-9860f01d79405521f2af05fe4040ed4f50498c9108956723f4bc5f89d88ab5ad :

  • ConversableAgent
  • AssistantAgent
  • GroupChatManager
  • UserProxyAgent...

Once the agent's abstraction is in place, it will be a matter of prioritizing the design and implementation for the agents in a way that aligns with SK's overall approach, including DI, filters, tools, and so on.

@SergeyMenshykh
Copy link
Member Author

But let me get to the core, I mean Kernel ;) The other point I want to bring is a suggestion on the design which the ADR does not mention: kernel centricity. There is no central kernel in the agent story. A new one is created on each Agent, that's why we see the configuration and the model passed on the agent builder. And then it is hidden from the developer, it is internal. IMHO this is not good.

Why not pass the "central" kernel on its builder, and then use it? in case that we need parallelized execution - the main reason I believe that triggered this design decision - each agent has its own kernel (heart) - why not create a pool of agents and then each agent can take the next available, free, kernel? ;)

And that could be easily coordinated by the kernel, with a semaphore, thread-safe et voila... we have a better solution than creating all the kernels beforehand. It also would scale better and be more performant.

And also solve the multi-model compatability (sort of) - as the Kernel is supporting already different AI Models.

Thoughts?

Even though it is not stated in the ADR, and it should have been and will be, every feature we design or add to SK is supposed to be built around the kernel and leverage as much as possible of all the benefits SK provides or supports, such as DI, plugins, function calling, abstractions, etc. Therefore, agents are no exception in this regard.

Regarding passing the kernel to agents, it is a good idea and aligns with the way we plan to implement agents. SK agents should be flexible and accept an instance of a kernel provided by the SK consumer code. Whether it is one instance of the kernel for all agents, or each agent gets its own instance, it is up to the SK consumer code to decide, depending on the scenario it wants to implement.

Kernel/Agents Pool: It might make sense to arrange agents or kernels into a pool if the cost of creating or destroying them is high. For example, if the classes use expensive system resources that can be shared and reused, it would be logical to organize them in a pool to minimize system resource usage. Regardless, the SK consumer code has a few mechanisms to optimize the cost if it is high. A kernel/agent can be registered as a singleton and reused during the hosting app's lifetime to minimize the cost. If the singleton solution is not sufficient, nothing prevents it from pooling kernels or agents if that is what the hosting app requires.

@AshD
Copy link

AshD commented Jan 30, 2024

What is the rationale for the Agent class requiring the Assistant APIs?

AutoGen uses the Chat Completion API which is implemented by multiple providers like vLLM, Llama.cpp, etc.

Thanks,
Ash

@joslat
Copy link
Contributor

joslat commented Jan 31, 2024

What is the rationale for the Agent class requiring the Assistant APIs?

AutoGen uses the Chat Completion API which is implemented by multiple providers like vLLM, Llama.cpp, etc.

Thanks, Ash

Hi Ash,
Initially The agents was developed on top of the OpenAI Assistants API, which offers some features like thread management, tools, function calling and files (not supported just yet but in short will - there are two PRs ongoing)

But as the team said, they want to decouple from the OpenAI implementation, and after this "base work is done" - which is what this ADR is about, to provide a base architecture/structure from where to build the agent story for all models (and also OpenAI).
@SergeyMenshykh correct me if I am wrong or not precise enough...

docs/decisions/0031-agents.md Outdated Show resolved Hide resolved
docs/decisions/0031-agents.md Outdated Show resolved Hide resolved
@AshD
Copy link

AshD commented Feb 2, 2024

What is the rationale for the Agent class requiring the Assistant APIs?

AutoGen uses the Chat Completion API which is implemented by multiple providers like vLLM, Llama.cpp, etc.

Thanks, Ash

Hi Ash,

Initially The agents was developed on top of the OpenAI Assistants API, which offers some features like thread management, tools, function calling and files (not supported just yet but in short will - there are two PRs ongoing)

But as the team said, they want to decouple from the OpenAI implementation, and after this "base work is done" - which is what this ADR is about, to provide a base architecture/structure from where to build the agent story for all models (and also OpenAI).

@SergeyMenshykh correct me if I am wrong or not precise enough...

Thanks @joslat Looking forward to test this

@SergeyMenshykh SergeyMenshykh added this pull request to the merge queue Feb 4, 2024
Merged via the queue into microsoft:main with commit 8931ab6 Feb 4, 2024
12 checks passed
@SergeyMenshykh SergeyMenshykh deleted the agents-adr branch February 4, 2024 12:44
Bryan-Roe pushed a commit to Bryan-Roe-ai/semantic-kernel that referenced this pull request Oct 6, 2024
### Motivation and Context
This ADR outlines high-level ideas/design for SK agents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agents documentation PR: ready for review All feedback addressed, ready for reviews
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants