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

Enable polymorphism / dynamic dispatch for AzureConfig and OpenAIConfig #106

Open
deipkumar opened this issue Sep 13, 2023 · 5 comments
Open
Labels
enhancement New feature or request

Comments

@deipkumar
Copy link

deipkumar commented Sep 13, 2023

Would like to enable something like this:

fn use_client<C: Config>(client: Box<dyn CommonClientTrait<ConfigType = C>>) {
    // You can now call any method defined in the CommonClientTrait on the client
    let models = client.models();
    // ...
}

fn main() {
    let openai_config = OpenAIConfig::default();
    let openai_client = Client::with_config(openai_config);
    use_client(Box::new(openai_client));

    let azure_config = AzureConfig::default();
    let azure_client = Client::with_config(azure_config);
    use_client(Box::new(azure_client));
}
@deipkumar
Copy link
Author

deipkumar commented Sep 13, 2023

Also, I can't push branches:

(base) ➜  async-openai git:(deip/common-config-trait) git push --set-upstream origin deip/common-config-trait
ERROR: Permission to 64bit/async-openai.git denied to deipkumar.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Do I need to be added as a contributor?

@64bit
Copy link
Owner

64bit commented Sep 13, 2023

Please use fork for personal branches and contributions

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

ifsheldon commented Oct 18, 2023

@64bit Why did you want Config: Clone in the first place? I looked through the codebase, it seems config.clone is never used. So, after removing the trait bound Clone in Config, you can just use a box like

pub struct Client {
    ...
    config: Box<dyn Config>,
    ...
}

No need to propagate the generics everywhere

@64bit
Copy link
Owner

64bit commented Oct 21, 2023

I do not recall :)

Your idea of Box-ing it is actually good. Not sure what the rippling effects would be but worth exploring.

@schneiderfelipe
Copy link
Contributor

schneiderfelipe commented Nov 7, 2023

Having a trait with associated type would allow the idea to work using only generics (no need for dynamic dispatch):

// Only Config seems to be polymorphic here.
// In any case, one can always have client: impl GenericClient<Config=C> below (see my comment on #125 below)
fn use_client<C: GenericConfig>(client: Client<C>) {
    let models = client.models();
    // ...
}

fn main() {
    let openai_config = OpenAIConfig::default();
    let openai_client = Client::with_config(openai_config);
    // No need for dynamic dispatch: the types in your problem are known at compile time. No boxing means zero cost dispatch at run time.
    use_client(openai_client);

    let azure_config = AzureConfig::default();
    let azure_client = Client::with_config(azure_config);
    use_client(azure_client);
}

Related to a comment of mine (#125 (comment)).

Keep in mind that the suggestion in the comment does allow dynamic dispatch using a simple design widely used in the Rust world (Iterators use it). It's just that dynamic dispatch is not required for solving the present issue.

@64bit 64bit added the upcoming release This is going to be part of one of the next few upcoming releases label Jan 26, 2024
@64bit 64bit removed the upcoming release This is going to be part of one of the next few upcoming releases label May 4, 2024
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

No branches or pull requests

4 participants