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: hybrid search improvements for v1.8.x #1647

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

mdubus
Copy link
Member

@mdubus mdubus commented Apr 23, 2024

Pull Request

Related issue

Fixes #1641

What does this PR do?

  • Removal of the vector and _semanticScore fields from the SearchResponse
  • Addition of a new distribution field to all embedders
  • Addition of 2 new embedders: REST and Ollama
  • Update failing tests & snapshots
  • Addition of tests + missing tests

⚠️ CI failing

CI fails because of 2 things:

Once those 2 things are fixed, it should be fine 🙌

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!

@mdubus mdubus force-pushed the hybrid-search-improvements branch from 363d8ab to 8d368d6 Compare April 23, 2024 09:33
expect(response).toEqual({
default: {
...newEmbedder.default,
apiKey: '<yoXXXXX...',
Copy link
Member Author

Choose a reason for hiding this comment

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

apiKey is now hidden 🙈 🥳

Comment on lines +476 to +481
expect(response).toHaveProperty('hits')
expect(response).toHaveProperty('semanticHitCount')
// Those fields are no longer returned by the search response
// We want to ensure that they don't appear in it anymore
expect(response).not.toHaveProperty('vector')
expect(response).not.toHaveProperty('_semanticScore')
Copy link
Member Author

Choose a reason for hiding this comment

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

I test that I retrieve the hits and semanticHitCount fields correctly.
And since the vector and _semanticScore fields have been removed from the response, I check that I don't retrieve them anymore

Comment on lines +484 to +488
test(`${permission} key: search without vectors`, async () => {
const client = await getClient(permission)
const response = await client.index(index.uid).search('prince', {})

expect(response).not.toHaveProperty('semanticHitCount')
Copy link
Member Author

Choose a reason for hiding this comment

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

Creation of this specific test to make sure that with a "non-vector" search, I don't retrieve the semanticHitCount field in the response

@@ -146,6 +149,41 @@ describe.each([{ permission: 'Master' }, { permission: 'Admin' }])(
expect(response).toMatchSnapshot()
})

test(`${permission} key: Update embedders settings `, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Creation of a new test to test the embedder's addition on the settings route (it was forgotten previously).
Since it needs the activation of the vectorStore exp. feature, I thought it would be best to create a specific test for this one

@@ -183,6 +221,29 @@ describe.each([{ permission: 'Master' }, { permission: 'Admin' }])(
expect(response).toMatchSnapshot()
})

test(`${permission} key: Reset embedders settings `, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Creation of a new test to test the embedders deletion on the settings route

@mdubus mdubus force-pushed the hybrid-search-improvements branch from 8d368d6 to 94b1875 Compare April 23, 2024 09:46
@mdubus mdubus added the enhancement New feature or request label Apr 23, 2024
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.

LGTM!

Merging!

@curquiza curquiza merged commit 4e5e70b into bump-meilisearch-v1.8.0 Apr 23, 2024
4 of 6 checks passed
@curquiza curquiza deleted the hybrid-search-improvements branch April 23, 2024 17:26
@curquiza curquiza mentioned this pull request Apr 29, 2024
9 tasks
meili-bors bot added a commit that referenced this pull request May 6, 2024
1648: Update version for the next release (v0.39.0) r=brunoocasali a=meili-bot

## 🚀 Enhancements

* feat: hybrid search improvements for v1.8.x (#1647) `@mdubus`
* Add `null` to Embedder type (#1646) `@amit-ksh`
* Add searchCutoffMs index setting (#1643, #1645) `@amit-ksh`
    ```js
    client.index('movies').getSearchCutoffMs()
    client.index('movies').updateSearchCutoffMs(150)
    client.index('movies').resetSearchCutoffMs()
    ```

## ⚙️ Maintenance/misc

* Update ESLint, Prettier, TypeScript and fix/improve their configuration files (#1616) `@flevi29`
* Fix code style after configuration changes (#1638) `@brunoocasali`

Thanks again to `@amit-ksh,` `@brunoocasali,` `@curquiza,` `@flevi29,` `@mdubus,` `@meili-bors[bot]` ! 🎉


Co-authored-by: meili-bot <74670311+meili-bot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants