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

5 high priosdxl #15

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

5 high priosdxl #15

wants to merge 10 commits into from

Conversation

ovuruska
Copy link
Owner

No description provided.

@ovuruska ovuruska linked an issue Apr 30, 2024 that may be closed by this pull request
@ovuruska ovuruska marked this pull request as ready for review April 30, 2024 13:08
@@ -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):
Copy link
Collaborator

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?

Copy link
Owner Author

@ovuruska ovuruska May 4, 2024

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.

Copy link
Owner Author

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.

@@ -2,7 +2,7 @@
This module contains the constants used by the DeepInfra API client.
"""

MAX_RETRIES = 5
MAX_RETRIES = 0
Copy link
Collaborator

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):
Copy link
Collaborator

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.

Copy link
Owner Author

@ovuruska ovuruska May 4, 2024

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.

@ovuruska ovuruska marked this pull request as draft May 6, 2024 11:05
Copy link

sonarqubecloud bot commented May 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@ovuruska ovuruska marked this pull request as ready for review May 7, 2024 12:45
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.

[High Prio] SDXL
2 participants