-
Notifications
You must be signed in to change notification settings - Fork 0
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
5 high priosdxl #15
base: main
Are you sure you want to change the base?
5 high priosdxl #15
Conversation
deepinfra/clients/deepinfra.py
Outdated
@@ -40,18 +36,9 @@ def post(self, data, config=None): | |||
"User-Agent": USER_AGENT, | |||
"Authorization": f"Bearer {self.auth_token}", | |||
} | |||
for attempt in range(self.max_retries + 1): |
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.
So you dropped the retries logic, which is probably OK, as long as we replace it with something equivalent (very likely requests has some builtin, you can dig a bit). Otherwise what is the purpose?
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 idea is to leave this decision to the user. For instance, me as a user does not want backoff by default.
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.
Guess you're right. This change expands scope of this PR which is irrelevant.
deepinfra/constants/client.py
Outdated
@@ -2,7 +2,7 @@ | |||
This module contains the constants used by the DeepInfra API client. | |||
""" | |||
|
|||
MAX_RETRIES = 5 | |||
MAX_RETRIES = 0 |
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.
Is MAX_RETRIES even used any more?
@@ -14,7 +14,7 @@ class AutomaticSpeechRecognition(BaseModel): | |||
@docs Check the available models at https://deepinfra.com/models/automatic-speech-recognition | |||
""" | |||
|
|||
def generate(self, body) -> AutomaticSpeechRecognitionResponse: | |||
def generate(self, body): |
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.
I guess this steps on top of the other PR, so the same arguments apply. The generate method absolutely needs annotations on the input (body
) and response.
Now I see the code is blocking (i.e not async). We should provide sync and async variants in that case.
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.
for annotation, I got an idea which may be scope of another PR.
Example usage
`
request = EmbeddingsRequest(inputs=["Hello World!"])
model = Embeddings()
model.generate(request)
`
By creating a request class for each base class, we can
1-) Validate the input before request is sent.
2-) Increase iteration speed for our users by improving code completion aspects.
|
No description provided.