[WIP] First basic draft of the dispatch system using Redis #1310

lasryaric · 2023-09-07T15:17:44Z

This is a first draft of what the dispatch system could look like, using Redis streams.

The two main Redis commands used are:

XADD to add data to a "persistent stream".
XREAD to read data from the persistant stream.

Let me know what you think.

docker-compose.yml

spolu

NAICE

spolu · 2023-09-07T15:23:28Z

Reminder that this is on the blocking path for moving to the UI but it looks simple and well advanced enough that we likely will have it ready by tomorrow \o/

lasryaric · 2023-09-07T15:25:35Z

Yes! Will do my best to unlock the API layer asap.

…

On Thu, Sep 7, 2023 at 5:23 PM Stanislas Polu ***@***.***> wrote: Reminder that this is on the blocking path for moving to the UI but it looks simple and well advanced enough that we likely will have it ready by tomorrow \o/ — Reply to this email directly, view it on GitHub <#1310 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACXUNPEK7NX6ZRCFDVYIYTXZHRHVANCNFSM6AAAAAA4PCFC5I> . You are receiving this because you authored the thread.Message ID: ***@***.***>

lasryaric · 2023-09-07T15:25:58Z

The new port is dor the http layer of qdrant

…

On Thu, Sep 7, 2023 at 5:22 PM Stanislas Polu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In docker-compose.yml <#1310 (comment)>: > ports: - 6334:6334 + - 6333:6333 why this change? — Reply to this email directly, view it on GitHub <#1310 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACXUNLNVKJBTCXDMQOK2NLXZHRD5ANCNFSM6AAAAAA4PCFC5I> . You are receiving this because you authored the thread.Message ID: ***@***.***>

spolu · 2023-09-07T15:28:13Z

re qdrant, do we need it?

lasryaric · 2023-09-07T19:31:44Z

Re qdrant port: I use it often for debugging vis curl.

spolu · 2023-09-08T07:33:41Z

Gotcha I realize now this is not impacting production 👍

lasryaric · 2023-09-08T07:49:32Z

👍 side note but Qdrant cloud exposes this port, and I also use it there for debugging / profiling sometimes.

spolu

If conversation sub (which I presume you created for minimal testing) then LGTM

spolu · 2023-09-08T11:25:00Z

front/pages/api/v1/w/[wId]/assistant/[cId]/messages/index.ts

+        },
+      });
+      res.status(200).end();
+      await p;


do we need the await? why not void?

front/pages/api/v1/w/[wId]/assistant/[cId]/messages/index.ts

spolu

Actually a few comments

front/lib/redis.ts

front/lib/api/assistant/pubsub.ts

spolu · 2023-09-08T11:30:16Z

front/lib/api/assistant/pubsub.ts

+  while (true) {
+    const events = await client.xRead(
+      { key: pubsubChannel, id: lastEventId ? lastEventId : "0-0" },
+      { COUNT: 1, BLOCK: 10000 }


What's the plan with 10s here? what happens once it's reached?

front/lib/api/assistant/pubsub.ts

lasryaric · 2023-09-08T11:31:22Z

There is a weird behavior that I am not understanding yet with the subscribe (sub) endpoint. Give me a bit more time before using it (~15 minutes).

…

On Fri, Sep 8, 2023 at 1:26 PM Stanislas Polu ***@***.***> wrote: ***@***.**** approved this pull request. If conversation sub (which I presume you created for minimal testing) then LGTM ------------------------------ In front/pages/api/v1/w/[wId]/assistant/[cId]/messages/index.ts <#1310 (comment)>: > + // not sure how to provide the content here for now. + content: [], + visibility: conv.visibility, + }, + message: payload.message, + mentions: [], + context: { + timezone: payload.context.timezone, + username: payload.context.username, + fullName: payload.context.fullName, + email: payload.context.email, + profilePictureUrl: payload.context.profilePictureUrl, + }, + }); + res.status(200).end(); + await p; do we need the await? why not void? ------------------------------ In front/pages/api/v1/w/[wId]/assistant/[cId]/messages/index.ts <#1310 (comment)>: > + const conv = await Conversation.findOne({ + where: { + sId: req.query.cId as string, + }, + }); + if (!conv) { + return apiError(req, res, { + status_code: 404, + api_error: { + type: "conversation_not_found", + message: "Conversation not found.", + }, + }); + } + + // no time for actual io-ts parsing right now, so here is the expected structure. roger sounds good 👍 — Reply to this email directly, view it on GitHub <#1310 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACXUNKTGG45JX3MEQCHWK3XZL6FXANCNFSM6AAAAAA4PCFC5I> . You are receiving this because you authored the thread.Message ID: ***@***.***>

lasryaric · 2023-09-08T12:42:19Z

Ready for review on my side.

spolu · 2023-09-08T12:44:03Z

front/lib/api/assistant/pubsub.ts

+    const events = await client.xRead(
+      { key: pubsubChannel, id: lastEventId ? lastEventId : "0-0" },
+      // weird, xread does not return on new message when count is = 1. Anything over 1 works.
+      { COUNT: 10, BLOCK: 60 * 1000 }


// weird right in the middle of the dispatch system is not great.

COUNT must have an impact no? Does it work if we have more than 10 events in the queue?

spolu · 2023-09-08T12:45:17Z

front/pages/api/v1/w/[wId]/assistant/conversation/[cId]/sub.ts

+    });
+  }
+
+  const conv = await Conversation.findOne({


Let's remove the Conversation loading for now

What happens if we call getConversationEvents on a bogus sId?

it will block until the timeout has been reached out and return 0 event.

philipperolet · 2023-09-08T14:23:38Z

front/lib/api/assistant/pubsub.ts

+    while (true) {
+      const events = await redis.xRead(
+        { key: pubsubChannel, id: lastEventId ? lastEventId : "0-0" },
+        // weird, xread does not return on new message when count is = 1. Anything over 1 works.


Hey! Reading the code to prepare, thanks for all that aric 🙏

NRE - Just asking : something I don't understand here: if we use count = 1 then only 1 event on the array max right, and no need for the loop below? Why not get e.g. count = 100, to limit the calls to redis -- and keep the loop?

Sorry to drop in uninvited 😅 hope I'm not disturbing

For context: this comment was wrong, count is honored, I am not sure exactly if redis can return more than 1 event when using the blocking call because I have read that it returns on the first one, but for the first call with no "lastEventId" set, it does return multiple rows.

In any case, that's the redis interface, they always return an array, so we'll always have to loop. We could fetch event[0] but it does not change the code much I think. Does it?

lasryaric marked this pull request as draft September 7, 2023 15:19

spolu reviewed Sep 7, 2023

View reviewed changes

docker-compose.yml Outdated Show resolved Hide resolved

spolu reviewed Sep 7, 2023

View reviewed changes

docker-compose.yml Show resolved Hide resolved

spolu reviewed Sep 7, 2023

View reviewed changes

spolu approved these changes Sep 8, 2023

View reviewed changes

spolu requested changes Sep 8, 2023

View reviewed changes

lasryaric added 4 commits September 8, 2023 13:52

[WIP] First basic draft of the dispatch system using Redis

9f31ebb

Add APIs: POST /conv, POST conv/message, GET conv/sub

8e5932d

Making sub works

0015fa6

Clean up docker compose

09d1bd3

lasryaric force-pushed the aric-aissistant_dispatch branch from a6834c4 to 09d1bd3 Compare September 8, 2023 11:52

Addressing @spolu's comments

8762860

lasryaric requested a review from spolu September 8, 2023 12:19

lasryaric marked this pull request as ready for review September 8, 2023 12:19

Clean up after rebasing

83075c9

spolu reviewed Sep 8, 2023

View reviewed changes

One redis client per connection

50578d8

philipperolet reviewed Sep 8, 2023

View reviewed changes

lasryaric merged commit cc00fae into main Sep 8, 2023

lasryaric deleted the aric-aissistant_dispatch branch September 8, 2023 14:37

spolu mentioned this pull request Sep 8, 2023

Redis-based dispatching layer #1306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] First basic draft of the dispatch system using Redis #1310

[WIP] First basic draft of the dispatch system using Redis #1310

lasryaric commented Sep 7, 2023 •

edited

Loading

spolu left a comment

spolu commented Sep 7, 2023

lasryaric commented Sep 7, 2023 via email

lasryaric commented Sep 7, 2023 via email

spolu commented Sep 7, 2023

lasryaric commented Sep 7, 2023

spolu commented Sep 8, 2023

lasryaric commented Sep 8, 2023

spolu left a comment

spolu Sep 8, 2023

spolu left a comment

spolu Sep 8, 2023

lasryaric commented Sep 8, 2023 via email

lasryaric commented Sep 8, 2023

spolu Sep 8, 2023

spolu Sep 8, 2023

spolu Sep 8, 2023

lasryaric Sep 8, 2023

philipperolet Sep 8, 2023

philipperolet Sep 8, 2023

lasryaric Sep 8, 2023

[WIP] First basic draft of the dispatch system using Redis #1310

[WIP] First basic draft of the dispatch system using Redis #1310

Conversation

lasryaric commented Sep 7, 2023 • edited Loading

spolu left a comment

Choose a reason for hiding this comment

spolu commented Sep 7, 2023

lasryaric commented Sep 7, 2023 via email

lasryaric commented Sep 7, 2023 via email

spolu commented Sep 7, 2023

lasryaric commented Sep 7, 2023

spolu commented Sep 8, 2023

lasryaric commented Sep 8, 2023

spolu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spolu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasryaric commented Sep 8, 2023 via email

lasryaric commented Sep 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lasryaric commented Sep 7, 2023 •

edited

Loading