-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Failed to save the Qianfan large model #2092
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -45,7 +45,7 @@ def is_valid(self, model_type: str, model_name, model_credential: Dict[str, obje | |||
gettext('{model_type} Model type is not supported').format(model_type=model_type)) | |||
model = provider.get_model(model_type, model_name, model_credential, **model_params) | |||
model_info = [model.lower() for model in model.client.models()] | |||
if not model_info.__containsgettext(model_name.lower()): | |||
if not model_info.__contains__(model_name.lower()): | |||
raise AppApiException(ValidCode.valid_error.value, | |||
gettext('{model_name} The model does not support').format(model_name=model_name)) | |||
for key in ['api_key', 'secret_key']: |
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.
The provided code snippet has a few issues and can be optimized:
Issues found:
- Incorrect usage of
__containsgettext
:- The correct method to check for membership in a list is
in
, not__containsgettext
.
- The correct method to check for membership in a list is
- String interpolation using
{}
instead of%s
:- Using string formatting with curly braces (
{}
) is generally considered more modern and readable compared to old-style format strings.
- Using string formatting with curly braces (
- Redundant parentheses around
gettext('...'
.format(...)):- These can be omitted.
Optimizations:
- Inline error message construction in exceptions:
- It's better practice to inline the error messages directly inside the exception constructor rather than concatenating them outside.
Here's the corrected and optimized version of the code:
def is_valid(self, model_type: str, model_name, model_credential: Dict[str, object], model_params):
# Check if the model type is supported
if model_type not in self.supported_models():
raise AppApiException(ValidCode.invalid_error.value,
gettext('{model_type} Model type is not supported')
# Get the model instance
model = provider.get_model(model_type, model_name, model_credential, **model_params)
# Get a sorted list of available models (case-insensitive) from the client
model_info = [model.lower() for model in sorted(model.client.models())]
# Check if the requested model name exists
if model_name.lower() not in model_info:
raise AppApiException(ValidCode.valid_error.value,
gettext('{model_name} The model does not support'.format(model_name=model_name)))
# Iterate over keys required for authentication
for key in ['api_key', 'secret_key']:
Additional Considerations:
- Ensure that
self.supported_models()
returns a valid set or list of supported model types. - Make sure that
provider.get_model()
correctly retrieves the model based on the provided parameters. - Consider adding logging to help diagnose any issues that may arise during execution.
fix: Failed to save the Qianfan large model