-
-
Notifications
You must be signed in to change notification settings - Fork 12
[Store] Add Meilisearch #105
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Guikingone, thanks for jumping on that and providing the first commits :) 👍
In general it looks good already and i think going with the API directly is a good and lightweight idea 👍
I left a couple of comments and was wondering, if we can ease the usage of the example here. were you using the cloud or a docker container of meilisearch?
I think, if reasonable to you as well, it would be great to extend the docker compose setup of the example to bring in meilisearch there.
and please add docs and a test for the Store class
156caae
to
a41118c
Compare
a41118c
to
2827164
Compare
Hi @chr-hertel 👋🏻 The PR is fixed, the CI fails due to a zip error (not on unit tests side), the store is improved and tested 🙂 |
Thanks for the update and especially the docker changes - now I can easily run it, but I get this error while executing the example:
i'm just using the default values you provided with the |
Ah yes, forgot to call |
Yes, that works better - now I'm getting this error:
which refers to this: https://github.com/symfony/ai/pull/105/files#diff-969e7ab9dcae7946fdb6f82fcc866bb86e4389e23e3d40b752e3976de997dd09R58 after fixing this, i get the next one:
Looks like I'm doing something wrong while running the example or is it also failing for you? |
Ah yes, forgot about the initialization of an embedder in |
@Guikingone so the provided example runs locally for you and is functional? |
Yes, works locally (I failed to see the indexes embeddings as my local indexes were already configured) 🙂 However, I can't run the script in
How are you running this file? Any error on the autoload? 🤔 |
And the underlying issue that OpenAI returns is
Due to the fact that the vectors are part of the payload. Might be rather a design flaw of the Was just wondering if it's only me, or if the example works for you |
Ah, I know that one, that should've been documented, sorry - see #112 The issue is that Composer is installing the dependencies from main currently and therefore your new class is not available. Meaning, after |
Oh, and that's blocked by #108 🙈 - a bit rough here still :D |
#[CoversClass(Store::class)] | ||
final class StoreTest extends TestCase | ||
{ | ||
public function testStoreCannotInitializeOnInvalidResponse(): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove all void
return types in tests please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but why @OskarStark?
it looks like it's common in symfony/symfony
but that also supports other versions of PHP and PHPUnit I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, not to mention that PHPStan doesn't like when return types are not specified 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just, that it does not add any value in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I get that there is no functional value, but same is true for the public
keyword for test methods.
I get that in combination with supporting various versions and having merge conflicts for nothing but coding best practices is annoying for symfony/symfony
- but we don't have that struggle here. so for me adding return type declarations is a best practice to opt-out for reasons, but i see no reason here.
i'd vote for keeping them in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it
Hi 👋🏻
As explained in #93, this PR aim to add
meilisearch
as a vector store, the full integration is not finished (tests to be validated) but the main implementation is ready.