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

Custom Client support #831

Closed
wants to merge 14 commits into from
Closed

Conversation

olgavrou
Copy link
Contributor

@olgavrou olgavrou commented Dec 1, 2023

Why are these changes needed?

This PR adds support for custom client calls to be made. The idea is that the user can specify their own custom client class and load it into the configuration (e.g. for loading local models). As long as they adhere to the Client class's interface and response protocol then everything should work fine with only this addition to the config (full usage shown in the unit test):

from custom_file import CustomClient

config_list = [
    {
        "model": "local_model_name",
        "custom_client" = CustomClient(),
        "device": "cuda",
        "params": {
            "max_length": 1000,
            "temperature": 1.1,
            [other params that will be consumed in the Custom Client so user has full control over what is set and what is consumed]
        }
    }
]

The PR looks big but it is mostly moving code around. If this is something that we want then I can go ahead and add docs and a notebook

Ideally the client interface would not live under a directory called oai and the client wrapper wouldn't be called OpenAIWrapper but something more generic, but that can be examined at a later date

Related issue number

Checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2023

Codecov Report

Attention: 117 lines in your changes are missing coverage. Please review.

Comparison is base (a0288e0) 26.54% compared to head (756e7b8) 38.12%.

Files Patch % Lines
autogen/oai/client.py 27.04% 112 Missing and 4 partials ⚠️
autogen/oai/openai_utils.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #831       +/-   ##
===========================================
+ Coverage   26.54%   38.12%   +11.57%     
===========================================
  Files          28       28               
  Lines        3805     3864       +59     
  Branches      865      917       +52     
===========================================
+ Hits         1010     1473      +463     
+ Misses       2724     2263      -461     
- Partials       71      128       +57     
Flag Coverage Δ
unittests 38.06% <27.77%> (+11.57%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sonichi sonichi requested review from AaronWard, LeoLjl and a team December 1, 2023 16:42
@sonichi
Copy link
Contributor

sonichi commented Dec 1, 2023

Suggestion: use pre-commit to format the code.

@ekzhu
Copy link
Collaborator

ekzhu commented Jan 6, 2024

I tried to merge the conflicts but don't have access to the repo. @olgavrou can merge the conflicts?

Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Customizable client adds a lot of value. Regarding this PR I think it moves the code to the right direction. A couple of things to consider in a broader context:

  1. The current PR addresses the client API. Ideally, we want to bind message format API call parameters with the client as well. See [Bug]: name Field Compatibility Issue in AssistantAgent and UserProxyAgent with fireworks.ai API #1129 for some issues with alternative message format.
  2. How should we approach LLM serving tools that aims for OpenAI compatibility, like vllm and litellm. I think a customizable client layer in our code is still needed as we may need to move faster than dependencies.

Lastly, the PR does a lot of code moving so need to make sure the latest changes in the main branch were copied to the new destination.

@olgavrou
Copy link
Contributor Author

I tried to merge the conflicts but don't have access to the repo. @olgavrou can merge the conflicts?

@ekzhu

I'll open a fresh PR for this since this one has gone quite out of sync with main branch, and close this one

@olgavrou olgavrou mentioned this pull request Jan 19, 2024
3 tasks
@olgavrou
Copy link
Contributor Author

closing in favour of #1345

@olgavrou olgavrou closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants