Support `embedders` setting #554

CommanderStorm · 2024-03-02T15:42:36Z

Pull Request

Related issue

Fixes #541
Fixes #612
Fixes #621
Fixes #646

What does this PR do?

Adds the required settings
- with_embedders does use the same "API" (not using impl AsRef for items passed) as with_synonyms, as this is the closest existing
- given set_embedders has not been implemented upstream (at least when I try to PATCH the object, it does not work)
- only {get,reset}_embedders settings have been implemented.
  Said implementation goes with the work done in Implement vector search experimental feature v2 (v1.6) meilisearch-python#924
adds the hybrid field to search via the vector search to add an end-to-end test of this feature with the huggingface configuration.

userProvided seens more brittle, but we may want change to this instead
using userProvided instead would mean (at the cost of hardcoding stuff)
=> lower cpu effort
=> no higher timeout necceeseary
=> aligning with meilisearch/meilisearch to only have a test for userProvided)

TODO:

find a combination of semantic search model + configuration that does not fail the assumptions (see search testcase) spectacularly

PR checklist

Please check if your PR fulfills the following requirements:

Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
Have you read the contributing guidelines?
Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

curquiza · 2024-04-15T17:09:08Z

Hello @CommanderStorm

Can you rebase your branch? We made changes recently to improve the library

Sorry for the inconvenience, I try to review your PR as soon as possible after the rebase

CommanderStorm · 2024-04-16T03:48:59Z

Can you rebase your branch?

Done ^^

irevoire

Hey @CommanderStorm,

Code-wise, the PR is very nice and well-documented; I love it!

But I’m hitting way too many timeouts to really help you; sorry 😖

I don’t think we’ll be able to merge this PR with tests that take about 10 minutes. Could you mock Meilisearch as we did here:

meilisearch-rust/src/tasks.rs

Lines 902 to 924 in cb32534

    
           async fn test_get_tasks_with_params() -> Result<(), Error> { 
        
               let mut s = mockito::Server::new_async().await; 
        
               let mock_server_url = s.url(); 
        
               let client = Client::new(mock_server_url, Some("masterKey")).unwrap(); 
        
               let path = 
        
                   "/tasks?indexUids=movies,test&statuses=equeued&types=documentDeletion&uids=1&limit=0&from=1"; 
        
               let mock_res = s.mock("GET", path).with_status(200).create_async().await; 
        
               let mut query = TasksSearchQuery::new(&client); 
        
               query 
        
                   .with_index_uids(["movies", "test"]) 
        
                   .with_statuses(["equeued"]) 
        
                   .with_types(["documentDeletion"]) 
        
                   .with_from(1) 
        
                   .with_limit(0) 
        
                   .with_uids([&1]); 
        
               let _ = client.get_tasks_with(&query).await; 
        
               mock_res.assert_async().await; 
        
               Ok(())

That way, we simply ensure that meilisearch-rust sends a valid payload and hope meilisearch works as expected.

userProvided seens more brittle, but we may want change to this instead

Or we could do that and actually send the payload to meilisearch.
Or do both; let me know what you prefer, but I would very much like the tests to not take that much time to run 😖

Where in the Meilisearch codebase could I find an e2e-test how to use feature?

I don’t think there is. I believe you’re right, and we only wrote tests for user-provided vectors

introduces the experimental-vector-search-feature flag

I don’t know if this is required, @curquiza. Could we simply expose the feature as-is instead of hiding it behind a feature flag (that will make it harder to test and use).
Since it doesn’t add any dependency, I don’t see much point in putting it behind a feature flag

src/search.rs

curquiza · 2024-04-17T07:29:53Z

I don’t know if this is required, @curquiza. Could we simply expose the feature as-is instead of hiding it behind a feature flag (that will make it harder to test and use).

Yes, that's ok not to do feature flag 😊

CommanderStorm · 2024-04-17T16:17:48Z

I did a few tests, and on my side it never took 120s, the quickest execution took 150s but most of the time I was over 200s

Time after being migrated to userProvided seems fine in CI (this is likely the slowest machine running the tests). ✅

Or we could do that and actually send the payload to meilisearch.

I think keeping the testcases from requiring a active internet connection is better as otherwise the test might be unnessesarily flaky in CI.

Could you mock Meilisearch as we did here

I can add a testcase where I mock the routes.
I am unsure if you actually would want to mock this for an experimental feature (= where the api might change => requiring changes)

I have tweaked a bunch with the vectors available and can't get the test_hybrid testcase to work without fully using the same dataset as in tests/search/hybrid.rs.

It seems to always return everything when I set semantic_ratio=1.0...
Is this operating as intended?

I have tried similar 2D vectors as the upstream test and went as far as to use 1D vectors with considerable (1/10/1k/1m) spread (=>something that as far as I understand embeddings should not match)

=> I am missing something. 😅
Could you point me into the right direction?

Note

I can obviously steal the testcase from tests/search/hybrid.rs, but for longer-term maintainability I would like this testcase to not be such an oddball compared to the existing testcases in this repo ^^

curquiza · 2024-07-01T12:39:37Z

@CommanderStorm really sorry for this.
Can you fix the git conflicts? 😊

CommanderStorm · 2024-07-01T16:44:03Z

Thanks for the pinging (github does not give me notifications for this) ^^
No need to worry. If this takes a few months that is fine.

src/search.rs

curquiza

@CommanderStorm thank you again! Can you add the rest and ollama models we added in v1.8.0?
Sorry for the late notice again!
And thank you for your involvement 🙏

irevoire · 2024-07-03T14:58:37Z

Hey @CommanderStorm, since we introduced a new rest embedder, I think we could write better tests by mocking the rest embedder and ensuring it works with meilisearch; here's some example I wrote earlier this week in meilisearch: meilisearch/meilisearch@2141cb3

CommanderStorm · 2024-08-24T18:27:30Z

I think I have adressed the requested changes ^^
Sorry for the long delay, I was on vacation and then a bit buisy with other work. Hope you understand.

AND we should absolutely remove these two tests

I have only removed the test which disables the vector store, but have kept the test which .set_vector_store(true)'s.
I have also improved a test a bit to also set, but this should be minor ^^

Next time, I will split such PRs earlier ^^

src/settings.rs

CommanderStorm

This updates the PR to the breaking 1.11 changes.

src/settings.rs

CommanderStorm · 2024-11-17T20:24:42Z

binaryQuantized will have to be in a different PR, as according to the docs this is only avaliable for updating via PATCH, which is currently not supported.

Docs for this feature: https://meilisearch.notion.site/Binary-quantization-usage-v1-11-2a9c9559461a4a9d9fa3e0ea5149ad68

CommanderStorm · 2024-11-17T20:29:11Z

@irevoire sorry to ping you.
I think it would be much more maintainable to do this work in different PRs.

Is merging this feature in the experimental stage still something you are interested in, or would this feature need to be stabilised first?

src/search.rs

CommanderStorm changed the title ~~Vector search embedder~~ [v1.6] support embedders setting Mar 2, 2024

CommanderStorm marked this pull request as ready for review March 2, 2024 19:52

CommanderStorm force-pushed the vector-search-embedder branch from 1076241 to 8ffb555 Compare March 11, 2024 11:42

CommanderStorm force-pushed the vector-search-embedder branch 2 times, most recently from 51820a9 to 8ade09d Compare April 16, 2024 03:48

irevoire requested changes Apr 16, 2024

View reviewed changes

src/search.rs Outdated Show resolved Hide resolved

src/search.rs Outdated Show resolved Hide resolved

src/search.rs Outdated Show resolved Hide resolved

CommanderStorm force-pushed the vector-search-embedder branch from 0285e62 to 9033232 Compare April 17, 2024 15:37

CommanderStorm added 4 commits April 17, 2024 18:18

Added the ability to configure vector-search embeddings in the settings

333c79a

Added the ability to configure the hybrid keyword in the search

83f543d

Migrated the testcase to use _vectors instead

a4e50c9

Removed the experimental-vector-search feature

0e044b1

CommanderStorm force-pushed the vector-search-embedder branch from 9033232 to 0e044b1 Compare April 17, 2024 16:18

curquiza requested a review from irevoire May 14, 2024 09:25

curquiza and others added 2 commits May 14, 2024 14:45

Merge branch 'main' into vector-search-embedder

3a4d876

Merge branch 'main' into vector-search-embedder

a167c50

Merge branch 'main' into vector-search-embedder

37b5410

CommanderStorm commented Jul 1, 2024

View reviewed changes

src/search.rs Outdated Show resolved Hide resolved

Merge branch 'main' into vector-search-embedder

4c7475e

curquiza requested changes Jul 2, 2024

View reviewed changes

NoodleSamaChan mentioned this pull request Jul 2, 2024

getting similar documents #600

Draft

3 tasks

CommanderStorm added 3 commits July 8, 2024 03:08

feat: added support for ollama and rest

5e72f98

chore: improved the documentation

75e5585

feat: implemnted the retrieve_vectors flag

464de44

CommanderStorm requested a review from irevoire August 24, 2024 18:23

CommanderStorm commented Aug 26, 2024

View reviewed changes

src/settings.rs Show resolved Hide resolved

fix: changed the rest embedder to the 1.10.0 schema

7398185

CommanderStorm mentioned this pull request Aug 29, 2024

[v1.10.0] AI-powered search changes #612

Closed

6 tasks

feat: added headers support

93b0bca

CommanderStorm requested a review from curquiza October 3, 2024 18:18

CommanderStorm commented Nov 17, 2024

View reviewed changes

src/settings.rs Outdated Show resolved Hide resolved

src/settings.rs Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

src/settings.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

53083ed

CommanderStorm added 3 commits January 3, 2025 18:03

Merge branch 'main' into vector-search-embedder

9079410

chore: formatting fix

a0b13c6

Merge branch 'main' into vector-search-embedder

5c406e3

CommanderStorm mentioned this pull request Jan 24, 2025

Changes related to Meilisearch v1.13.0 meilisearch/integration-guides#314

Open

90 tasks

Remove mentions to the experimental search from the PR

9cd547d

CommanderStorm commented Jan 24, 2025

View reviewed changes

src/search.rs Outdated Show resolved Hide resolved

CommanderStorm added 3 commits January 24, 2025 22:37

Fix typos during the initial drafts

f32abe2

Merge branch 'main' into vector-search-embedder

b64e424

fix formatting issues that crept in somehow in the merge

279e974

CommanderStorm commented Feb 5, 2025

View reviewed changes

src/search.rs Outdated Show resolved Hide resolved

CommanderStorm added 3 commits February 17, 2025 21:55

Merge branch 'main' into vector-search-embedder

fb45587

Merge branch 'main' into vector-search-embedder

b0bd2a7

Update src/search.rs

73305ac

CommanderStorm commented Feb 20, 2025

View reviewed changes

src/search.rs Outdated Show resolved Hide resolved

simplified code a bit

b7735a2

CommanderStorm mentioned this pull request Feb 20, 2025

[v1.9] Get similar documents #596

Open

4 tasks

CommanderStorm added 3 commits February 20, 2025 18:47

update docs with newer phrasing

8e265df

formatting fix

20c4d50

fix doctests

6838937

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `embedders` setting #554

Support `embedders` setting #554

CommanderStorm commented Mar 2, 2024 •

edited

Loading

curquiza commented Apr 15, 2024 •

edited

Loading

CommanderStorm commented Apr 16, 2024 •

edited

Loading

irevoire left a comment

curquiza commented Apr 17, 2024

CommanderStorm commented Apr 17, 2024 •

edited

Loading

curquiza commented Jul 1, 2024

CommanderStorm commented Jul 1, 2024

curquiza left a comment

irevoire commented Jul 3, 2024

CommanderStorm commented Aug 24, 2024

CommanderStorm left a comment

CommanderStorm commented Nov 17, 2024 •

edited

Loading

CommanderStorm commented Nov 17, 2024

	async fn test_get_tasks_with_params() -> Result<(), Error> {
	let mut s = mockito::Server::new_async().await;
	let mock_server_url = s.url();
	let client = Client::new(mock_server_url, Some("masterKey")).unwrap();
	let path =
	"/tasks?indexUids=movies,test&statuses=equeued&types=documentDeletion&uids=1&limit=0&from=1";

	let mock_res = s.mock("GET", path).with_status(200).create_async().await;

	let mut query = TasksSearchQuery::new(&client);
	query
	.with_index_uids(["movies", "test"])
	.with_statuses(["equeued"])
	.with_types(["documentDeletion"])
	.with_from(1)
	.with_limit(0)
	.with_uids([&1]);

	let _ = client.get_tasks_with(&query).await;

	mock_res.assert_async().await;

	Ok(())

Support embedders setting #554

Are you sure you want to change the base?

Support embedders setting #554

Conversation

CommanderStorm commented Mar 2, 2024 • edited Loading

Pull Request

Related issue

What does this PR do?

PR checklist

curquiza commented Apr 15, 2024 • edited Loading

CommanderStorm commented Apr 16, 2024 • edited Loading

irevoire left a comment

Choose a reason for hiding this comment

curquiza commented Apr 17, 2024

CommanderStorm commented Apr 17, 2024 • edited Loading

curquiza commented Jul 1, 2024

CommanderStorm commented Jul 1, 2024

curquiza left a comment

Choose a reason for hiding this comment

irevoire commented Jul 3, 2024

CommanderStorm commented Aug 24, 2024

CommanderStorm left a comment

Choose a reason for hiding this comment

CommanderStorm commented Nov 17, 2024 • edited Loading

CommanderStorm commented Nov 17, 2024

Support `embedders` setting #554

Support `embedders` setting #554

CommanderStorm commented Mar 2, 2024 •

edited

Loading

curquiza commented Apr 15, 2024 •

edited

Loading

CommanderStorm commented Apr 16, 2024 •

edited

Loading

CommanderStorm commented Apr 17, 2024 •

edited

Loading

CommanderStorm commented Nov 17, 2024 •

edited

Loading