64bit / async-openai

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

feat: Move most of the `Config` impl into the trait. #97

Closed lukesneeringer closed 1 year ago

lukesneeringer commented 1 year ago

This PR moves the implementations of headers, url, and query from the OpenAIConfig struct to the Config trait, so other config implementations can override just the attribute methods without having to re-implement the common material (and take a direct dependency on reqwest in the process).

Note that this PR also adds org_id as a trait method, because headers needs it. For backwards-compatibility, it implements a default of empty string rather than being required.

64bit commented 1 year ago

Hi @lukesneeringer

Thank you for PR. The changes in this PR are probably inconsequential to library users, however it changes the semantics of Config trait because org id has no meaning for Azure (afaik).

If you do want to pursue in this direction - the functions I see that are repeated for both OpenAIConfig and AzureConfig are fn api_key, fn api_base, fn with_api_key, and fn with_api_base - may be these four can have default implmentation?

I'd rather pick correct semantics over code duplication. Please let me know if you have toughts or ideas on this.

lukesneeringer commented 1 year ago

Hmm. I admit to not being familiar with the Azure specifics.

I think api_key and api_base are actually the ones that can't have a default implementation, because traits themselves can't have properties.

I'm also having a hard time understanding why, e.g. headers, isn't implemented for both. It's required by the trait, isn't it? And I would expect the implementations to be the same or at least very similar. (I'd also argue that a "sensible default" is reasonable over requiring a re-implementation; nothing stops you from overriding the trait's function if you need to.)

I actually agree with you on the org_id part. My rationale there was that I felt that "not forcing a breaking change" was more important. The concept of org_id could ostensibly be removed from a generic headers implementation.

64bit commented 1 year ago

I'm same page as you regarding api_key and api_base.

Regarding headers, the current implementation is separate because of differences in OpenAI and Azure APIs.

I wish Azure would maintain parity with OpenAI API and we would not have needed everything that exists in config.rs.