-
Notifications
You must be signed in to change notification settings - Fork 386
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
base: main
Are you sure you want to change the base?
Implement cancellation by utilizing native URLSession functionality #251
Conversation
By utilizing native URLSession functionality
Backdeploy using manual wrapper
Tried to make this PR non-breaking by putting all the needed methods back to |
…ery-query # Conflicts: # Sources/OpenAI/Private/StreamingSession.swift
Quality Gate passedIssues Measures |
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 isURLSession
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 ofOpenAI
, it would actually call async APIs onURLSession
. This way all the Swift Concurrency cancellations would work automatically. And for Combine too, as OpenAI's combine based methods will now calldataTaskPublisher
onURLSession
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 interfacesOpenAIAsync
andOpenAICombine
, whichOpenAI
now conforms to.Question: do we really need OpenAIProtocol?
I think we can just go with
OpenAI
. Even remove newOpenAIAsync
andOpenAICombine
. So there will only beOpenAI
, which will have all the methods: closure based, async and Combine. If a user of the library wants to mockOpenAI
in their code they can create any protocol and extendOpenAI
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 methodsAnother 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 inApolloClientProtocol
,ApolloClient
documents only what's not documented inApolloClientProtocol
. 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 ofperformRequest
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 addeddiscardableResult
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 returnCancellable
.Internal vs private
In
OpenAI.swift
these fields were private, now internal, so thatOpenAI+OpenAIAsync
andOpenAI+OpenAICombine
are able to use session and streamingSessions.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 asfunc 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 themake
prefixes there if you think it's better.Thanks in advance for you feedback