-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Mongo] optimizations #859
Conversation
src/routes/+layout.server.ts
Outdated
const assistantIds = conversations | ||
.map((conv) => conv.assistantId) | ||
.filter((el) => !!el) as ObjectId[]; | ||
const assistantIds = settings?.assistants?.map((assistantId) => assistantId) ?? []; |
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.
with this change, it is possible to re-use the results from collections.assistants.find
query below & completely remove the query being done in src/routes/settings/+layout.server.ts
cc: @nsarrazin is this change good?
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.
the change is: getting all the assistants of a user, instead of only ones user has conversation with
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.
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.
could you elaborate on what's expected behavior?
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.
we need all assistants for conversations in order to compute their avatarHash below:
avatarHash:
conv.assistantId &&
assistants.find((a) => a._id.toString() === conv.assistantId?.toString())?.avatar,
If a user removes an assistant from their settings, with this PR, the avatar of the assitant won't show up in the conversation list next to the conversation title. Eg julien's avatar wouldn't show here:
(if I removed him in my settings)
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.
You can still do this:
const assistantIds = settings?.assistants?.map((assistantId) => assistantId) ?? []; | |
const assistantIds = [ | |
...(settings?.assistants?.map((assistantId) => assistantId) ?? []), | |
...conversations.map(conv => conv.assistantId) | |
]; |
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.
handled in 07bd2d1
@@ -95,9 +95,7 @@ export const load: LayoutServerLoad = async ({ locals, depends }) => { | |||
}) | |||
.toArray(); |
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.
.toArray(); | |
.limit(100) | |
.toArray(); |
Maybe another optimization to do?
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.
applied the change. Will implement pagination for conversation in a subsequent PR
src/routes/+layout.server.ts
Outdated
const assistantIds = conversations | ||
.map((conv) => conv.assistantId) | ||
.filter((el) => !!el) as ObjectId[]; | ||
const assistantIds = settings?.assistants?.map((assistantId) => assistantId) ?? []; |
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.
You can still do this:
const assistantIds = settings?.assistants?.map((assistantId) => assistantId) ?? []; | |
const assistantIds = [ | |
...(settings?.assistants?.map((assistantId) => assistantId) ?? []), | |
...conversations.map(conv => conv.assistantId) | |
]; |
Co-authored-by: Eliott C. <[email protected]>
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.
Looks good now!
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.
We should probably add some monitoring at some point
eg create a timeseries collection (we can add a TTL index to it too) with url, method, timestamps, user / session id, and time taken for different parts of the code (just the +layout.server.ts's load at the start)
Then we can calculate stats like p50/p90/p99 and also see if the slow times are triggered by specific users or urls/parts of the code
Follow up to #859, specifically to [this comment](#859 (comment)) Bug: when a user removes an assistant, the assistant was NOT being removed from the settings list. [This PR fixes this problem](#869 (comment))
Could be addressed by #692, I think everyone is kind of standardising around opentelemetry at the moment so that would be my vote. |
* [Mongo] remove duplicated assistants query * Update src/routes/+layout.server.ts Co-authored-by: Eliott C. <[email protected]> * fix avatarHash problem * limit convo to last 300 --------- Co-authored-by: Eliott C. <[email protected]>
Follow up to huggingface#859, specifically to [this comment](huggingface#859 (comment)) Bug: when a user removes an assistant, the assistant was NOT being removed from the settings list. [This PR fixes this problem](huggingface#869 (comment))
Mongo optimizations