implement experimental ai-powered search #398

memishood · 2024-12-25T15:59:33Z

Pull Request

Related issue

Fixes #369

What does this PR do?

I've implemented all the required steps mentioned in the issue above. I made every effort to respect the existing project conventions. I hope this pull request will be helpful and add value to Meilisearch 💜

PR: I didn't uncomment vector search tests in search_tests.dart as I'm focused on embedders capability.

Some technical points that I might have considered wrong in the pull-req

Ensure semanticRatio is between 0.0 and 1.0
Throw exception when embedder source is not one of them: openAi, huggingFace, userProvided, rest, ollama

Any comment & feedback is appreciated!

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!

ahmednfwela

Thanks for your great contribution!

I have provided a quick initial review for now, will continue once current changes are solved.

ahmednfwela · 2024-12-27T09:21:29Z

test/utils/client.dart

+String? get openAiKey {
+  return Platform.environment['OPEN_AI_API_KEY'];
+}
+


this will not work if the tests are run on web

I suggest also adding documentation in CONTRIBUTING.md for users who want to run tests to set this env variable

I've updated the code to cover flutter web, and updated the documentation in CONTRIBUTING.md file

ahmednfwela · 2024-12-27T09:23:48Z

lib/src/settings/embedder.dart

+    required this.source,
+  });
+
+  Map<String, Object?> toMap();


since source is shared between all embedders, this can be changed to:

Suggested change

Map<String, Object?> toMap();

Map<String, Object?> toMap() {

'source': source,

}

I've updated the code, now it is mapped only once in the base class as you described

ahmednfwela · 2024-12-27T09:24:40Z

lib/src/settings/embedder.dart

+
+  @override
+  Map<String, Object?> toMap() => {
+        'source': source,


for all embedders: this can now be changed to

Suggested change

'source': source,

...super.toMap(),

ahmednfwela · 2024-12-27T09:26:01Z

lib/src/settings/embedder.dart

+      'userProvided' => UserProvidedEmbedder.fromMap(map),
+      'rest' => RestEmbedder.fromMap(map),
+      'ollama' => OllamaEmbedder.fromMap(map),
+      _ => throw Exception('unexpected embedder source'),


This will be a breaking change if meilisearch adds new embedder versions, so I suggest adding an UnkownEmbedder class that stores the map as is.

Yes, this makes more sense, however, I guess I'll need to update the structure a bit. It seems that I need to either remove source field in the base class or make it nullable as unknown embedder's source field might not be defined.

I've updated the code, now we return unknown embedder with the original data when the source is not handled.

ahmednfwela · 2024-12-27T09:28:34Z

lib/src/settings/embedder.dart

+    this.url,
+    this.documentTemplateMaxBytes,
+    this.binaryQuantized,
+  }) : super(source: 'openAi');


for all embedders: we are typing the source twice, which might lead to typos, once here and once in Embedder.fromMap, we should probably unify this in a static const sourceValue = 'openAi';

I've updated the code, now it is typed only once

ahmednfwela · 2024-12-27T09:30:07Z

lib/src/settings/distribution.dart

+  Map<String, Object?> toMap() => {
+        'mean': mean,
+        'sigma': sigma,
+      };


since this is used in query parameters, it should automatically exclude null values (as per meilisearch conventions), the logic for this is already present in Queryable class, so it can be uesd.

Distribution isn't used in the query parameters; it is a field of some embedders, and the fields in this class aren't actually nullable.

memishood · 2024-12-27T13:38:28Z

Thanks for your great contribution!

I have provided a quick initial review for now, will continue once current changes are solved.

Hi, thanks for the good feedback.

I'll be updating the code based on your comments soon.

…andled

memishood · 2025-01-09T21:49:17Z

@ahmednfwela any update here?

ahmednfwela · 2025-01-10T11:46:37Z

I will do another review now, thanks for your contribution! @memishood

ahmednfwela · 2025-01-10T12:31:34Z

@memishood since the fork belongs to an organization I can't directly push changes to your PR, so can you please merge main into this ?

ahmednfwela · 2025-01-10T21:48:42Z

test/search_test.dart

@@ -590,6 +591,60 @@ void main() {
  //   });
  // });

+  group('Embedders', () {


These tests need to be disabled if the openAI API key was not present, so that CI can pass

sure, i have updated it

codecov · 2025-01-11T00:53:35Z

Codecov Report

Attention: Patch coverage is 24.34211% with 115 lines in your changes missing coverage. Please review.

Project coverage is 75.19%. Comparing base (ec68546) to head (d728b29).
Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/src/settings/embedder.dart	20.51%	93 Missing ⚠️
lib/src/index.dart	0.00%	13 Missing ⚠️
lib/src/query_parameters/hybrid_search.dart	0.00%	5 Missing ⚠️
lib/src/settings/index_settings.dart	66.66%	2 Missing ⚠️
lib/src/query_parameters/index_search_query.dart	0.00%	1 Missing ⚠️
lib/src/query_parameters/search_query.dart	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #398      +/-   ##
==========================================
- Coverage   75.83%   75.19%   -0.64%     
==========================================
  Files          55       60       +5     
  Lines        1320     1524     +204     
==========================================
+ Hits         1001     1146     +145     
- Misses        319      378      +59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…h-dart into pr/memishood/398

ahmednfwela · 2025-01-11T13:21:35Z

I have started a PR of my own suggested edits here: Phosum#1
feel free to give me any comments

improvements to "implement experimental ai-powered search meilisearch#398"

test/search_test.dart

ahmednfwela · 2025-01-11T15:27:11Z

one last change to fix the test and we can merge 👍

Co-authored-by: Ahmed Fwela <[email protected]>

ahmednfwela · 2025-01-11T16:05:33Z

bors merge

398: implement experimental ai-powered search r=ahmednfwela a=memishood # Pull Request ## Related issue Fixes #369 ## What does this PR do? I've implemented all the required steps mentioned in the issue above. I made every effort to respect the existing project conventions. I hope this pull request will be helpful and add value to Meilisearch 💜 **PR:** I didn't uncomment vector search tests in search_tests.dart as I'm focused on embedders capability. ### Some technical points that I might have considered wrong in the pull-req - Ensure `semanticRatio` is between 0.0 and 1.0 - Throw exception when embedder source is not one of them: `openAi`, `huggingFace`, `userProvided`, `rest`, `ollama` **Any comment & feedback is appreciated!** ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Emre Memis <[email protected]> Co-authored-by: ahmednfwela <[email protected]> Co-authored-by: Emre <[email protected]>

meili-bors · 2025-01-11T16:07:36Z

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"1 review requesting changes by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches","status":"422"}

ahmednfwela

LGTM

ahmednfwela · 2025-01-11T16:09:40Z

bors merge

meili-bors · 2025-01-11T16:11:31Z

Build succeeded:

implement experimental ai-powered search

51d76fc

ahmednfwela self-requested a review December 27, 2024 09:17

ahmednfwela requested changes Dec 27, 2024

View reviewed changes

memishood added 3 commits December 27, 2024 16:11

Fix openAiKey for unit tests not working for flutter web

b214ea2

Map the source field only once in the base embedder class

3a51c8b

Return unknown embedder instead of exception when the source is not h…

6fc1e47

…andled

memishood marked this pull request as draft December 27, 2024 14:38

Create static source variables for each embedder

9c2dd85

memishood marked this pull request as ready for review December 27, 2024 14:47

memishood requested a review from ahmednfwela December 27, 2024 14:47

update semantic ratio assert

a3d03d3

memishood force-pushed the feat/embedders branch from d3380ce to a3d03d3 Compare December 31, 2024 13:43

Merge remote-tracking branch 'origin/main' into pr/memishood/398

47cd6c0

Merge branch 'main' into feat/embedders

80421da

ahmednfwela reviewed Jan 10, 2025

View reviewed changes

Comment embedders unit tests

b7f7e2a

memishood requested a review from ahmednfwela January 10, 2025 22:27

ahmednfwela added 2 commits January 11, 2025 11:58

Merge branch 'feat/embedders' of https://github.com/phosum/meilisearc…

0808f83

…h-dart into pr/memishood/398

fixes and improvements

49699f2

ahmednfwela mentioned this pull request Jan 11, 2025

improvements to "implement experimental ai-powered search #398" Phosum/meilisearch-dart#1

Merged

Merge pull request #1 from ahmednfwela/pr/memishood/398

5a9144f

improvements to "implement experimental ai-powered search meilisearch#398"

ahmednfwela reviewed Jan 11, 2025

View reviewed changes

test/search_test.dart Outdated Show resolved Hide resolved

Update test/search_test.dart

d728b29

Co-authored-by: Ahmed Fwela <[email protected]>

ahmednfwela self-requested a review January 11, 2025 16:09

ahmednfwela approved these changes Jan 11, 2025

View reviewed changes

meili-bors bot merged commit be0ecb9 into meilisearch:main Jan 11, 2025
6 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement experimental ai-powered search #398

implement experimental ai-powered search #398

memishood commented Dec 25, 2024

ahmednfwela left a comment

ahmednfwela Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024 •

edited

Loading

memishood Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024

ahmednfwela Dec 27, 2024

memishood Dec 27, 2024

memishood commented Dec 27, 2024

memishood commented Jan 9, 2025

ahmednfwela commented Jan 10, 2025

ahmednfwela commented Jan 10, 2025

ahmednfwela Jan 10, 2025

memishood Jan 10, 2025

codecov bot commented Jan 11, 2025 •

edited

Loading

ahmednfwela commented Jan 11, 2025

ahmednfwela commented Jan 11, 2025

ahmednfwela commented Jan 11, 2025

meili-bors bot commented Jan 11, 2025

ahmednfwela left a comment

ahmednfwela commented Jan 11, 2025

meili-bors bot commented Jan 11, 2025

implement experimental ai-powered search #398

implement experimental ai-powered search #398

Conversation

memishood commented Dec 25, 2024

Pull Request

Related issue

What does this PR do?

Some technical points that I might have considered wrong in the pull-req

PR checklist

ahmednfwela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memishood Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

memishood commented Dec 27, 2024

memishood commented Jan 9, 2025

ahmednfwela commented Jan 10, 2025

ahmednfwela commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2025 • edited Loading

Codecov Report

ahmednfwela commented Jan 11, 2025

ahmednfwela commented Jan 11, 2025

ahmednfwela commented Jan 11, 2025

meili-bors bot commented Jan 11, 2025

ahmednfwela left a comment

Choose a reason for hiding this comment

ahmednfwela commented Jan 11, 2025

meili-bors bot commented Jan 11, 2025

memishood Dec 27, 2024 •

edited

Loading

codecov bot commented Jan 11, 2025 •

edited

Loading