-
Notifications
You must be signed in to change notification settings - Fork 3.7k
.Net: Add StateExtensions ADR, Abstractions, update AgentThread and add Unit tests. #11632
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
base: feature-conversation-state-extensions
Are you sure you want to change the base?
.Net: Add StateExtensions ADR, Abstractions, update AgentThread and add Unit tests. #11632
Conversation
dotnet/src/SemanticKernel.Core/Memory/ConversationStateExtensionsManagerExtensions.cs
Outdated
Show resolved
Hide resolved
/// Extension methods for <see cref="ConversationStateExtensionsManager"/>. | ||
/// </summary> | ||
[Experimental("SKEXP0001")] | ||
public static class ConversationStateExtensionsManagerExtensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming needs so work, using Extensions
twice in the same name seems like a bad code smell
/// the AI model in use just before invocation. | ||
/// </remarks> | ||
[Experimental("SKEXP0001")] | ||
public abstract class ConversationStateExtension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need Extension
in the name?
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtension.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtension.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtensionsManager.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtensionsManager.cs
Show resolved
Hide resolved
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtensionsManagerExtensions.cs
Outdated
Show resolved
Hide resolved
dotnet/src/SemanticKernel.Abstractions/Memory/ConversationStateExtensionsManagerExtensions.cs
Show resolved
Hide resolved
/// Gets or sets the container for conversation state extension components that manages their lifecycle and interactions. | ||
/// </summary> | ||
[Experimental("SKEXP0110")] | ||
public virtual ConversationStateExtensionsManager StateExtensions { get; init; } = new ConversationStateExtensionsManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property seems a little bit broad in meaning and might be a limiting factor later when there is a need to add another type of state extension. A narrower name could be ConversationStateExtensions
.
I am also entertaining the idea that the concept of context
might be a better alternative here instead of state
. It seems that context
is something more flexible and fluid compared to state
. The context can eventually be saved as a state or might not.
Another idea is to not use either context
or state
, and instead have just extensions and an extensions manager. This way, it will be up to the specific implementation of an extension to decide how to handle the On*
methods calls: whether to persist messages to the state, send them for analysis, telemetry, fact extraction, etc.
Motivation and Context
#10100
#10712
Description
This is the first PR of a number of PRs and adds
In follow up PRs I'll add:
Contribution Checklist