Narrower update to StreamingClientResult/SSE by trrwilson · Pull Request #68 · joseharriaga/openai-in-typespec

trrwilson · 2024-04-30T03:20:39Z

No description provided.

annelo-msft · 2024-04-30T16:27:41Z

+/// </summary>
+/// <typeparam name="T"> The data type representative of distinct, streamable items. </typeparam>
+public class StreamingClientResult<T>
+    : IDisposable


Should not implement IDisposable - see discussion in Azure/azure-sdk-for-net#43392 and related email thread.

annelo-msft · 2024-04-30T16:30:15Z

+    /// <returns> A new instance of <see cref="StreamingClientResult{T}"/>. </returns>
+    public static StreamingClientResult<T> Create(
+        PipelineResponse response,
+        Func<JsonElement, IEnumerable<T>> multiElementJsonDeserializerFunc,


We wouldn't take a Func in a ClientModel API - I found an alternate approach that works when you constrain T to IJsonModel that is illustrated in #49

Ah, just saw you added this below.

Got it; I was considering something similar; having an internal "collection of updates" type (that's the thing actually deserialized per chunk, StreamingChatResult in your PR) and doing the public enumeration from there does indeed get us out of the business of doing weird T : IJsonModel<IEnumerable<T>> things. I will snap to the clearer pattern with the intermediate internal type.

annelo-msft · 2024-04-30T16:31:09Z

+    /// The optional cancellation token used to control the enumeration.
+    /// </param>
+    /// <returns> A new instance of <see cref="StreamingClientResult{T}"/>. </returns>
+    public static StreamingClientResult<U> Create<U>(


Why wouldn't this be T?

Strangely, can't apply where clauses without making the method specifically generic (Create<T>) and then making the method generic emits an analyzer warning that's effectively "hey, that's a different T, FYI."

annelo-msft · 2024-04-30T16:31:57Z

+        finally
+        {
+            // Always dispose the stream immediately once enumeration is complete for any reason
+            contentStream.Dispose();


Disposal should be handled by the enumerator, not the enumerable.

KrzysztofCwalina · 2024-04-30T16:23:42Z

+        try
+        {
+            response = requestMessage.ExtractResponse();
+            ClientResult genericResult = ClientResult.FromResponse(response);


Do you need this genericResult? It does not seem to be used anywhere.

Without it (and exposure via GetRawResponse()), is there another mechanism by which we'd allow customers to query for response headers, etc.?

but you are not returning this genericResult to the user, and so they cannot read the headers anyway.

KrzysztofCwalina · 2024-04-30T16:26:48Z

+                                finishReason = null;
+                                continue;
+                            }
+                            finishReason = choiceProperty.Value.GetString() switch


this is lower pri, but would it be better to stay in UTF8 span world and use a sequence of if statements (like for top level properties)

KrzysztofCwalina · 2024-04-30T16:33:12Z

+    /// <returns> A new instance of <see cref="StreamingClientResult{T}"/>. </returns>
+    public static StreamingClientResult<T> Create(
+        PipelineResponse response,
+        Func<JsonElement, IEnumerable<T>> multiElementJsonDeserializerFunc,


Why do we need this parameter? Why the method below (with the constraint to IJsonModel) is not enoughl? In addition, we would need a Create method overload that returns StreamingClientResult for the protocol method case, i.e. when the caller want to get access to raw bytes of each event data so they can deserialize additional properties themselves.

The complexity arises because StreamingChatUpdate has a many-to-one relationship with the wire payload, since we "splat" the uncommon case of multiple choices on a single SSE into separate update instances.

Anne's suggestion to just add an internal intermediate type that implements the canonical T : IJsonModel<T> pattern should get around the need for this; StreamingChatUpdate will then not be directly serializable. (Open question from that: does that mean we should actually make the StreamingChatUpdateCollection type public, despite it not being directly consumed or produced by operations?)

annelo-msft · 2024-05-02T00:13:45Z

+    /// The optional cancellation token used to control the enumeration.
+    /// </param>
+    /// <returns> A new instance of <see cref="StreamingClientResult{T}"/>. </returns>
+    public static StreamingClientResult<U> Create<U>(PipelineResponse response, CancellationToken cancellationToken = default)


Curious: is this used anywhere in the OAI client?

annelo-msft · 2024-05-02T00:18:04Z

+        return new(response, GetServerSentEventDeserializationEnumerator<U>);
+    }
+
+    public static StreamingClientResult<TInstanceType> Create<TInstanceType, TJsonDataType>(PipelineResponse response, CancellationToken cancellationToken = default)


Curious: is the TJsonDataType strictly required? I notice that it is an internal collection type in your implementation. Is it only to represent a collection? Is the collection format part of the SSE spec? If so, I am wondering if the factory method could just be Create<T> somehow and the collection could be left as an internal implementation detail.

…entation (#43840) * Add async and sync enumerable client results * Implement IAsyncDisposable * Remove constrant and disposable; update ClientResult so response can be replaced in a polling paradigm * rename and upate tests * initial addition of files from joseharriaga/openai-in-typespec#68 * make it build * hello world test * bootstrap more tests * more internal tests * adding enumerator tests; haven't figured out the batch piece yet * Make batch test pass * remove collection-event functionality and add tests for public type * reshuffle * Add mock convenience SSE type to give POC of lazy request sending * add tests of delayed request * Add BinaryData factory method * remove funcs for creating enumerators * renames * postpone call to protocol method from convenience APIs * implement IAsyncDisposable correctly * initial pass over cancellation token * Per FDG, throw OperationCanceledException if cancellation token is cancelled. * remove factory method taking Func<T> and provide example of layering convenience implementation in a way that postpones sending the request * rename internal types and WIP adding reader tests * nits * parameterize terminal event; TBD to provide virtual method on collection type * WIP: nits * WIP: added concatenation of data lines per SSE spec * updates and bug fixes * add tests and update per SSE spec * WIP: refactor to reuse field processing across sync and async methods * make look a little more like the BCL type proposal * simplify field implementation a bit * cosmetic reworking of creating an event from a pending event * Remove factory method from public API; move MockSseClient to Tests.Internal to access internal SSE types * update API; reimplement mock client implementations without internal BinaryData enumerable * Add sync client result collection abstraction * tidy up and add tests * add default constructor to ClientResult * more tidy-up * rename and add refdocs * comments * pr fb * rework last event id and retry per BCL design shift * add CHANGELOG entry

* Add async and sync enumerable client results * Implement IAsyncDisposable * Remove constrant and disposable; update ClientResult so response can be replaced in a polling paradigm * rename and upate tests * initial addition of files from joseharriaga/openai-in-typespec#68 * make it build * hello world test * bootstrap more tests * more internal tests * adding enumerator tests; haven't figured out the batch piece yet * Make batch test pass * remove collection-event functionality and add tests for public type * reshuffle * Add mock convenience SSE type to give POC of lazy request sending * add tests of delayed request * Add BinaryData factory method * remove funcs for creating enumerators * renames * postpone call to protocol method from convenience APIs * implement IAsyncDisposable correctly * initial pass over cancellation token * Per FDG, throw OperationCanceledException if cancellation token is cancelled. * remove factory method taking Func<T> and provide example of layering convenience implementation in a way that postpones sending the request * rename internal types and WIP adding reader tests * nits * parameterize terminal event; TBD to provide virtual method on collection type * WIP: nits * WIP: added concatenation of data lines per SSE spec * updates and bug fixes * add tests and update per SSE spec * WIP: refactor to reuse field processing across sync and async methods * make look a little more like the BCL type proposal * simplify field implementation a bit * cosmetic reworking of creating an event from a pending event * Remove factory method from public API; move MockSseClient to Tests.Internal to access internal SSE types * update API; reimplement mock client implementations without internal BinaryData enumerable * Add sync client result collection abstraction * tidy up and add tests * add default constructor to ClientResult * more tidy-up * rename and add refdocs * comments * Add pageables * add ClientPage<T> * pageables implement enumerators * give page a response constructor * updates post merging main branch * Add hello world test for pageable collections * wip: tests * some API updates * make page non-abstract * add test for continuation token * add tests for async pageable * remove nonnull constraint from T on pageables * add comments to public types * thread through response updates * export API * update comments per pr fb * Expose factory method for creating ResultPage instead of constructor * update CHANGELOG * nit

trrwilson added 2 commits April 29, 2024 20:11

standalone port of sse updates

a4916d2

comments

2750a56

annelo-msft reviewed Apr 30, 2024

View reviewed changes

KrzysztofCwalina reviewed Apr 30, 2024

View reviewed changes

partial pattern fixup

e225ab5

annelo-msft reviewed May 2, 2024

View reviewed changes

annelo-msft added a commit to annelo-msft/azure-sdk-for-net that referenced this pull request May 2, 2024

initial addition of files from joseharriaga/openai-in-typespec#68

63e1351

trrwilson closed this May 21, 2024

trrwilson deleted the user/travisw/sse-update branch May 21, 2024 06:38

Conversation

trrwilson commented Apr 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trrwilson Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trrwilson Apr 30, 2024 •

edited

Loading