Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Deserialize to OpenAIConfig and AzureConfig #107

Closed
ifsheldon opened this issue Sep 13, 2023 · 3 comments · Fixed by #108
Closed

Add Deserialize to OpenAIConfig and AzureConfig #107

ifsheldon opened this issue Sep 13, 2023 · 3 comments · Fixed by #108
Labels
enhancement New feature or request

Comments

@ifsheldon
Copy link
Contributor

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
Copy link
Owner

64bit commented Sep 13, 2023

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

Contributions are welcome!

@64bit 64bit added the enhancement New feature or request label Sep 13, 2023
@ifsheldon
Copy link
Contributor Author

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
Copy link
Owner

64bit commented Sep 14, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants