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

Avoid deadlock in event start #322

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Avoid deadlock in event start #322

merged 1 commit into from
Aug 29, 2023

Conversation

xsyann
Copy link
Contributor

@xsyann xsyann commented Aug 25, 2023

When using streaming, the main event loop is blocked to await tokens, if we try to execute a function (Message.send() here) in the same event loop, it leads to a deadlock.

Adding a new_loop parameter to the run_sync function allows to create a new event loop in the current Thread and allows to send a Message during a stream.

@xsyann xsyann force-pushed the yk/event-deadlock branch 3 times, most recently from 2697eaa to 439dec0 Compare August 28, 2023 12:48
Copy link
Contributor

@ramnes ramnes left a comment

Choose a reason for hiding this comment

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

Good if that fixes an issue.

It looks like a very weird approach to me though, because:

  1. It doesn't really make sense to run an event loop for just one function;
  2. I don't understand, as a run_sync() user, how do I know if I should pass new_loop=True or new_loop=False? Am I supposed to try one, and if that doesn't work, try the other one? My point being that the event loop handling probably shouldn't be the responsibility of the API user.

@xsyann xsyann force-pushed the yk/event-deadlock branch from 439dec0 to d329739 Compare August 28, 2023 13:32
@xsyann
Copy link
Contributor Author

xsyann commented Aug 28, 2023

Good if that fixes an issue.

It looks like a very weird approach to me though, because:

  1. It doesn't really make sense to run an event loop for just one function;
  2. I don't understand, as a run_sync() user, how do I know if I should pass new_loop=True or new_loop=False? Am I supposed to try one, and if that doesn't work, try the other one? My point being that the event loop handling probably shouldn't be the responsibility of the API user.
  1. We don't really have the choice as we need to execute the fonction in the Thread (as the main event loop is blocked)
    I agree that a better solution would have been to make the token generator async to avoid blocking the main event loop but it is not yet implemented in llama_index.

  2. I changed it to a main_thread boolean and added some doc. As it has a default value, generally a user shouldn't have to deal with it, except in this particular use case.

@xsyann xsyann force-pushed the yk/event-deadlock branch 4 times, most recently from fbf2667 to 02cb0b0 Compare August 28, 2023 16:28
@@ -61,7 +61,7 @@ types-requests = "^2.31.0.2"
types-aiofiles = "^23.1.0.5"

[tool.mypy]
python_version = "3.8"
python_version = "3.10"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are we bumping the ci python version?

if threading.current_thread() == threading.main_thread():
return sync(co)
else:
if not main_thread:
return asyncio.run(co)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we use asyncio.run here instead of sync()? Also the main_thread parameter naming might be confusing since we check threading.current_thread() == threading.main_thread() in the condition above

When using streaming, the main event loop is blocked to await tokens, if we try to
execute a function (Message.send() here) in the same event loop, it leads to a deadlock.
Adding a new_loop parameter to the run_sync function allows to create a new event loop
in the current Thread and allows to send a Message during a stream.
@xsyann xsyann force-pushed the yk/event-deadlock branch from 02cb0b0 to 2e0efa3 Compare August 29, 2023 08:16
Copy link
Contributor

@ramnes ramnes left a comment

Choose a reason for hiding this comment

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

I like force_new_loop better 👍

@ramnes ramnes merged commit 1ccab0d into main Aug 29, 2023
4 checks passed
@willydouhard willydouhard deleted the yk/event-deadlock branch September 7, 2023 15:16
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.

3 participants