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

Switch from socket.io to vanilla websockets & fix state updates for websockets #1048

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

wwwillchen
Copy link
Collaborator

@wwwillchen wwwillchen commented Oct 23, 2024

Based on a suggestion from a co-worker who used socket.io quite a bit, it's quite a lot of hassle (e.g. protocol version compatibility) compared to using vanilla websocket since we don't need to use an HTTP fallback in our use case.

Notes:

  • Uses the native WebSockets API on the client-side and flask-sock which is a modern successor to flask-socketio (note: same author)
  • Creates a UUID for the session (i.e. WebSockets lifetime) so that we can reuse the same Context instance across multiple user events (this is the same behavior as introduced in Add proper concurrency support in websockets mode #1036, but using a different mechanism because flask-sock doesn't include request.sid)
  • Launches a background thread with the Flask request context so that multiple user events can be processed concurrently.
  • Disables updating state from the client or sending state to the client because the state is already on the server (since have a long-lived Context object in websockets mode).
  • Related to the above point, I've added a warning message in the docs for MESOP_CONCURRENT_UPDATES_ENABLED which I should have added earlier. I'm not sure if MESOP_CONCURRENT_UPDATES_ENABLED is really viable as a standalone option outside of using it with MESOP_WEB_SOCKETS_ENABLED

@wwwillchen wwwillchen changed the title Use vanilla WebSockets (client-side) and flask-sock (server-side) instead of socket.io Switch from socket.io to vanilla websockets Oct 24, 2024
@wwwillchen wwwillchen changed the title Switch from socket.io to vanilla websockets Switch from socket.io to vanilla websockets & fix state updates for websockets Oct 24, 2024
@wwwillchen wwwillchen requested a review from richard-to October 24, 2024 05:24
Copy link
Collaborator

@richard-to richard-to left a comment

Choose a reason for hiding this comment

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

I think this looks reasonable. I think the part I didn't fully understand was the use of the while True loop and the threading.

@wwwillchen
Copy link
Collaborator Author

I think this looks reasonable. I think the part I didn't fully understand was the use of the while True loop and the threading.

the while True loop basically means a thread is allocated per websocket connection and watching for any incoming messages, which isn't super scalable (compared to a proper async approach), but I think good enough for now.

re: the threading, it's to allow multiple user events for the same websocket connection to be processed concurrently, otherwise they would end up getting processed serially.

The concurrency still here is a bit tricky and I might try to simplify/change this in the future.

@wwwillchen wwwillchen merged commit 2a0e9e5 into google:main Oct 25, 2024
6 checks passed
@wwwillchen wwwillchen deleted the vanilla-ws branch October 25, 2024 05:01
@richard-to
Copy link
Collaborator

re: the threading, it's to allow multiple user events for the same websocket connection to be processed concurrently, otherwise they would end up getting processed serially.

Yes, I understood that part. I guess I was wondering about thread safety. I remember we do have the lock for when the context is updated. So I assume the rest is fine. The other thing I was wondering about was the GIL. Are there scenarios where the threading may not actually work that well (there was one time where I was using threads and it ended up using a ton of compute and being way slower than just single threaded -- but I think I just picked a poor use case since it was more processing intensive versus I/O intensive)? I think for a long LLM call that's hitting the network, I'm assuming that should work. But I'll try to play around with this version of the web sockets implementation a bit more when I get the chance.

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.

2 participants