Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CancellationToken support #35

Closed
kescherCode opened this issue Jun 10, 2024 · 14 comments · Fixed by #53
Closed

CancellationToken support #35

kescherCode opened this issue Jun 10, 2024 · 14 comments · Fixed by #53

Comments

@kescherCode
Copy link

Simple, really:

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

@KrzysztofCwalina
Copy link
Collaborator

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
Copy link

godefroi commented Jun 11, 2024

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
Copy link
Collaborator

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

See #48

@JadynWong
Copy link

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
Copy link

godefroi commented Jun 12, 2024

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
Copy link
Collaborator

@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
Copy link

@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
Copy link

dspear commented Jun 12, 2024

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
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants