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

fix: do not block entire program with sleep #1374

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

Conversation

sandangel
Copy link
Contributor

@sandangel sandangel commented Sep 25, 2024

Signed-off-by: San Nguyen [email protected]

with time.sleep(), the entire program is stopped, which yield very bad performance for server handling concurrent requests.

Introducing new async init method for creating Step and Message with asyncio.sleep so that the async loop will give back the control to the program while waiting for sleep.

Let me know what is your thought and I help to replace existing usage with the new one.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. backend Pertains to the Python backend. bug Something isn't working labels Sep 25, 2024
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
@dokterbob
Copy link
Collaborator

Signed-off-by: San Nguyen [email protected]

with time.sleep(), the entire program is stopped, which yield very bad performance for server handling concurrent requests.

Introducing new async init method for creating Step and Message with asyncio.sleep so that the async loop will give back the control to the program while waiting for sleep.

Let me know what is your thought and I help to replace existing usage with the new one.

Why is this sleep statement there in the first place? @willydouhard

created_at,
)
return self

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where this init() is called, here or anywhere. Could you please explain why this is here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note the type error raised by mypy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dokterbob , as mentioned, if sleep is needed then we will need to initiate all Message or Step with init, which will be lots of changes. May I confirm on your side that sleep is absolutely needed before making a big change?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the sleep statement is here. Perhaps @willydouhard does?

I am asking about init() because you add this code to this PR and it does not seem to be called anywhere. I'd like to avoid dead code as much as possible.

@dokterbob dokterbob added the blocked Awaiting update or feedback from user after initial review/comments. label Nov 6, 2024
@sandangel
Copy link
Contributor Author

Hi, is there any update so we can just remove the sleep statement?

@sandangel
Copy link
Contributor Author

hi @willydouhard , can we have your comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Pertains to the Python backend. blocked Awaiting update or feedback from user after initial review/comments. bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants