-
Notifications
You must be signed in to change notification settings - Fork 62
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
Comments
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 |
(This is as of 2.0.0-beta.3) |
Sorry, I forgot to mention that this overload does not show in intellisense. See #48 |
Hopefully this will happen soon. It will be easy to use with strong type support. Is the |
It's not very idiomatic C# code to bury a Also, it's worth pointing out that cast operators, both |
@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. |
@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 |
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. |
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 |
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. |
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? |
@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. |
@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! |
BTW, one of the commits in the PR is fixing tests as they broke because of the overload ambiguities these change introduced. |
Simple, really:
We should be able to pass
CancellationToken
s to any methods that make API calls.The text was updated successfully, but these errors were encountered: