async-openai icon indicating copy to clipboard operation
async-openai copied to clipboard

Enable polymorphism / dynamic dispatch for AzureConfig and OpenAIConfig

Open deipkumar opened this issue 1 year ago • 5 comments

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 avatar Sep 13 '23 00:09 deipkumar

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?

deipkumar avatar Sep 13 '23 00:09 deipkumar

Please use fork for personal branches and contributions

64bit avatar Sep 13 '23 19:09 64bit

@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

ifsheldon avatar Oct 18 '23 09:10 ifsheldon

I do not recall :)

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

64bit avatar Oct 21 '23 00:10 64bit

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 (https://github.com/64bit/async-openai/pull/125#issuecomment-1798400589).

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.

schneiderfelipe avatar Nov 07 '23 12:11 schneiderfelipe