ClientModel: Add async and sync enumerable client results #43392

annelo-msft · 2024-04-12T22:51:05Z

Draft design proposal for discussion for AsyncEnumerableResult<T> and EnumerableClientResult<T> types to add to System.ClientModel.

Based on requirements from the OpenAI clients to support streaming endpoints for chat and assistants APIs, we have the need to be able to return a result from clients' service methods that represents a collection of values returned via the response's ContentStream.

In .NET, this is best modeled as an async stream, and this means such a ClientResult type in System.ClientModel should implement the IAsyncEnumerable interface. Since System.ClientModel clients also support synchronous service methods, we will provide a synchronous counterpart to the asynchronous that implements IEnumerable.

@tg-msft noted that these "streaming client results" look a lot like Azure.Core's AsyncPageable<T> and Pageable<T> types, and it is true that to the extent that are simply abstractions that represent collections of values, where streaming vs. polling is an implementation detail.

There are discussion points in the comments of this PR noting that we could use a GetRawResponses method in place of a GetRawResponse method to support implementations that use multiple responses to return subsets of elements in the collection.

Addresses:

annelo-msft · 2024-04-12T22:54:16Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+namespace System.ClientModel;
+
+#pragma warning disable CS1591 // public XML comments
+public abstract class AsyncEnumerableResult<T> : ClientResult, IAsyncEnumerable<T>, IDisposable


Inheriting from ClientResult adds the GetRawResponse method to this type. For enumerable client result implementations that use a polling mechanism and therefore have multiple responses associated with them, we might need to either say "GetRawResponse returns the first response and subtypes must expose other public APIs to access subsequent responses," or possibly expose a GetRawResponses method that returns a collection of PipelineResponse values. The latter is not a great Dev UX in the streaming response case where there is only a single response returned, but the former could require us to add additional types to support polling-enumerable scenarios such as paging.

annelo-msft · 2024-04-12T22:57:21Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+
+#pragma warning disable CS1591 // public XML comments
+public abstract class AsyncEnumerableResult<T> : ClientResult, IAsyncEnumerable<T>, IDisposable
+    where T : IPersistableModel<T>


Another question is whether or not we want to add this type constraint. The benefit is that it could simplify implementations by allowing them to use ModelReaderWriter to deserialize values coming across the wire, rather than requiring them to use other ways to pass in bespoke deserialization routines. A drawback is that it limits the use of the type to models that implement the interface -- if we wanted to enable scenarios like pages of primitives, that could be a problem.

Since we did not constrain ClientResult<T>, maybe we should keep not constraining. Though in retrospect, maybe we should have.

The libraries haven't GA'ed yet, so we could still constrain it if we feel strongly about it.

Ah, but not if we want Azure.Core Pageable<T> to be able to inherit from it. Alas! I will remove it.

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

KrzysztofCwalina · 2024-04-16T15:29:09Z

sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs

@@ -40,6 +48,14 @@ public partial class ClientResult<T> : System.ClientModel.ClientResult
        public virtual T Value { get { throw null; } }
        public static implicit operator T (System.ClientModel.ClientResult<T> result) { throw null; }
    }
+    public abstract partial class EnumerableClientResult<T> : System.ClientModel.ClientResult, System.Collections.Generic.IEnumerable<T>, System.Collections.IEnumerable, System.IDisposable where T : System.ClientModel.Primitives.IPersistableModel<T>


The names don't seem symetrical. What about the following:

AsyncEnumerableResult EnumerableResult //or ClientResults ClientResultsAsync

I'm actually not wild about ClientResults because I think it complicates what we're defining a "result" to be (although I am open to debate here).

To take a concrete example, Pageable<T> is a collection of T's. Does the "result" mean the T or with the response? In a simple ClientResult<T> case there's a 1-1 relationship between them. But ClientResult itself just exposes GetRawResponse so it seems like result maps more closely to the response than the T. Given that, it feels strange to me to make the "result" plural when the T is the plural thing and there might only ever be one response.

Let me think.

Looks like 3.5.2 in the FDG recommends using Collection in the name of types that derive from IEnumerable<T> ... what about ClientResultCollection<T> and AsyncClientResultCollection<T>? Those are getting long, though, and don't really address my question about whether it is a collection of results, but I think I'm willing to drop argument since we defined ClientResult in the refdocs as "the result of a cloud service operation." A sequence of such results could easily be a result collection. Is it too long?

KrzysztofCwalina · 2024-04-16T15:32:47Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+
+#pragma warning disable CS1591 // public XML comments
+public abstract class AsyncEnumerableResult<T> : ClientResult, IAsyncEnumerable<T>, IDisposable
+    where T : IPersistableModel<T>


Since we did not constrain ClientResult<T>, maybe we should keep not constraining. Though in retrospect, maybe we should have.

KrzysztofCwalina · 2024-04-16T15:33:52Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+public abstract class AsyncEnumerableResult<T> : ClientResult, IAsyncEnumerable<T>, IAsyncDisposable
+    where T : IPersistableModel<T>
+{
+    protected internal AsyncEnumerableResult(PipelineResponse response) : base(response)


can the response be updated/changed?

I don't think it can. Is there a scenario where we would want that?
(Not sure I understand the implications of your question.)

The scenario is using this type to expose pageable REST APIs. In such scenarios there are multiple responses representing all items in the collection/pageable

Would you want to do that by changing the value returned from the response? Why not return a collection of PipelineResponses instead? (i.e. not inherit from ClientResult)

Addressed. I see that we already do that in Azure.Core Operation implementations such as KeyVaultBackupOperation and FuzzySearchBatchOperation.

KrzysztofCwalina · 2024-04-16T15:39:34Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+
+    public abstract IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default);
+
+    protected abstract ValueTask DisposeAsyncCore();


FDGs say to make it virtual

Removed per discussion about moving disposal of resources into the enumerator.

KrzysztofCwalina · 2024-04-16T15:39:36Z

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs

+
+    protected abstract ValueTask DisposeAsyncCore();
+
+    protected abstract void Dispose(bool disposing);


what is this for? i.e. who call it? BTW, FDGs (that Jeremy wrote) say to implement both IAsyncDisposabel and IDisposable

Removed per discussion about moving disposal of resources into the enumerator.

…umresult

…be replaced in a polling paradigm

azure-sdk · 2024-04-20T00:32:23Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

System.ClientModel

…umresult

annelo-msft · 2024-05-10T19:13:35Z

Closing in favor of #43840

Add async and sync enumerable client results

54f822b

github-actions bot added the Azure.Core label Apr 12, 2024

annelo-msft commented Apr 12, 2024

View reviewed changes

annelo-msft mentioned this pull request Apr 12, 2024

ClientModel: investigation for StreamingClientResult<T> #42873

Closed

Implement IAsyncDisposable

d239e17

annelo-msft requested review from jsquire, tg-msft, christothes, KrzysztofCwalina, trrwilson and JoshLove-msft April 15, 2024 17:31

jsquire reviewed Apr 15, 2024

View reviewed changes

sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs Outdated Show resolved Hide resolved

KrzysztofCwalina reviewed Apr 16, 2024

View reviewed changes

annelo-msft added 3 commits April 19, 2024 15:43

Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…

115295a

…umresult

Remove constrant and disposable; update ClientResult so response can …

3b12f1f

…be replaced in a polling paradigm

rename and upate tests

dbf18ad

annelo-msft mentioned this pull request Apr 30, 2024

Narrower update to StreamingClientResult/SSE joseharriaga/openai-in-typespec#68

Closed

annelo-msft added 2 commits April 30, 2024 17:13

Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…

a798fd9

…umresult

Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…

72951b9

…umresult

annelo-msft closed this May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ClientModel: Add async and sync enumerable client results #43392

ClientModel: Add async and sync enumerable client results #43392

annelo-msft commented Apr 12, 2024 •

edited

Loading

annelo-msft Apr 12, 2024 •

edited

Loading

annelo-msft Apr 12, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 16, 2024

annelo-msft Apr 19, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 19, 2024

annelo-msft Apr 20, 2024

KrzysztofCwalina Apr 16, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 16, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 16, 2024

annelo-msft Apr 20, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 20, 2024

KrzysztofCwalina Apr 16, 2024

annelo-msft Apr 20, 2024

azure-sdk commented Apr 20, 2024

annelo-msft commented May 10, 2024


		public abstract IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default);

		protected abstract ValueTask DisposeAsyncCore();


		protected abstract ValueTask DisposeAsyncCore();

		protected abstract void Dispose(bool disposing);

ClientModel: Add async and sync enumerable client results #43392

ClientModel: Add async and sync enumerable client results #43392

Conversation

annelo-msft commented Apr 12, 2024 • edited Loading

annelo-msft Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Apr 20, 2024

annelo-msft commented May 10, 2024

annelo-msft commented Apr 12, 2024 •

edited

Loading

annelo-msft Apr 12, 2024 •

edited

Loading