ClientModel: AsyncResultCollection<T> and SSE event collection implementation #43840
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…be replaced in a polling paradigm
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
sdk/core/System.ClientModel/src/Internal/SSE/AsyncServerSentEventEnumerator.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/src/Internal/SSE/StreamingClientResult.cs
Outdated
Show resolved
Hide resolved
annelo-msft
commented
May 6, 2024
annelo-msft
changed the title
annelo-msft
requested review from
jsquire,
christothes,
pallavit,
trrwilson,
joseharriaga and
JoshLove-msft
jsquire
reviewed
May 10, 2024
sdk/core/System.ClientModel/src/Internal/SSE/AsyncServerSentEventEnumerable.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/src/Internal/SSE/ServerSentEvent.cs
Outdated
Show resolved
Hide resolved
sdk/core/System.ClientModel/src/Internal/SSE/ServerSentEventReader.cs
Outdated
Show resolved
Hide resolved
...ore/System.ClientModel/tests/internal/Convenience/SSE/AsyncServerSentEventEnumerableTests.cs
Outdated
Show resolved
Hide resolved
christothes
reviewed
May 13, 2024
christothes
reviewed
May 13, 2024
sdk/core/System.ClientModel/src/Internal/SSE/ServerSentEventReader.cs
Outdated
Show resolved
Hide resolved
christothes
approved these changes
May 14, 2024
jsquire
approved these changes
May 14, 2024
This was referenced May 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
System.ClientModel-based clients need to be able to return collections of values from service methods, for example, for endpoints such as OpenAI's streaming chat completions where incremental updates are delivered sequentially via SSE streams. In .NET, we represent such collections as async streams.
What's in this PR
This PR adds sync and async abstractions for collections of results,
ResultCollection<T>andAsyncResultCollection<T>. It also adds an internal implementation of an SSE parser and event enumerable that will likely be replaced by dotnet/runtime#98105 when it becomes available. The internal implementation is provided here to enable test coverage and review, as well as illustrate how clients would implement the abstractions to support both convenience method return values (see MockSseClient.cs) and factory-method helpers for use with protocol methods (see MockSseClientExtensions.cs).Additional notes
Given our timelines, the current implementation uses System.IO.StreamReader, which converts SSE lines to UTF-16. This adds a performance cost in cases where we convert the data payload back to UTF-8, e.g. for use with ModelReaderWriter.Create APIs. It is beyond the scope of this PR to provide UTF-8 stream reading capabilities, but part of our longer term plans for third-party clients.
Also: This builds on a great deal of prior work on the SSE reader by @KrzysztofCwalina and @trrwilson.
Addresses #42284