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: Moved IChatHistoryReducer from Agents to SK packages #10285

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dmytrostruk
Copy link
Member

@dmytrostruk dmytrostruk commented Jan 24, 2025

Motivation and Context

Resolves: #10203

This PR:

  • Moves IChatHistoryReducer from Agents.Core to Microsoft.SemanticKernel.Abstractions package.
  • Moves implementations ChatHistorySummarizationReducer and ChatHistoryTruncationReducer from Agents.Core to Microsoft.SemanticKernel.Core package.

The functionality is marked as experimental.

Contribution Checklist

@dmytrostruk dmytrostruk self-assigned this Jan 24, 2025
@dmytrostruk dmytrostruk requested a review from a team as a code owner January 24, 2025 00:58
@markwallace-microsoft markwallace-microsoft added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel kernel.core labels Jan 24, 2025
@dmytrostruk dmytrostruk changed the title .Net: Moved IChatHistoryReducer from Agents to SK Abstractions package .Net: Moved IChatHistoryReducer from Agents to SK packages Jan 24, 2025
/// Interface for reducing the chat history.
/// </summary>
[Experimental("SKEXP0001")]
public interface IChatHistoryReducer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we copied the one from concepts and not the one from agents. I.e. the one missing the equality methods. Do we not need the equality methods after all? I'm assuming that it would be odd to create and use two reducers with the same configuration anyway, so maybe the methods aren't required. Just double checking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two points:

  1. It looks like the equality methods are used within the Agents scenario, but if reducer abstraction is moved to SK Abstractions, I'm not sure if equality methods are required there. If somebody wants to implement a custom reducer and use it on ChatHistory in a scenario where equality doesn't mean anything, the developers should be able to do that without defining the equality methods.
  2. Even in Agents scenario, I'm not sure if Equals/GetHashCode methods should be a part of IChatHistoryReducer interface, since it's already available in System.Object and can be overridden in actual implementation. By defining it within the interface we just force the implementations to have these methods, but for such purposes maybe interface like IEquatable<T> would be a better option.

But in all cases, the first point is the main reason I decided to avoid equality methods in that interface, while the implementations ChatHistorySummarizationReducer and ChatHistoryTruncationReducer still have it, so it shouldn't impact already existing logic. If somebody wants their reducer to be used within the Agents scenario where it matters, it's possible to override them in implementations without any issues. Maybe we can specify this in the Agents documentation.

/// Using the existing <see cref="ChatHistory"/> for a reduction in collection size eliminates the need
/// for re-allocation (of memory).
/// </remarks>
public static async Task<bool> ReduceAsync(this ChatHistory history, IChatHistoryReducer? reducer, CancellationToken cancellationToken)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both the extension methods here can operate on ChatHistory since ChatHistory inherits from IReadOnlyList<ChatMessageContent>. Does this mean that in order to do a non mutating reduction, you have to do some casting first, like ((IReadOnlyList<ChatMessageContent>)myHistory).ReduceAsync(myReducer, ct)?
If so, would it make sense to be more explicit in the naming, so that it's easier to invoke one or the other on aChatHistory object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, would it make sense to support all 3 possible scenarios:

  1. Mutating ChatHistory
  2. Creating new ChatHistory with changes
  3. Create new IReadOnlyList<ChatMessageContent> with changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, accessing non-mutating reduction with casting only is not a great user experience. I renamed mutating reduction method to ReduceInPlaceAsync while the methods that returns a new instance are called ReduceAsync. I'm happy to chat more about naming if needed.

@@ -62,7 +62,7 @@ public async Task ShowHowToReduceChatHistoryToLastMessageAsync()
apiKey: TestConfiguration.OpenAI.ApiKey);

var truncatedSize = 2; // keep system message and last user message only
IChatCompletionService chatService = openAiChatService.UsingChatHistoryReducer(new TruncatingChatHistoryReducer(truncatedSize));
IChatCompletionService chatService = openAiChatService.UsingChatHistoryReducer(new ChatHistoryTruncationReducer(truncatedSize));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest moving the ChatHistoryReducer to its own file keeping it within MultiProviders prefix can be difficult to find, I would rather split our different ChatHistoryReducers_***Type of reducer** file.

Suggested change
IChatCompletionService chatService = openAiChatService.UsingChatHistoryReducer(new ChatHistoryTruncationReducer(truncatedSize));
IChatCompletionService chatService = openAiChatService.UsingChatHistoryReducer(new ChatHistoryTruncationReducer(truncatedSize));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but on the other hand, having these reducers in one file allows to run and compare them more easily. Since this file contains just 5 methods and around 200 lines of code, I don't have a strong preference about its structure at the moment. Maybe we can keep it as it is and split it later if it grows in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel.core kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Chat History Reducer is possibly in the wrong spot
4 participants