-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Xinference add #4249
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: Xinference add #4249
Conversation
--bug=1062934 --user=张展玮 【github#4242】添加xinference平台的语音识别模型失败 https://www.tapd.cn/62980211/s/1789683
|
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. DetailsInstructions 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-sigs/prow 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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
||
| def get_model_params_setting_form(self, model_name): | ||
| pass | ||
| return |
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.
There are a few potential issues, optimizations, and improvements to consider:
-
Optimize
get_modelcall: The current implementation of theget_modelcall passes three parameters (model_type,model_name, andmodel_credential). If additional keyword arguments may be needed in the future (such as**kwargs), it might make sense to update this method to handle them properly. -
Clarify
passstatement: In theget_model_params_setting_formfunction, thepassstatement could indicate some functionality that should be implemented later on. It would be better to removereturn:or replace it with an actual implementation when necessary. -
Consider moving validation logic elsewhere: Currently, all the validation logic is mixed into the main body of the class's methods. This can lead to duplication and reduce readability. Consider extracting these validations to separate functions or classes where they belong more logically.
Here's the updated code snippet incorporating suggested changes:
def __init__(self, *args, **kwargs):
# Initialize instance variables here
def is_valid(self, model_type: str, model_name, model_credential: Dict[str, object]):
if not self._provider.get_provider_status():
return False
params = {'model_type': model_type, "key": model_credential['api_key'], 'name': model_name}
try:
model_data = self._provider.get_model(params) # Use named arguments instead
model.check_auth() # Ensure authentication before further processing
except Exception as e:
if isinstance(e, AppApiException):
raise e
return False
def encryption_dict(self, model: Dict[str, object]):
encrypted_dict = {**model, 'api_key': self._encryption.encrypt(model.get('api_key', ''))}
return encrypted_dict
def get_model_params_setting_form(self, model_name):
# Implement form creation or settings configuration for specific models if required
passThese changes aim to improve clarity, maintainability, and flexibility in the code while addressing potential runtime errors related to missing keywords and redundant checks.
| params: dict | ||
|
|
||
| def __init__(self, **kwargs): | ||
| super().__init__(**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.
The provided code snippet for XInferenceSpeechToText is missing some important attributes. The params attribute is defined within the class definition but is not initialized anywhere during object creation or usage.
Here's an updated version of the class with corrections and additional comments:
from abc import ABC, abstractmethod
import max_kb_base_model # Assuming this module exists
class MaxKBBaseModel(ABC):
@abstractmethod
def process(self):
pass
class BaseSpeechToText(ABC):
@abstractmethod
def transcribe(self, audio_file):
pass
class XInferenceSpeechToText(MaxKBBaseModel, BaseSpeechToText):
api_base: str
api_key: str
model: str
params: dict # Initialize params in __init__ method
def __init__(self, **kwargs):
super().__init__()
self.api_base = kwargs.get('api_base', 'default_url')
self.api_key = kwargs.get('api_key', '')
self.model = kwargs.get('model', 'default_model')
self.params = kwargs.get('params', {}) # Default to an empty dictionary if none provided
print(f"Initialized {type(self).__name__} instance with API base: '{self.api_base}', "
f"API key: '{self.api_key}', model: '{self.model}'")
# Example usage:
# inference_stt = XInferenceSpeechToText(api_base='https://example.com/api',
# api_key='your_api_key',
# model='inception_v3')Key Improvements:
- Initialization of
params: Added an initialization line in the__init__method that assigns a default value toself.params. - Default Values: Set defaults for each parameter (
api_base,api_key,model) usingkwargs, which allows flexibility when creating instances (e.g., via function arguments). - Error Handling: Used
kwargs.get()to safely access parameters, setting sane default values where needed. - Print Statement: Included a message in the constructor to confirm initialization details.
This should address any irregularities present in the original code. Let me know if you have any further questions!
fix: Xinference add --bug=1062934 --user=张展玮 【github#4242】添加xinference平台的语音识别模型失败 https://www.tapd.cn/62980211/s/1789683