-
-
Notifications
You must be signed in to change notification settings - Fork 52
[Platform] Meilisearch message store #239
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
Wouldn't it be more related to Chat and the MessageStore? |
Ah yes, I wasn't sure about the interface to use, I'll check the store one. |
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.
PR title and body would need an update please
Yes, I need to finish the test in the same time, I'll push the updated version once it's done 🙂 |
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.
Great idea, thanks! added some minor comments.
I wonder tho if we should move that feature into the store component - WDYT?
@chr-hertel I agree, the Maybe we can move the classes/interfaces after this PR to prevent any bloating of it? |
Agreed 👍 |
What about a dedicated MessageStore component? or too much? |
@OskarStark Well, here's my humble opinion (I don't know how this package was initially designed and what are the "rules" that it must follow to comply with Symfony approach): The Maybe we could find a new "way" to split the |
d4a2fba
to
3ca29bc
Compare
One thing I wonder is about having multiple conversations happening in parallel. Let's say per user. How would that work with this implementation? separate index? |
Actually, yes, multiple index. Another solution might be to use a "session" per agent, the session is initiated by the agent (constructor call, maybe an UUID?) then sent to the message store to "lock" messages per agent. I don't see a big technical bottleneck but IMO, this refactoring must be brought via another PR (I can work on it if you want). |
10e3653
to
07a89f8
Compare
07a89f8
to
707dab5
Compare
$store->initialize(); | ||
|
||
$chat = new Chat($agent, $store); |
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.
What about letting the Chat call initialize internally if it is some kind of initalizeable store?
Just thinking out loud
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.
I'm thinking the same way, the actual API is not built for initializeable store, that's one of the issue I'm facing with #254
private HttpClientInterface $httpClient, | ||
private string $endpointUrl, | ||
#[\SensitiveParameter] private string $apiKey, | ||
private string $indexName, |
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.
Does it make sense to provide a default here?
*/ | ||
private function request(string $method, string $endpoint, array $payload = []): array | ||
{ | ||
$url = \sprintf('%s/%s', $this->endpointUrl, $endpoint); |
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.
Can be inlined
$store = new MessageStore( | ||
$httpClient, | ||
'http://localhost:7700', | ||
'test', | ||
'test', | ||
); |
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.
One line please
Hi 👋🏻
Here's a small POC to store message bags outside of memory (for long term agents), the implementation is built around MS but the main interfaces are defined (Redis, Doctrine, etc, any bridge can use it) for any bridge (and heavily inspired by the
Store
package).Let me know if the idea is interesting, if not, feel free to close the PR 🙂