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

Implement cancellation by utilizing native URLSession functionality #251

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

nezhyborets
Copy link
Collaborator

@nezhyborets nezhyborets commented Jan 31, 2025

What

Implement cancellation.
Add CancellableRequest for closure based API.
Refactor Concurrency and Combine APIs to use their native counterparts of URLSession

Why

The functionality is requested here #70
And generally it's good to give ability to cancel any long running tasks

Affected Areas

This introduces breaking changes. I'll go through big changes and reasons.

Remove OpenAIProtocol+Async and OpenAIProtocol+Combine

The core thought of this PR is that we should utilise system abilities. Consider the use case - request is make in SwiftUI's View.task, thus if call to model is async, it would be implicitly cancelled when view is closed. On the other side of this call is URLSession that has async API and, if used directly, the cancellation would just happen without needing to do anything. Thus, I wanted it to be this way. So now when you call async APIs of OpenAI, it would actually call async APIs on URLSession. This way all the Swift Concurrency cancellations would work automatically. And for Combine too, as OpenAI's combine based methods will now call dataTaskPublisher on URLSession

With the new implementation, async and combine methods are not just extension over public OpenAIProtocol API, but actually methods with a different (non compatible) implementation. Thus, methods described there should be part of an actual interface. So, they moved into their own interfaces OpenAIAsync and OpenAICombine, which OpenAI now conforms to.

Question: do we really need OpenAIProtocol?

I think we can just go with OpenAI. Even remove new OpenAIAsync and OpenAICombine. So there will only be OpenAI, which will have all the methods: closure based, async and Combine. If a user of the library wants to mock OpenAI in their code they can create any protocol and extend OpenAI to conform to it. There is no real benefit of providing such a protocol for users of the lib. Except that doc looks cleaner, but... OpenAI is an interface itself and it seems to be ok to document it's methods

Another argument to go with just a class is how Apple provides us functionality in their frameworks. Like URLSession it's just a class. I've also looked at Apollo as an example, as it seems like an SDK of the same nature. They seem to use ApolloClient class, but they also have ApolloClientProtocol. Main documentation is in ApolloClientProtocol, ApolloClient documents only what's not documented in ApolloClientProtocol. I still don't see much reason for having such a protocol, and I may be missing out something. Please let me know.

But of course there is a problem that if we remove OpenAIProtocol it would break compatibility and people would have to update their existing calls. 😪

CancellableRequest and discardableResult

Closure based API now returns CancellableRequest. It's identical to a lot of performRequest APIs, so I don't see an issue with it (let me know if there is any). Maybe there is a better naming.

The bigger question is about discardableResult. In order to not make a lot of warnings for existing calls to closure based API, I've added discardableResult to all closure-based methods. But those methods are core of our API and it would be better to not base their ideal design solely on backwards compatibility. Thus question: would ideal closure based OpenAI API have discardable result? For example, Apollo doesn't have it for their requests which return Cancellable.

Internal vs private

In OpenAI.swift these fields were private, now internal, so that OpenAI+OpenAIAsync and OpenAI+OpenAICombine are able to use session and streamingSessions.

let session: URLSessionProtocol
var streamingSessions = ArrayWithThreadSafety<NSObject>()

Let me know if you think that's a bad idea.

"make" prefix in method names

See OpenAI+MakeRequest. On one hand - it's verbose and introduces kind of duplication. On the other hand it makes things more explicit and names such as func modelRequest(...) would not confuse into thinking that this method would actually do some request modelling. I'm not sure about the naming, can remove all the make prefixes there if you think it's better.

Thanks in advance for you feedback

By utilizing native URLSession functionality
@nezhyborets nezhyborets linked an issue Jan 31, 2025 that may be closed by this pull request
@nezhyborets
Copy link
Collaborator Author

nezhyborets commented Feb 1, 2025

Tried to make this PR non-breaking by putting all the needed methods back to OpenAIProcotol using conditional declaration of OpenAIProtocol. Should work fine for anyone who has @available(iOS 13.0, tvOS 13.0, macOS 10.15, watchOS 6.0, *). For those on older systems they would have to use OpenAIClosureBased or OpenAI, as OpenAIProtocol won't be available for them, because I haven't found how to use @available or other version check to do conditional declaration over version number.

Copy link

sonarqubecloud bot commented Feb 4, 2025

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 this pull request may close these issues.

How to cancel closure openAI.chatsStream(query: query)
1 participant