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

feat: Add search template mutation #9802

Merged
merged 6 commits into from
Jul 1, 2024

Conversation

Dschoordsch
Copy link
Contributor

@Dschoordsch Dschoordsch commented May 30, 2024

Description

Fixes #9774, #9775
Add a template search query to the User object.

Demo

https://www.loom.com/share/114efbf0a1864ba09816c0bf87a28620?sid=86867080-17c0-405a-bf6a-828a591bdfad

Testing scenarios

  • search for templates in the activity library
  • create custom templates, wait for the embedding (ca. 1min, you can check EmbeddingsJobQueue is empty) and search for it

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@Dschoordsch Dschoordsch marked this pull request as draft May 30, 2024 11:41
Base automatically changed from feat/9772/createMeetingTemplateEmbeddings to master June 13, 2024 09:27
@Dschoordsch Dschoordsch force-pushed the feat/9774/templateSearchMutation branch from 48c7007 to 17235e3 Compare June 24, 2024 15:35
The tests only work once the embeddings are calculated, which does not
happen on CI.
@Dschoordsch Dschoordsch marked this pull request as ready for review June 25, 2024 10:44
@@ -224,13 +258,29 @@ export const ActivityLibrary = (props: Props) => {
onQueryChange,
resetQuery
} = useSearchFilter(templates, getTemplateDocumentValue)
const [debouncedSearchQuery] = useDebounce(searchQuery, 500)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tianrunhe I changed the debounced query logic. Because this is used for analytics, let me know if that's ok.

@nickoferrall
Copy link
Contributor

Hey @Dschoordsch, I'm getting a timeout error when using the semantic search: https://www.loom.com/share/11e27dce1ba747829b205b87f090a932

Is there any set-up required to test this one?

 rejection:
17:13:43 1|Socket Server     | Error: TIMEOUT
17:13:43 1|Socket Server     |     at Timeout._onTimeout (/Users/nickoferrall/parabol/dev/web.js:80818:16)
17:13:43 1|Socket Server     |     at listOnTimeout (node:internal/timers:573:17)
17:13:43 1|Socket Server     |     at process.processTimers (node:internal/timers:514:7)
17:14:25 2|GraphQL Executor  | TypeError: fetch failed
17:14:25 2|GraphQL Executor  |     at fetch (/Users/nickoferrall/parabol/node_modules/undici/index.js:112:15)
17:14:25 2|GraphQL Executor  |     at async Object.templateSearch (/Users/nickoferrall/parabol/dev/gqlExecutor.js:36867:22)
17:14:25 2|GraphQL Executor  | Trace: {
17:14:25 2|GraphQL Executor  |   error: '{"data":null,"errors":[{"message":"fetch failed","locations":[{"line":1,"column":575}],"path":["viewer","templateSearch"]}]}'
17:14:25 2|GraphQL Executor  | }

Copy link
Contributor

@tianrunhe tianrunhe left a comment

Choose a reason for hiding this comment

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

LGTM metrics side

@Dschoordsch
Copy link
Contributor Author

I'm getting a timeout error when using the semantic search

I tried to reproduce this, but had no success yet:

  • when no embedder is running and the embedding table is empty, I don't get a result but no error either
  • when I stop the text-embeddings-inference container, I get an error in fetch but a different one
  • I tried querying right after startup when the text-embeddings-inference container could be busy, but did not get any error

@nickoferrall Are you processing lots of data atm? Can you check your EmbeddingsJobQueue table how many entries you have?

@nickoferrall
Copy link
Contributor

My EmbeddingsJobQueue had over 100 entries, and my Embeddings_ember_1 table had no entries. I removed all of the entries in the job queue, but got the same result.

Are these env vars the same as .env.example for you?

@nickoferrall
Copy link
Contributor

@Dschoordsch after our call, I removed everything from Docker, restarted my laptop, cleared the job queue and metadata a couple of times, but no luck, unfortunately. The embeddings service is still slow to respond so I get a timeout.

We don't know whether it shouldn't be working for you, or it should be working for me, so perhaps someone else could test this PR so we can isolate the problem?

Could @tianrunhe test it as you're here please?

@tianrunhe
Copy link
Contributor

My EmbeddingsJobQueue had over 100 entries, and my Embeddings_ember_1 table had no entries. I removed all of the entries in the job queue, but got the same result.

I encountered the same behavior as Nick. The stateMessage field for the rows are unable to get tokens:

@Dschoordsch
Copy link
Contributor Author

Dschoordsch commented Jun 26, 2024

Could one of you try deleting the dev folder

rm -r dev

and the embedding data

delete from "Embeddings_ember_1";
delete from "EmbeddingsMetadata";
delete from "EmbeddingsJobQueue";

and then try again?

@Dschoordsch
Copy link
Contributor Author

@nickoferrall ☝️ could you try one more time? I noticed that locally I have some issues with the embedding service not always rebuilding.

@nickoferrall
Copy link
Contributor

I tried the step above, but no luck.

I'm working on getting the text-embeddings-inference server install on bare metal here following this now

@nickoferrall
Copy link
Contributor

I got the embedder up and running on my m1, but it’s still not working 😞

The job queue is empty as is the Embeddings_ember_1 table

@Dschoordsch
Copy link
Contributor Author

I will go ahead and merge the change. If it fails, it will just yield no results, which is safe. As embeddings work in production, I'm positive the search will work there.

@Dschoordsch Dschoordsch merged commit 486f670 into master Jul 1, 2024
7 checks passed
@Dschoordsch Dschoordsch deleted the feat/9774/templateSearchMutation branch July 1, 2024 08:25
// all team ids which could have accessible templates
const allTeamIds = ['aGhostTeam', ...allOrgTeams.map(({id}) => id)]

const response = await fetch('http://localhost:3040/embed', {
Copy link
Contributor

Choose a reason for hiding this comment

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

-1 this should be a env variable instead. I'm now using port 3041 for the text-embeddings-inference server because I'm running on my mac rather than Docker

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, no localhost or port should be hardcoded into our code as each component can run in any port and location in other environments.

@github-actions github-actions bot mentioned this pull request Jul 2, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AL template search: mutation
4 participants