Add `Deserialize` to `OpenAIConfig` and `AzureConfig`

64bit / async-openai

Rust library for OpenAI

https://docs.rs/async-openai

MIT License

1.17k stars 178 forks source link

Add `Deserialize` to `OpenAIConfig` and `AzureConfig` #107

Closed ifsheldon closed 1 year ago

ifsheldon commented 1 year ago

Please add Deserialize derive to OpenAIConfig and AzureConfig. I store my Azure configs in a json file for testing. Without Deserialize, I need to copy a same struct and add Deserialize for it and convert it to AzureConfig, which is unnecessarily verbose.

64bit commented 1 year ago

It sounds like it would have been easier if you sent PR to this repo instead.

Contributions are welcome!

ifsheldon commented 1 year ago

I was about to, but I didn't want to make a PR that adds a feature without proper discussion, which is rude from my view.

There are a couple of issues:

Should we add Serialize as well? It just a convenient derive, but should we?
As far as I know, OpenAI requires at least an API key, so the other fields in OpenAIConfig are optional. To make them optional in deserialization, we need to change them to Option<String>, so this change leads to broader impacts I guess

I will make a PR, though.

64bit commented 1 year ago

Thank you for elaborating.

I think there is no downside to adding Serialize as well - at least you'll have the machinery to read from and write to config files without introducing any breaking change like making fields Optional - just that all the fields can be stored in config file to later Deserilaize.

If the semantics of required-fields and optional-fields are required in config file then you're welcome to send follow up PR - we'll just have to do a version bump for breaking change.