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.

Sign up for GitHub

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

Jump to bottom

Support embedders setting #554

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Support embedders setting #554

wants to merge 20 commits into from

Conversation

CommanderStorm
Copy link

@CommanderStorm CommanderStorm commented Mar 2, 2024 •
edited
Loading

Pull Request

Related issue

Fixes #541
Fixes #612
Fixes #621

What does this PR do?

  • introduces the experimental-vector-search-feature flag

  • 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)

Caution

This is the difficult part:
I have not gotten this feature to work as I expect it.
I have documented what I would expect to happen here:

meilisearch-rust/src/search.rs

Lines 1223 to 1304 in 86334cd

#[cfg(feature = "experimental-vector-search")]
#[meilisearch_test]
async fn test_hybrid(client: Client, index: Index) -> Result<(), Error> {
log::warn!("You are executing the vector search test. This WILL take a while and might lead to timeouts in other tests. You can disable this testcase by not enabling the `experimental-vector-search`-feature and running this ");
// enable vector searching and configure an embedder
let features = crate::ExperimentalFeatures::new(&client)
.set_vector_store(true)
.update()
.await
.expect("could not enable the vector store");
assert_eq!(features.vector_store, true);
let embedder_setting = crate::Embedder::HuggingFace(crate::HuggingFaceEmbedderSettings {
model: "BAAI/bge-base-en-v1.5".into(),
revision: None,
document_template: Some("{{ doc.value }}".into()),
});
let t3 = index
.set_settings(&crate::Settings {
embedders: Some(HashMap::from([("default".to_string(), embedder_setting)])),
..crate::Settings::default()
})
.await?;
t3.wait_for_completion(&client, None, None).await?;
setup_test_index(&client, &index).await?;
// "zweite" = "second" in german
// => an embedding should be able to detect that this is equivalent, but not the regular search
let results: SearchResults<Document> = index
.search()
.with_query("Facebook")
.with_hybrid("default", 1.0) // entirely rely on semantic searching
.execute()
.await?;
assert_eq!(results.hits.len(), 1);
assert_eq!(
&Document {
id: 1,
value: S("dolor sit amet, consectetur adipiscing elit"),
kind: S("text"),
number: 10,
nested: Nested { child: S("second") },
},
&results.hits[0].result
);
let results: SearchResults<Document> = index
.search()
.with_query("zweite")
.with_hybrid("default", 0.0) // no semantic searching => no matches
.execute()
.await?;
assert_eq!(results.hits.len(), 0);
// word that has a typo => would have been found via traditional means
// if entirely relying on semantic searching, no result is found
let results: SearchResults<Document> = index
.search()
.with_query("lohrem")
.with_hybrid("default", 1.0)
.execute()
.await?;
assert_eq!(results.hits.len(), 0);
let results: SearchResults<Document> = index
.search()
.with_query("lohrem")
.with_hybrid("default", 0.0)
.execute()
.await?;
assert_eq!(results.hits.len(), 1);
assert_eq!(
&Document {
id: 0,
value: S("Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum."),
kind: S("text"),
number: 0,
nested: Nested { child: S("first") }
},
&results.hits[0].result.id
);
Ok(())
}

  • Where in the Meilisearch codebase could I find an e2e-test how to use feature?
    I searched a bunch, but could not find any relevant testcases (only userProvided)
  • Am I misunderstanding what the capabilities should be, or do I have a bad HuggingFace-embedder?

Tip

The specific testcase can be executed via

cargo test --package meilisearch-sdk --lib search::tests::test_hybrid  --features=experimental-vector-search -- --exact

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!

@CommanderStorm CommanderStorm changed the title Vector search embedder [v1.6] support embedders setting Mar 2, 2024
@CommanderStorm CommanderStorm marked this pull request as ready for review March 2, 2024 19:52
@curquiza
Copy link
Member

curquiza commented Apr 15, 2024 •
edited
Loading

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 CommanderStorm force-pushed the vector-search-embedder branch 2 times, most recently from 51820a9 to 8ade09d Compare April 16, 2024 03:48
@CommanderStorm
Copy link
Author

CommanderStorm commented Apr 16, 2024 •
edited
Loading

Can you rebase your branch?

Done ^^

irevoire
irevoire requested changes Apr 16, 2024
View reviewed changes
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/search.rs Outdated Show resolved Hide resolved
src/search.rs Outdated Show resolved Hide resolved
@curquiza
Copy link
Member

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
Copy link
Author

CommanderStorm commented Apr 17, 2024 •
edited
Loading

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 curquiza requested a review from irevoire May 14, 2024 09:25
@curquiza
Copy link
Member

curquiza commented Jul 1, 2024

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

@CommanderStorm
Copy link
Author

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.

CommanderStorm
CommanderStorm commented Jul 1, 2024
View reviewed changes
src/search.rs Outdated Show resolved Hide resolved
curquiza
curquiza requested changes Jul 2, 2024
View reviewed changes
Copy link
Member

@curquiza curquiza left a comment

Choose a reason for hiding this comment

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

@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 🙏

@NoodleSamaChan NoodleSamaChan mentioned this pull request Jul 2, 2024
Draft
3 tasks
@irevoire
Copy link
Member

irevoire commented Jul 3, 2024

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
CommanderStorm commented Jul 8, 2024
View reviewed changes
src/search.rs Outdated Show resolved Hide resolved
CommanderStorm
CommanderStorm commented Jul 8, 2024
View reviewed changes
src/search.rs Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Author

The changes requested are implemented.

@curquiza could you please approve a workflow run
(I think I did check everything, but you never know)

In retrospective, with_retrieve_vectors should/could have been a different PR. If you want, I can move those changes to a different PR. 600 lines is pretty unreviewable. On the other side, most of the lines changed are docs..

@CommanderStorm CommanderStorm mentioned this pull request Jul 13, 2024
Draft
2 tasks
@curquiza curquiza changed the title [v1.6] support embedders setting Support embedders setting Jul 25, 2024
@curquiza curquiza added the enhancement New feature or request label Jul 25, 2024
irevoire
irevoire requested changes Jul 29, 2024
View reviewed changes
Copy link
Member

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

Hey @CommanderStorm,

The PR seems really good; I think we're almost ready to merge.

I think you missed one issue in your test; your definition of what a vector is was wrong, and thus, serde was ignoring most vectors when deserializing the documents.
I left a comment on the subject.

To write the test you wanted to write initially, where the vectors appear as None, you should enable the experimental feature first.

This brings me to another point; I think every test using the feature should start by enabling it just in case they run first.

AND we should absolutely remove these two tests:

meilisearch-rust/src/features.rs

Lines 108 to 132 in c673c8d

mod tests {
use super::*;
use meilisearch_test_macro::meilisearch_test;
#[meilisearch_test]
async fn test_experimental_features_get(client: Client) {
let mut features = ExperimentalFeatures::new(&client);
features.set_vector_store(false);
let _ = features.update().await.unwrap();
let res = features.get().await.unwrap();
assert!(!res.vector_store);
}
#[meilisearch_test]
async fn test_experimental_features_enable_vector_store(client: Client) {
let mut features = ExperimentalFeatures::new(&client);
features.set_vector_store(true);
let res = features.update().await.unwrap();
assert!(res.vector_store);
}
}

That will mess with everything

src/search.rs Outdated Show resolved Hide resolved
@CommanderStorm
Copy link
Author

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 ^^

@CommanderStorm CommanderStorm mentioned this pull request Aug 29, 2024
Open
6 tasks
CommanderStorm
CommanderStorm commented Nov 17, 2024
View reviewed changes
Copy link
Author

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

This updates the PR to the breaking 1.11 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
@CommanderStorm
Copy link
Author

CommanderStorm commented Nov 17, 2024 •
edited
Loading

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
Copy link
Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irevoire irevoire Awaiting requested review from irevoire

@curquiza curquiza Awaiting requested review from curquiza

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[v1.11.0] AI search changes [v1.10.0] AI-powered search changes [v1.6] support embedders setting
3 participants
@CommanderStorm @curquiza @irevoire