CancellationToken support

[kescherCode]

Simple, really:

We should be able to pass CancellationTokens to any methods that make API calls.

[KrzysztofCwalina]

There are overloads that take RequestOptions, and RequestOptions has a CancellationToken property. For now, it might be difficult to use some of the overloads, as they don't take "model types", and instead they take BinaryContent (untyped request content). We will be adding cast operators from models to BinaryContent, at which point calling these RequestOptions overloads will become easy

[godefroi]

ChatClient for example has two overloads of CompleteChatAsync, both of which take an instance of ChatCompletionOptions, but ChatCompletionOptions has no property CancellationToken.

ChatClient has no overload of CompleteChatAsync which takes an instance of RequestOptions.

(This is as of 2.0.0-beta.3)

[KrzysztofCwalina]

Sorry, I forgot to mention that this overload does not show in intellisense.

See https://github.com/openai/openai-dotnet/pull/48

[JadynWong]

We will be adding cast operators from models to BinaryContent, at which point calling these RequestOptions overloads will become easy

Hopefully this will happen soon. It will be easy to use with strong type support. Sometimes developers may want to be able to modify request behaviors, such as CancellationToken, ClientErrorBehaviors, and so on.

---

Is the cast operator from model to binary content is called publicly? We would like to get the actual content of the request so that it can be logged for tracking.

[godefroi]

It's not very idiomatic C# code to bury a CancellationToken parameter in a property of another parameter. Generally (and there's even a code style warning, CA1068) the CancellationToken parameter is provided as the last parameter of an async method, often optional.

Also, it's worth pointing out that cast operators, both implicit and explicit suffer from poor discoverability. It's not obvious to anyone not reading the library's source code that the cast operator exists, which leads to frustration, especially when the method that someone really wants to use is hidden through EditorBrowseableAttribute.

[KrzysztofCwalina]

@godefroi, I agree with everything you said. We are trying to balance between the objectives you spelled out and some issues related to how protocol methods (taking RequestOptions) and model-based overloads (which would be taking cancellation tokens) evolve. We want to be able to start with protocol methods and make the RequestOptions parameter optional. Then we want to be able to add the model-based overload, and also make the CancellationToken (CT) parameter optional. This works if there is an input model. But if there is only output model, the overloads become ambiguous and we then need to make the CT parameter required.

Anyway, we are still debating what's the best tradeoff here, and your (and others) input/upvotes will help us in the decision.

BTW, I also don't have a good understanding for how important cancellations are for .NET developers. I searched public repos for usage of CTs and the results were discouraging. Most calls passing CTs to various APIs were either passing CT.None, null, CTs that can never be cancelled, CTs that only get cancelled when the process shuts down, etc. I think I found just a handful mildly reasonable usages of CTs. Possibly public GitHub sources are not very representative.

[joakimriedel]

@KrzysztofCwalina jumping in here from the sideline but I just migrated a semi large project from Azure.AI.OpenAI 1.0.0-beta to the new 2.0.0-beta built on top of this repo, and the three major hurdles was the lack of CancellationToken support on async methods, no legacy completions endpoint for our gpt-3 based fine-tunes and no ModelFactory which I found useful for unit testing.

The cancellation support is a first class citizen in our project for various reasons, since generating output tokens is a lengthy task which frequently gets cancelled by user navigation, stop buttons or where we use generation preemptive to reduce latency in UI. Technically this is implemented by injecting CancellationToken in an Action method and passing it forward and down the code to be able to cancel the HttpRequest from a Typescript client calling reader.cancel(); on the reader from a fetch call. If there are other solutions to this than using CancellationToken, I'm all ears.

[dspear]

I am doing the same as @joakimriedel . The lack of CancellationToken is the biggest problem we have currently. I'm looking at doing some sort of UI-based simulation of cancellation if I can't really cancel the operation, or attempting to inject a cancellation token in a Pipeline policy. All much harder than just an optional CancellationToken parameter to existing calls.

[godefroi]

Given that many (most?) users of this library will be paying per-token, cancellation could be seen as a financial requirement. Cancellation, in the general case, might be rare (most people don't want to cancel, say, an asynchronous disk write that only lasts a few microseconds, or even an HTTP request that lasts milliseconds, when there's no money involved), but can be significant when you're generating large numbers of tokens over a potentially long time and paying for every single one.

Your "protocol methods" should also take CancellationToken parameters. The cancellation token should NOT be a property of RequestOptions.

[KrzysztofCwalina]

We could easily add CT to all the non-protocol methods. It would just mean higher change of source breaking changes (not binary breaks), as we evolve the APIs. As I mentioned above, such source breaking changes would occur rarely, only in methods that don't have any request content, and when a convenience (model-based) overload is added after a protocol overload is released, which we do sometimes. Maybe it's a good compromise.

[godefroi]

Given the fact that the 2.0 API is still in beta, I would think that now is an ideal opportunity to make any changes that would be breaking?

[KrzysztofCwalina]

@godefroi, it's not about breaking changes now. It's about future source breaks as we evolve APIs. The breaks are because overloads might become ambiguous.

[godefroi]

@KrzysztofCwalina Ah, ok, I've misunderstood. Looks like the linked PR resolves all the most common cases. It would certainly take care of the cases I've encountered. Thanks!

[KrzysztofCwalina]

BTW, one of the commits in the PR is fixing tests as they broke because of the overload ambiguities these change introduced.