Borrow instead of moving CreateChatCompletionRequest

[amosjyng]

I wish to retain control over the CreateChatCompletionRequest for logging and caching purposes after the call is made. Since the create function doesn't actually need to own any of the data in the request, it can simply borrow it instead.

This PR would unfortunately break the current API, but I'd like to bring it to your attention as a consideration for the next breaking release.

[64bit]

Thank you for the PR!

While its a small change, it creates an inconsistent developer experience with remaining APIs that library exposes.

Here are few options that I'd suggest:

1. Make all other APIs to accept a reference (you're also welcome to create an issue to track it)
2. Clone CreateChatCompletionRequest to retain control (Clone trait is already derived on all types)

You're welcome to pick any choice from above.

[amosjyng]

Sorry for the delay, I was trying to meet a deadline.

This looks to be a little harder than expected. If I do it for, say, this function in Files:

    pub async fn create(&self, request: &CreateFileRequest) -> Result<OpenAIFile, OpenAIError> {
        self.client.post_form("/files", request).await
    }

I get the error

error[E0277]: the trait bound `Form: async_convert::TryFrom<&types::file::CreateFileRequest>` is not satisfied
   --> async-openai\src\file.rs:26:41
    |
26  |         self.client.post_form("/files", request).await
    |                     ---------           ^^^^^^^ the trait `async_convert::TryFrom<&types::file::CreateFileRequest>` is not implemented for `Form`
    |                     |
    |                     required by a bound introduced by this call 
    |

If I then try to change that TryFrom to take in a reference instead:

#[async_convert::async_trait]
impl<'a> async_convert::TryFrom<&'a CreateFileRequest> for reqwest::multipart::Form {
    type Error = OpenAIError;

    async fn try_from(request: &'a CreateFileRequest) -> Result<Self, Self::Error> {
        let file_part = create_file_part(request.file.source).await?;
        let form = reqwest::multipart::Form::new()
            .part("file", file_part)
            .text("purpose", request.purpose);
        Ok(form)
    }
}

then this error crops up:

error[E0276]: impl has stricter requirements than trait
   --> async-openai\src\types\impls.rs:753:6
    |
753 | impl<'a> async_convert::TryFrom<&'a CreateFileRequest> for r... 
    |      ^^ impl has extra requirement `'a: 'async_trait`

This appears to be a known issue, and the suggested solution is hard to apply here because we can't edit the signature of the async_convert::TryFrom trait to support the 'async_trait lifetime.

I've uploaded my edits thus far, but I don't know how feasible it is to do this for the async traits. What do you think about doing this just for the requests that don't require a TryFrom?

I'm ok with simply closing this PR too and maintaining my own fork. I just wanted to run the idea by you, that's all.

[64bit]

Sorry for the delay, I was trying to meet a deadline.

No worries, thank you for taking time to follow up with your findings!

This appears to be a known issue, and the dtolnay/async-trait#8 (comment) is hard to apply here because we can't edit the signature of the async_convert::TryFrom trait to support the 'async_trait lifetime.

I've uploaded my edits thus far, but I don't know how feasible it is to do this for the async traits. What do you think about doing this just for the requests that don't require a TryFrom?

Thanks for digging deeper and venturing into the unknowns - based on your findings - let say we do this for requests that don't require a TryFrom yet (required mostly for file uploads) and extrapolating that to future chat completion request which takes file uploads too - it would have similar problem and wont be future proof. Right?

Alternatively, if editing signature of async_convert::TryFrom is the only blocker and you'd like to explore further - async-convert is very small library with MIT license, may be copy that (with honoring the license requirements) file into here and modify as needed? Or make upstream contributions to it and use here?

[amosjyng]

let say we do this for requests that don't require a TryFrom yet (required mostly for file uploads) and extrapolating that to future chat completion request which takes file uploads too - it would have similar problem and wont be future proof. Right?

Good point!

Alternatively, if editing signature of async_convert::TryFrom is the only blocker and you'd like to explore further - async-convert is very small library with MIT license, may be copy that (with honoring the license requirements) file into here and modify as needed? Or make upstream contributions to it and use here?

Sounds good. I've gotten the code compiling and tests passing. Some Clone's are still needed unless we change the structs to take in a Cow string for reqwest. I had to change the test in forks/async-openai/async-openai/tests/boxed_future.rs because the Babbage model appears completely incapable of following any patterns anymore. Let me know what you think!