Skip to content

Conversation

@bilqisium
Copy link
Contributor

@bilqisium bilqisium commented Jun 3, 2025

This PR add progress report for OpenAI's transcription request, at least for the file uploading part.

I reused the AIProxyCertificatePinningDelegate to hook into the delegate method reporting progress.

Unfortunately, since Swift doesn't allow optional parameters in protocols, the progressCallback parameter is required at call site (but we can pass nil). This can be seen as a breaking change.

Also, it properly works only for OpenAIProxiedService, since the OpenAIDirectService architecture for having its URLSession doesn't allow injecting a delegate without significantly changing the internals.

This means that OpenAIDirectService never updates this callback, but it has to have it because of its conformance to
the OpenAIService protocol.

The transcription part wasn't unit tested, so I didn't add any tests either.

I tried to adhere to the style of the library.

If you have ideas on how to improve this PR or want some fixes, I'll happily implement them.

@lzell
Copy link
Owner

lzell commented Jun 3, 2025

@bilqisium this is cool!

I think we can make this a nonbreaking change. You keep exactly what you have now, except you add a proto extension to OpenAIService.swift that looks like this:

extension OpenAIService {
    public func createTranscriptionRequest(
        body: OpenAICreateTranscriptionRequestBody
    ) async throws -> OpenAICreateTranscriptionResponseBody {
        return try await self.createTranscriptionRequest(body: body, progressCallback: nil)
   }
}

That should make existing callers happy.

Also, please add a check to the direct case that looks something like this:

    public func createTranscriptionRequest(snip) {
       if progressCallback != nil {
                 logIf(.error)?.error("progressCallback for transcription is not implemented")
       }
   }

@lzell lzell self-assigned this Jun 3, 2025

private var progressCallback: ((Double) -> Void)?

public func setProgressCallback(_ callback: @escaping (Double) -> Void) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any advantage to keeping these accessors versus changing the ACL to public on:
public var progressCallback: ((Double) -> Void)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No advantage indeed, it's much better to expose it indeed. 7982355

@bilqisium
Copy link
Contributor Author

@bilqisium this is cool!

I think we can make this a nonbreaking change. You keep exactly what you have now, except you add a proto extension to OpenAIService.swift that looks like this:

extension OpenAIService {
    public func createTranscriptionRequest(
        body: OpenAICreateTranscriptionRequestBody
    ) async throws -> OpenAICreateTranscriptionResponseBody {
        return try await self.createTranscriptionRequest(body: body, progressCallback: nil)
   }
}

That should make existing callers happy.

Also, please add a check to the direct case that looks something like this:

    public func createTranscriptionRequest(snip) {
       if progressCallback != nil {
                 logIf(.error)?.error("progressCallback for transcription is not implemented")
       }
   }

Thank you @lzell, I've made the requested changes respectively in a030d78 and edb0f14.

@lzell lzell merged commit 2745f96 into lzell:main Jun 4, 2025
@lzell
Copy link
Owner

lzell commented Jun 4, 2025

Available in 0.106.0: https://github.com/lzell/AIProxySwift/releases/tag/0.106.0

Thank you @bilqisium !

_ progressCallback: ((Double) -> Void)? = nil
) async throws -> (Data, HTTPURLResponse) {
if let progressCallback {
(session.delegate as? AIProxyCertificatePinningDelegate)?.setProgressCallback(progressCallback)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we can now ditch these accessors and use (session.delegate as? AIProxyCertificatePinningDelegate)?.progressCallback = progressCallback since we exposed the member. I'll touch that up on main

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.

2 participants