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

[v1.6] support embedders setting #554

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

Conversation

CommanderStorm
Copy link

@CommanderStorm CommanderStorm commented Mar 2, 2024

Pull Request

Related issue

Fixes #541

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

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

Can you rebase your branch?

Done ^^

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:

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.6] support embedders setting
3 participants