-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Assure created model of OpenAIWrapper is in config_list #847
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
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
af8bdd2
assure created model in config_list
yiranwu0 15da7b5
Merge remote-tracking branch 'origin/main' into fixcreate
yiranwu0 ca97d16
update
yiranwu0 7910423
update
yiranwu0 ef4085a
Merge remote-tracking branch 'origin/main' into fixcreate
yiranwu0 5c41cbb
update
yiranwu0 6ab018a
update
yiranwu0 ca02914
update
yiranwu0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,7 +74,13 @@ def __init__(self, *, config_list: List[Dict] = None, **base_config): | |
| openai_config, extra_kwargs = self._separate_openai_config(base_config) | ||
| if type(config_list) is list and len(config_list) == 0: | ||
| logger.warning("openai client was provided with an empty config_list, which may not be intended.") | ||
| if isinstance(config_list, list) and any((c.get("model") is None for c in config_list)): | ||
| logger.warning( | ||
| "OpenAIWrapper: one or more configs in config_list do not have a model specified, which may not be intended." | ||
| ) | ||
| if config_list: | ||
| self._check_model_in_config_list(model=base_config.get("model"), config_list=config_list) | ||
| self._default_model = base_config.get("model", None) | ||
| config_list = [config.copy() for config in config_list] # make a copy before modifying | ||
| self._clients = [self._client(config, openai_config) for config in config_list] # could modify the config | ||
| self._config_list = [ | ||
|
|
@@ -184,6 +190,16 @@ def _construct_create_params(self, create_config: Dict, extra_kwargs: Dict) -> D | |
| ] | ||
| return params | ||
|
|
||
| @staticmethod | ||
| def _check_model_in_config_list(model, config_list=None): | ||
| if model is None or config_list is None: | ||
| return | ||
|
|
||
| if not any(c.get("model", None) == model for c in config_list): | ||
| raise ValueError( | ||
| f"model {model} is not found in the config_list. Please add the model with the corresponding api_key when creating the OpenAIWrapper." | ||
| ) | ||
|
|
||
|
Comment on lines
+193
to
+202
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check doesn't make sense as config_list is used to update the base_config. It's valid to have model in the base_config and not specified in config_list. |
||
| def create(self, **config): | ||
| """Make a completion for a given config using openai's clients. | ||
| Besides the kwargs allowed in openai's client, we allow the following additional kwargs. | ||
|
|
@@ -215,9 +231,14 @@ def yes_or_no_filter(context, response): | |
| if ERROR: | ||
| raise ERROR | ||
| last = len(self._clients) - 1 | ||
| model_to_use = config.get("model", self._default_model) | ||
| self._check_model_in_config_list(model=model_to_use, config_list=self._config_list) | ||
|
|
||
| for i, client in enumerate(self._clients): | ||
| if model_to_use and model_to_use != self._config_list[i].get("model"): | ||
| continue | ||
| # merge the input config with the i-th config in the config list | ||
| full_config = {**config, **self._config_list[i]} | ||
| full_config = {**self._config_list[i], **config} | ||
| # separate the config into create_config and extra_kwargs | ||
| create_config, extra_kwargs = self._separate_create_config(full_config) | ||
| # process for azure | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This check shouldn't be here because one can provide the model in base_config and omit model in config_list.