-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: Allow OpenAI client config in OpenAIChatGenerator
and AzureOpenAIChatGenerator
#9215
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
Conversation
|
Pull Request Test Coverage Report for Build 14497519261Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach...
I suggest creating a test module in test/test_utils/test_http_client.py with a few tests: test_init_http_client
, test_http_client_kwargs_type_validation
, and test_http_client_kwargs_with_invalid_params
.
After doing that, we might also remove the specific tests from the components - I'll leave that up to you.
@@ -128,6 +131,8 @@ def __init__( # pylint: disable=too-many-positional-arguments | |||
the schema provided in the `parameters` field of the tool definition, but this may increase latency. | |||
:param azure_ad_token_provider: A function that returns an Azure Active Directory token, will be invoked on | |||
every request. | |||
:param http_client_kwargs: | |||
A dictionary of keyword arguments to configure a custom httpx.Client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Amnah199!
I want to add similar support to other components, and I think they all should have the same description. I suggest adding documentation similar to the following for :param http_client:
:
A dictionary of keyword arguments to configure a custom httpx.Client
for your use case: proxies, authentication and other advanced functionalities of HTTPX.
You can set a proxy with basic authorization using the environment variables: HTTP_PROXY
and HTTPS_PROXY
, ALL_PROXY
and NO_PROXY
, for example HTTP_PROXY=http://user:[email protected]:8080
.
A dictionary of keyword arguments to configure a custom `httpx.Client` for your use case: [proxies](https://www.python-httpx.org/advanced/proxies), [authentication](https://www.python-httpx.org/advanced/authentication) and other [advanced functionalities](https://www.python-httpx.org/advanced/clients) of HTTPX.
You can set a proxy with basic authorization using [the environment variables](https://www.python-httpx.org/environment_variables): `HTTP_PROXY` and `HTTPS_PROXY`, `ALL_PROXY` and `NO_PROXY`, for example `HTTP_PROXY=http://user:[email protected]:8080`.
raise TypeError("The parameter 'http_client_kwargs' must be a dictionary.") | ||
if async_client: | ||
return httpx.AsyncClient(**http_client_kwargs) | ||
return httpx.Client(**http_client_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using openai.DefaultHttpxClient
because it provides specific configurations from OpenAI (see here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2 cents.
In case the user wants to provide their own client, I would leave them free to choose the configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anakin87 As per my understanding, DefaultAsyncHttpxClient
does not limit the configuration options, just the default parameters are pre-configured to be optimal. The user you retains full configurability as with a regular httpx.AsyncClient
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I understand this point, as a user I would find it unexpected to pass some client kwargs and then get a client that automatically sets others as well.
If we decide to go this path, I would clearly indicate this in all docstrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexengrig I see the merits in both approaches. However, we've decided to stick with httpx.Client
for now to avoid introducing potential confusion from using a preconfigured client. This way, users have full transparency and control when creating and customizing their own client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left 2 minor comments.
(There also are merge conflicts)
raise TypeError("The parameter 'http_client_kwargs' must be a dictionary.") | ||
if async_client: | ||
return httpx.AsyncClient(**http_client_kwargs) | ||
return httpx.Client(**http_client_kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My 2 cents.
In case the user wants to provide their own client, I would leave them free to choose the configurations.
assert http_client.base_url == "https://example.com" | ||
|
||
|
||
def test_init_http_client_async(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed tests for init_http_client
method calls from all components. I think these should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great.
Feel free to address my comment (or not 😄 )
…om/deepset-ai/haystack into configure-openai-client-generators
Related Issues
Proposed Changes:
Currently, users cannot configure the HTTP client used in these chat generators. To solve this:
http_client_kwargs
param to store HTTP client configuration options. These options will be used in theinit
method to construct the client and can be safely serialized and deserialized.init_http_client
which will be reused in configuring openai client in different components.How did you test it?
Added new tests
Updated the existing tests
Notes for the reviewer
Similar to this PR
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
and added!
in case the PR includes breaking changes.