-
-
Notifications
You must be signed in to change notification settings - Fork 102
[AI Bundle][Agent][Demo][Chat] Add message store session support #254
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
I'd say it's not the agent, that needs the ID, but the MessageBag. The message bag represents the conversations of a specific user/interaction. It is the subject we want to persist. |
Agree but you need to define the id of the agent at the agent level then pass it into the message bag, without it, you'll be not able to "tie together" both the agent and the messages 🤔 |
@Guikingone but you're referring to differentiating various agent configurations not instances of the same agent per User, right? Would a name of the agent solve that concern? |
dba7145
to
f753481
Compare
My POV is the following:
Regarding the technical implementation, to be fair, I have two approaches in mind:
The first approach seems "the way to do it" in my mind, each agent is unique, we might want to reuse messages linked to this one, it should "store" its identifier. The second one is tied to the chat in a way that any agent that "connect" to this chat can retrieve the chat history, IMO, that's an issue as we sometimes prefer to link to a specific agent (due to messages concerns, etc). Either way, I just pushed a "new version" of the POC, feel free to explore it (it doesn't change the "public API") 🙂 |
Why this doesn't make sense to me is because I still see the On top we already have a state object, that we can persist and hydrate - which is the |
Ah, got it, didn't thought about the kernel reboot (especially during container compilation), I was focused on the approach via the examples, not the "framework usage" one. Of course, it make sense now, I was wrong, completely wrong, I'll check the idea of having the UUID at the bag level, it should be easier to handle in the store, etc. Sorry for the dead end discussion, wasn't on the "right path" 🙁 |
@Guikingone nooo, all good, you're on a streak anyhow :D |
f753481
to
46c9bcc
Compare
@chr-hertel I just pushed an update with your ideas implemented along with the configuration using the bundle, the tests are not working (due to various issues on the mock usage) but If you have time to see if it fits your vision of the "identifier -> chat -> agent" usage, I'll be glad to have feedbacks on it 🙂 |
I like this way more now, thanks 👍 The only thing that turns be off at this point is the state awareness in the I'm fine merging this when the pipeline is green - just sharing my concerns in case it resonates with you and you have an idea. Thanks already! |
Hum, a solution might be to use an identifier at the If we move the identifier to |
what do you think about this? $agent = new Agent($platform, $llm, logger: logger());
$store = new InMemoryStore();
$chat = new Chat($agent, $store);
$messagesOne = new MessageBag(
Message::forSystem('You are a helpful assistant. You only answer with short sentences.'))
);
$messagesTwo = clone $messagesOne; // copies the messages but changes the ID
$chat->initiate($messagesOne);
// Calling "$chat->initiate($messagesOne);" should throw an exception "already initiated" or something - meaning present in the store
$chat->initiate($messagesTwo);
$firstMessage = $chat->submit($messagesOne->getId(), Message::ofUser('My name is Christopher.'));
$secondMessage = $chat->submit($messagesTwo->getId(), Message::ofUser('My name is Christopher.'));
echo $firstMessage->content.\PHP_EOL;
echo $secondMessage->content.\PHP_EOL; so basically extending the |
Hum, not convinced about the final API (especially the fact that we need to pass the
There could be an alternative solution, it's just an idea, open to debate:
Actually, we're allowing users to define a key for each store instances while storing vectors, this approach allows to keep the "chain of vectors" even if the container is cleared / built again, if my store is defining a key If a user wants to use the same "implementation" but for another agent, he can define a new message store at the configuration level and "split" the storage for each agent, we're already doing this for vectors so IMHO, that's not a "technical issue" but a "design / developer experience" one 🤔 |
a) next to the name the comparison with ParameterBag doesn't work for me - we're not talking VO here, but a persistent object - compare it to an entity. b) the chat is an integration service - it takes an agent and a store and some state. it should not track and have state itself. but will give it another read and thought later. maybe this feature isn't worth the trouble as well ... would rather see more sophisticated pattern in the agent component 🤔 |
Well,
Yes, agree, that's why I talked about the id at the message store level (like vector stores), I found the chat approach weird while writing the comment 😅 |
I thinking is, how would you otherwise ensure the correct order of the stored messages? Edit: hmm the id is time based, so it is possible 🤔 |
I've discuss with @Guikingone, because I've a use case preparing my talk for API Platform Con, here's my thoughts:
Conceptually a Chat is a couple, consisting of an Setting up the Chat would consist of passing an Agent, and a MessageStore, but to be able to retrieve an existing MessageBag, the MessageStore needs the identifier of this specific MessageBag (exactly like an Entity in a database, using Doctrine ORM). Well, here's my suggestion:
final readonly class Chat implements ChatInterface
{
public function __construct(
private AgentInterface $agent,
private MessageStoreInterface $store,
private string $storeKey,
) {
}
public function initiate(MessageBagInterface $messages, ?string $storeKey = null): void
{
// Using symfony/uid or any other way to generate a unique identifier.
$this->storeKey ??= $storeKey ?? (string) Uuid::v7();
$this->store->clear();
$this->store->save($messages, $this->storeKey);
}
// Should be added in the ChatInterface
public function getStoreKey(): string
{
return $this->storeKey;
}
public function submit(UserMessage $message): AssistantMessage
{
$messages = $this->store->load($this->storeKey);
$messages->add($message);
$result = $this->agent->call($messages);
\assert($result instanceof TextResult);
$assistantMessage = Message::ofAssistant($result->getContent());
$messages->add($assistantMessage);
$this->store->save($messages, $this->storeKey);
return $assistantMessage;
}
} In interface MessageStoreInterface
{
public function save(MessageBagInterface $messages, ?string $key = null): void;
public function load(?string $key = null): MessageBagInterface;
public function clear(?string $key = null): void;
public function setKey(string $key): void;
} This way, the responsibility of the Is this design looks ok for you? @Guikingone @chr-hertel @OskarStark EDIT: If developers want to use directly |
Alright, maybe it's easier to find a common ground here when we talk about user land code, for example a controller using the chat. And let's imagine we're not using the We have a route that expects a submitted message of a user and returns the agents response - chat & message store making sure it is persistent. We expect the chat already to be initialized before (or maybe that's something on demand based on config 🤔) Version A: StoreId + Chat Injected class MyChatController
{
public function __construct(
private ChatInterface $chat,
) {
}
#[Route(...)]
public function __invoke(Request $request): JsonResponse
{
$message = $request->get('message');
$storeId = $request->getSession()->get('store_id');
// making sure it is the right storage used
$chat->setStoreId($someId);
$response = $chat->submit($message);
return new JsonResponse($response);
}
} B: StoreId + Chat Constructed class MyChatController
{
public function __construct(
private AgentInterface $agent,
private MessageStoreInterface $store,
) {
}
#[Route(...)]
public function __invoke(Request $request): JsonResponse
{
$message = $request->get('message');
$storeId = $request->getSession()->get('store_id');
$chat = new Chat($this->agent, $this->store, $storeId);
$response = $chat->submit($message);
return new JsonResponse($response);
}
} C: MessageBagId in class MyChatController
{
public function __construct(
private ChatInterface $chat,
) {
}
#[Route(...)]
public function __invoke(Request $request): JsonResponse
{
$message = $request->get('message');
$messageBagId = $request->getSession()->get('message_bag_id');
$response = $chat->submit($messageBagId, $message);
return new JsonResponse($response);
}
} D: MessageStore Magic (tbd) class MyChatController
{
public function __construct(
private ChatInterface $chat,
) {
}
#[Route(...)]
public function __invoke(Request $request): JsonResponse
{
$message = $request->get('message');
$response = $chat->submit($message);
return new JsonResponse($response);
}
} So with A) we create a stateful service that users would need to handle with care, with B) we introduce basically another state object on top of the MessageBag with service dependencies, and with C) we make the contract less sexy. So we still have D) find a way that message store implementations would handle that mess for us 🤔 (Please, if you have better examples to optimize for - bring 'em on! 🙏) |
Regarding the IMHO, a That's why the approach explained by @welcoMattic seems "a good start" on my side, you're defining the key while configuring the chat then "if and only if" you need to override it, you can do it thanks to If you're using a IMHO, C) approach is weird, adding the "id" of messages while submitting them looks strange, you need to define it at runtime and/or wait for it (Request, etc) before submitting the message. I would love to see the D) approach but I'm completely honest, I don't know how to do it without creating a stateful object either via My approach would be either to side with @welcoMattic (without the getter from |
To be explicit, here is my use case (simplified): <?php
namespace App\MessageHandler;
#[AsMessageHandler]
class ProcessAiResponseMessageHandler
{
public function __construct(
private readonly AgentInterface $agent,
private readonly MessageStoreInterface $messageStore,
private readonly ConversationRepository $conversationRepository,
) {
}
public function __invoke(ProcessAiResponseMessage $message): void
{
$conversation = $this->conversationRepository->find($message->conversationId);
$bagId = md5('conversation/' . $conversation->getId());
$chat = new Chat($this->agent, $this->messageStore, $bagId);
$assistantMessage = $chat->submit(Message::ofUser($message->userMessage));
// continue processing
}
}
ChatInterface.php interface ChatInterface
{
public function initiate(MessageBagInterface $messages, ?string $bagId = null): void;
public function submit(UserMessage $message): AssistantMessage;
public function setBagId(string $bagId): void;
} MessageStoreInterface.php interface MessageStoreInterface
{
public function save(MessageBagInterface $messages, ?string $bagId = null): void;
public function load(?string $bagId = null): MessageBagInterface;
public function clear(?string $bagId = null): void;
public function setBagId(string $bagId): void;
} It's similar to what suggested by @Guikingone (including the rewrite of This way, a user can configure stores with a default I've tricked my vendors to test it in a real case, and it works like a charm. Let me know if you want me to send more code here (especially implementations). |
46c9bcc
to
db970e5
Compare
9e8b6b0
to
eb14cc7
Compare
please give me a chance to have another look before merging - was quite busy with that mcp thingy, but this is quite a change, and i'd like to give it more thought. |
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.
wow, that profiler thing is new :D 👍
a few things:
- i think it's a good to rework the wikipedia example, but for the youtube and audio ones, it's a step into the wrong direction in my opinion
- twig components are like controllers to me, and should only bring infrastructure and logic together - in the case of audio and youtube they now do more - due to the fact that there is no way to hook into the initialize of the chat - might be a missing extension point - or a InputProcessor :)
- just look at the constructors of those classes - that's too much
- so we either work on that as well, or revert those changes for now, and do it in another
- the chat should not leak the message store - if i need to inject both services, the integration is not complete => so the chat needs a
clear
orreset
- state handling is odd
- still stateful services
- nullable ids in the interfaces also break my heart here
- mixing ID and session keys
- ... but we can work on better example than session and cache to get it right => doctrine maybe?
Hi @chr-hertel @OskarStark 👋🏻 Regarding your thoughts @chr-hertel, I might have a solution for the id issue, it's not "perfect" but it could help keeping the Don't know if you've planned to work on this during the hackathon, if so, could you give me a few days to test/push my idea? It might not be THE solution but we never know, could be one 😅 |
eb14cc7
to
7b60dc4
Compare
I think we have enough other tasks to tackle, so take your time on this one I would say |
I would create a new |
Just to store chat messages? 🤔 The |
I am thinking about it and in terms of packages it may make sense to split them, but on the other hand... |
7b60dc4
to
cc44f81
Compare
@OskarStark I was thinking about the split of the message store / chat, what about a |
I just pushed a new version (on a separate commit to help reading it), this one remove the You'll also find a @welcoMattic It's close to the previous API, just "different" on its usage 😅 |
c191646
to
6ab2030
Compare
6ab2030
to
f57a693
Compare
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 go with a chat
folder in the examples 👍
|
||
private function doStart(string $videoId): void | ||
{ | ||
$transcript = $this->transcriptFetcher->fetchTranscript($videoId); | ||
$system = <<<PROMPT | ||
You are an helpful assistant that answers questions about a YouTube video based on a transcript. | ||
If you can't answer a question, say so. | ||
Video ID: {$videoId} | ||
Transcript: | ||
{$transcript} | ||
PROMPT; | ||
|
||
$messages = new MessageBag( | ||
Message::forSystem($system), | ||
Message::ofAssistant('What do you want to know about that video?'), | ||
); | ||
|
||
$this->reset(); | ||
$this->chat->initiate($messages); | ||
} |
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.
not a big fan of putting more logic into the TwigComponent - especially additional service dependencies.
can we have some kind of initialize hook/method/event in the chat instead?
#[Autowire(service: 'ai.chat.youtube')] | ||
private readonly ChatInterface $chat, | ||
#[Autowire(service: 'ai.message_store.cache.youtube')] | ||
private readonly MessageStoreInterface $messageStore, |
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 would still be great to hide the store as internal of the chat - instead having to deal with both services in user land.
message_store: | ||
cache: | ||
audio: | ||
service: 'cache.app' | ||
wikipedia: ~ | ||
youtube: ~ | ||
chat: | ||
audio: | ||
agent: 'audio' | ||
message_store: 'cache.audio' | ||
wikipedia: | ||
agent: 'wikipedia' | ||
message_store: 'cache.wikipedia' | ||
youtube: | ||
agent: 'youtube' | ||
message_store: 'cache.youtube' |
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 we make it more concise here?
message_store: | |
cache: | |
audio: | |
service: 'cache.app' | |
wikipedia: ~ | |
youtube: ~ | |
chat: | |
audio: | |
agent: 'audio' | |
message_store: 'cache.audio' | |
wikipedia: | |
agent: 'wikipedia' | |
message_store: 'cache.wikipedia' | |
youtube: | |
agent: 'youtube' | |
message_store: 'cache.youtube' | |
chat: | |
audio: | |
agent: 'audio' | |
type: 'cache' | |
service: 'cache.app' | |
wikipedia: | |
agent: 'wikipedia' | |
youtube: | |
agent: 'youtube' |
This PR was merged into the main branch. Discussion ---------- [Chat] Introduce a new component | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Docs? | yes | Issues | Related to #254 | License | MIT Hi 👋🏻 Ok, time to tackle the big work on the #254 issue, the current structure doesn't allows to introduce new stores neither handling the storage of messages, mainly due to the fact that we can't intervene in the agent component without breaking the SRP of it. This PR aims to split the `Chat` into a new component and ease the work on storing messages (with a new component, we can easily add storages here rather than in the `agent` component), plus, it helps splitting the responsibilities inside the initiative as we should be allowed to start new agents without relying on any chats. This PR is a draft, feel free to open debates about it 😄 Commits ------- 3f1dd48 refactor(core): chat sub-component started
Rebase unlocked, as #675 is now merged 👍 |
Hi 👋🏻
A small POC for the "session" that we discussed in #239, this PR is a WIP (and even more than a WIP, an open discussion 😅 ).
Here's the current state:
If we want to store "multiple sessions" depending on the agents, we can't interact with the bag (as it's not persisted), the only way is to do it at the store level, thing is, how?
Well, one way is to "initialize" the store with a session, the question is: Is it mandatory every time we initialize a store?
If so then we can just change the constructor of
InMemoryStore
and it's done, easy, simple, not much work, thing is, we'll be blocked if we want to handle multiple agents at the same time in the same store (parallel handling, etc).So here's my take, not perfect, open to discuss about it, it's a POC after all 😄
PS: At the end, this API calls will be hidden when using the bundle, the main concern is the "OOP without Sf" usage.
PS II: The interface is inspired by the one from
Cache
for tags, the behavior is not the same but the idea still there.EDIT (9/3/2025)
Here's the Profiler panel (before submitting):
Here's the Profiler panel (after submitting):