search

64bit / async-openai

Rust library for OpenAI
https://docs.rs/async-openai
MIT License
1.09k stars 161 forks source link

Update Audio APIs from updated spec #202

Closed emk closed 5 months ago

emk commented 5 months ago

This PR adds support for AudioResponseFormat::VerboseJson and TimestampGranularity, including updated example code. These were defined as types before, but not fully implemented.

Implements 64bit/async-openai#201.

Please let me know if I can improve this PR! And thank you for a very helpful crate.

64bit commented 5 months ago

Hello,

Thank you for the issue and the implementation! I'll leave review in a separate comment

64bit commented 5 months ago

It seems they've updated spec for this.

While logistically this implementation looks to fill the gap - however it doesn't adhere to the new spec - all doc comments, type names must match the spec.

A quick look on spec suggest there are multiple new types (CreateTranscriptionResponseJson, CreateTranscriptionResponseJsonVerbose, etc.) and this PR may require a bit more work to incorporate them all.

emk commented 5 months ago

While logistically this implementation looks to fill the gap - however it doesn't adhere to the new spec - all doc comments, type names must match the spec.

Ah, great! Do you have a tool to auto-generate this code from the spec, or are you synchronizing it by hand?

I was working from these documents here originally, because they contained quite a bit of information that was missing elsewhere.

64bit commented 5 months ago

Unfortunately no tool to auto generate and its written by hand, I have full context about it here https://github.com/64bit/async-openai/issues/163

64bit commented 5 months ago

I was working from these documents here originally, because they contained quite a bit of information that was missing elsewhere.

They also seem to generate website from the spec - and so I go with spec as source of truth.

emk commented 5 months ago

Ah, great! I'll try to get to this either during the week or the coming weekend.

Supporting the return types CreateTranscriptionResponseJson and CreateTranscriptionResponseJsonVerbose is going to be a bit tricky, because the API chooses between them based on the function arguments, and the Rust version can only return a single type. We have two choices here:

  1. Always return CreateTranscriptionResponseJsonVerbose, but make the fields which aren't in CreateTranscriptionResponseJson optional.
  2. Create a new return type, enum CreateTranscriptionResponse { JsonVerbose(CreateTranscriptionResponseJsonVerbose), Json(CreateTranscriptionResponseJson) }, and rely on serde's smart decoding to pick the first enum option that matches.

I think (2) is a bit more future-proof, but it would require introducing a new type CreateTranscriptionResponse which isn't explicitly present in the official API spec at the moment. I'd be inclined to go with (2), but please let me know if you'd prefer a different choice!

64bit commented 5 months ago

Thank you for the detailed choices. (2) is a more reasonable option. However:

because the API chooses between them based on the function arguments

For this a pattern is already used in the crate: to have separate functions to return separate types based on different value to a parameter to same request object.

For example, please take a look at Chat::create and Chat::create_stream, and a more recent addition of Embedding::create and Embedding::create_base64 . Same pattern can be applied here too.

emk commented 5 months ago

I've gotten caught up with emergency work stuff this weekend, and haven't had a chance to redo this PR. It's still very high on my list, and I hope to get to it ASAP!

emk commented 5 months ago

OK, here's an improved version! I focused on two things:

  1. Making the API as consistent as possible with the spec, and
  2. After satisfying (1), trying to make as few breaking changes as possible.

Here are the specific changes I made:

I'm pretty sure this could be improved in various ways by making different tradeoffs, but I hope it's closer. Thank you for your feedback and for a very helpful library!

64bit commented 5 months ago

I've gotten caught up with emergency work stuff this weekend, and haven't had a chance to redo this PR. It's still very high on my list, and I hope to get to it ASAP!

No worries, unlike work, here, there's is no pressure or deadline or emergency. Thank you for taking time to contribute!

emk commented 5 months ago

Please do feel free to edit the doc comments! I was trying to stick extremely strictly to OpenAPI spec's comments, but it would very likely help users to add more information to the transcribe_raw function in particular.