Skip to content

Custom Client support#831

Closed
olgavrou wants to merge 14 commits into
microsoft:mainfrom
VowpalWabbit:custom_client_impl
Closed

Custom Client support#831
olgavrou wants to merge 14 commits into
microsoft:mainfrom
VowpalWabbit:custom_client_impl

Conversation

@olgavrou

@olgavrou olgavrou commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Dec 1, 2023

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 27.77778% with 117 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.12%. Comparing base (a0288e0) to head (756e7b8).
⚠️ Report is 2704 commits behind head on main.

Files with missing lines 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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

sonichi commented Dec 1, 2023

Copy link
Copy Markdown
Contributor

Suggestion: use pre-commit to format the code.

@ekzhu

ekzhu commented Jan 6, 2024

Copy link
Copy Markdown
Contributor

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

@ekzhu ekzhu left a comment

Copy link
Copy Markdown
Contributor

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 #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
Copy Markdown
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
Copy Markdown
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