Skip to content

Conversation

stephentoub
Copy link
Member

@stephentoub stephentoub commented Nov 6, 2024

WithMessageAddedAsync enables writing code like:

List<ChatMessage> messages = ...;
await foreach (var update in client.CompleteStreamingAsync(messages).WithMessageAddedAsync(messages))
{
    Console.Write(update);
}

and upon completion of the loop, messages will contain a ChatMessage merged from all of the updates.

ToChatCompletion is what's used to achieve that, but is also exposed on its own so that code which has an enumerable of StreamingChatCompletionUpdates can merge them into a ChatCompletion. This enables a non-streaming implementation to easily be authored in terms of a streaming one.

Microsoft Reviewers: Open in CodeFlow

@stephentoub stephentoub requested a review from a team as a code owner November 6, 2024 22:22
@stephentoub stephentoub force-pushed the messageadditionextensions branch from dd9e6d5 to bdf387f Compare November 7, 2024 01:29
@stephentoub stephentoub changed the title Add StreamingChatCompletionUpdateExtensions WithMessageAddedAsync / MergeUpdates helpers Add StreamingChatCompletionUpdateExtensions WithMessageAddedAsync / ToChatCompletion helpers Nov 7, 2024
@eiriktsarpalis
Copy link
Member

WithMessageAddedAsync enables writing code like:

I hadn't realized that CompleteStreamingAsync doesn't update the chat history after the IAE has been enumerated. I'm guessing that this is influenced by the deferred execution of the resultant IAE, but it does seem possible we could go the other way and require coalescence to be a part of CompleteStreamingAsync for consistency with CompleteAsync.

@stephentoub
Copy link
Member Author

WithMessageAddedAsync enables writing code like:

I hadn't realized that CompleteStreamingAsync doesn't update the chat history after the IAE has been enumerated. I'm guessing that this is influenced by the deferred execution of the resultant IAE, but it does seem possible we could go the other way and require coalescence to be a part of CompleteStreamingAsync for consistency with CompleteAsync.

Neither of them update the chat history with the resulting message.

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Nov 7, 2024

The naming of WithMessageAddedAsync feels odd. Phrasing it in the past tense sounds as if the message has already been added in some way, but it adds the message in the future after the streaming completes.

Do any of the following feel right to you?

await foreach (var update in client.CompleteStreamingAsync(messages).AppendMessageTo(messages))
await foreach (var update in client.CompleteStreamingAsync(messages).AppendResultTo(messages))
await foreach (var update in client.CompleteStreamingAsync(messages).AppendResponseTo(messages))
await foreach (var update in client.CompleteStreamingAsync(messages).WithAppendTo(messages))
await foreach (var update in client.CompleteStreamingAsync(messages).ThenAppendTo(messages))

I mildly lean towards AppendResultTo or AppendResponseTo but don't feel strongly.

@eiriktsarpalis
Copy link
Member

I tend to associate the Append prefix with methods that return void; With more accurately reflects we're decorating the underlying IAE with behaviour; Then is typically used when appending callbacks in the style of Task.ContinueWith which isn't precisely what this method is doing.

@stephentoub
Copy link
Member Author

stephentoub commented Nov 7, 2024

The hard part here is the ToChatCompletion. With that, someone can do the equivalent of WithMessageAddedAsync with:

List<StreamingChatCompletionUpdate> updates = [];
await foreach (var update in client.CompleteStreamingAsync(messages))
{
    Console.Write(update);
    updates.Add(update);
}
messages.Add(updates.ToChatCompletion().Message);

Given that all of the raised concerns are about WithMessageAddedAsync, I'll just remove that for now.

@stephentoub
Copy link
Member Author

stephentoub commented Nov 7, 2024

This is now just:

public static ChatCompletion ToChatCompletion(
    this IEnumerable<StreamingChatCompletionUpdate> updates, bool coalesceContent = true)

@stephentoub
Copy link
Member Author

(though now that I write that out, I'm wondering if I should add one for IAsyncEnumerable as well)

@stephentoub stephentoub force-pushed the messageadditionextensions branch from 0e845f5 to dda3072 Compare November 7, 2024 22:30
@stephentoub stephentoub changed the title Add StreamingChatCompletionUpdateExtensions WithMessageAddedAsync / ToChatCompletion helpers Add ToChatCompletion{Async} methods for combining StreamingChatCompletionUpdates Nov 7, 2024
@stephentoub
Copy link
Member Author

Refactored to have ToChatCompletion{Async} and to have them in the Abstractions library. Having them there means a leaf client can choose to use e.g. ToChatCompletionAsync to implement its CompleteAsync method around its CompleteStreamingAsync, if desired.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

As Microsoft.Extensions.AI.Abstractions is accumulating more and more helper methods, I'm growing somewhat uncomfortable with the "Abstractions" suffix in this project. Would it make sense to rename this to be just Microsoft.Extensions.AI and then have the existing Microsoft.Extensions.AI called something along the lines of Microsoft.Extensions.AI.Middlewares?

cc @SteveSandersonMS

@stephentoub
Copy link
Member Author

stephentoub commented Nov 8, 2024

As Microsoft.Extensions.AI.Abstractions is accumulating more and more helper methods, I'm growing somewhat uncomfortable with the "Abstractions" suffix in this project. Would it make sense to rename this to be just Microsoft.Extensions.AI and then have the existing Microsoft.Extensions.AI called something along the lines of Microsoft.Extensions.AI.Middlewares?

cc @SteveSandersonMS

I think it's fine. These helpers are to aid in the development of implementations of the abstractions. It's very much in line with other M.E libs.

Copy link
Member

@SteveSandersonMS SteveSandersonMS left a comment

Choose a reason for hiding this comment

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

Looks good.

In a previous iteration of this PR, I think you were able to remove the equivalent logic from CachingChatClient and use your new helper instead. But in the current version of the PR that's no longer the case. Is it no longer possible to avoid having two versions of this logic?

@stephentoub
Copy link
Member Author

Is it no longer possible to avoid having two versions of this logic?

I put these into m.e.ai.abstractions, and the caching stuff is up in m.e.ai. We could reconsolidate if we wanted to expose the coalescing publicly from abstractions.

@stephentoub stephentoub merged commit c86b7ea into dotnet:main Nov 8, 2024
6 checks passed
@stephentoub stephentoub deleted the messageadditionextensions branch November 8, 2024 12:47
@stephentoub
Copy link
Member Author

Is it no longer possible to avoid having two versions of this logic?

I put these into m.e.ai.abstractions, and the caching stuff is up in m.e.ai. We could reconsolidate if we wanted to expose the coalescing publicly from abstractions.

Went another way: #5616

stephentoub added a commit to stephentoub/extensions that referenced this pull request Nov 19, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 2024
@stephentoub stephentoub requested a review from Copilot December 17, 2024 00:39
@jeffhandley jeffhandley added the area-ai Microsoft.Extensions.AI libraries label Mar 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ai Microsoft.Extensions.AI libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants