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 custom client registration #1653

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

olgavrou
Copy link
Member

Why are these changes needed?

For custom client registration:

  • Removing redundant check for placeholder config inside config list
  • configs in self._config_list can end up having the extra kwargs passed through (e.g. cache_seed) and therefore the check for placeholder_config in self._config_list will fail

Related issue number

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.

Can we add a test just to make sure additional config arguments like cache_seed won't trigger an error?

Also in test_custom_client.py (basically all other tests as well, but we don't have to do everything in this PR) we don't need to check for openai package existence as it is a required dependency.

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.

Can we add a test just to make sure additional config arguments like cache_seed won't trigger an error?

Also in test_custom_client.py (basically all other tests as well, but we don't have to do everything in this PR) we don't need to check for openai package existence as it is a required dependency.

@olgavrou
Copy link
Member Author

Can we add a test just to make sure additional config arguments like cache_seed won't trigger an error?

Also in test_custom_client.py (basically all other tests as well, but we don't have to do everything in this PR) we don't need to check for openai package existence as it is a required dependency.

@ekzhu good point, added unit test, checked that it failed with old code, and passes with PR changes. Also removed the openai req

@ekzhu
Copy link
Collaborator

ekzhu commented Feb 13, 2024

Remember to check the code in #1667 also to see if it works with this update. It works on my side.

@sonichi sonichi added this pull request to the merge queue Feb 13, 2024
Merged via the queue into microsoft:main with commit 71829f3 Feb 13, 2024
46 of 57 checks passed
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* fix custom client registration

* fix

* add test with extra args
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