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

Fix for http client #2579

Merged
merged 13 commits into from
May 4, 2024
Merged

Fix for http client #2579

merged 13 commits into from
May 4, 2024

Conversation

AbdurNawaz
Copy link
Collaborator

@AbdurNawaz AbdurNawaz commented May 3, 2024

Why are these changes needed?

When you add the parameter http_client (which is typically an instance of httpx.Client or httpx.AsyncClient) to llm_config, the method copy.deepcopy throws an error as it won't be able to pickle the client object. This is a fix to skip the copying if an http_client parameter is passed.

Code to reproduce:

import httpx

http_client = httpx.Client()
                          
mixtral_config_list = [                                                                                                
    {
        "base_url": "OPENAI_API_BASE_URL"
        "api_key": "OPENAI_API_KEY",                                                                                       
        "http_client": http_client,
    }
]

#the below line will throw an error
assistant = AssistantAgent("assistant", llm_config={"config_list": config_list})

Related issue number

#2412

Checks

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.

Thanks for the PR. Given @Gr3atWh173 's comment: #2412 (comment)
I think instead of checking for http_client key, we should try and raise the error but append a more meaningful error message saying please implement the value class with __deepcopy__ method to support deepcopy.

@AbdurNawaz
Copy link
Collaborator Author

@microsoft-github-policy-service agree

@AbdurNawaz
Copy link
Collaborator Author

@ekzhu agreed - I've added the necessary changes and updated the docs. Please review

@AbdurNawaz AbdurNawaz closed this May 4, 2024
@AbdurNawaz AbdurNawaz reopened this May 4, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 25.06%. Comparing base (ded2d61) to head (7ad33ce).
Report is 33 commits behind head on main.

Files Patch % Lines
autogen/agentchat/conversable_agent.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2579      +/-   ##
==========================================
- Coverage   33.33%   25.06%   -8.27%     
==========================================
  Files          83       85       +2     
  Lines        8636     9011     +375     
  Branches     1835     2069     +234     
==========================================
- Hits         2879     2259     -620     
- Misses       5516     6491     +975     
- Partials      241      261      +20     
Flag Coverage Δ
unittest 12.38% <0.00%> (?)
unittests 24.62% <50.00%> (-8.72%) ⬇️

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.

@ekzhu ekzhu added this pull request to the merge queue May 4, 2024
Merged via the queue into microsoft:main with commit 4711d7b May 4, 2024
75 of 88 checks passed
jayralencar pushed a commit to jayralencar/autogen that referenced this pull request May 28, 2024
* Fix for http client

* fixed constructor to only ignore the http_client while copying

* fixed comment formating

* removed check for http_client and added error message with docs

* fix formatting

* fix formatting

* added test for http-fix

* changed title and content of docs

* changed test func name
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.

3 participants