-
Notifications
You must be signed in to change notification settings - Fork 993
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
base: main
Are you sure you want to change the base?
fix: do not block entire program with sleep #1374
Conversation
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Signed-off-by: San Nguyen <[email protected]>
Why is this sleep statement there in the first place? @willydouhard |
created_at, | ||
) | ||
return self | ||
|
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 don't see where this init()
is called, here or anywhere. Could you please explain why this is here?
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.
Also note the type error raised by mypy.
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.
@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?
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 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.
Hi, is there any update so we can just remove the sleep statement? |
hi @willydouhard , can we have your comment? |
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 withasyncio.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.