Skip to content

feat: Add MessageStore base class abstraction for storing message thread in teams; Implement ListMessageStore for internal uses #6350

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: Add MessageStore base class abstraction for storing message thread in teams; Implement ListMessageStore for internal uses #6350

wants to merge 7 commits into from

Conversation

withsmilo
Copy link
Contributor

Why are these changes needed?

The discussion thread is #6169.

Related issue number

resolved #6227

Checks

@withsmilo
Copy link
Contributor Author

@microsoft-github-policy-service agree company="LINE"

@withsmilo withsmilo force-pushed the implement_message_store branch 2 times, most recently from 486bce3 to 8b4e3ee Compare April 22, 2025 00:14
This was referenced Apr 22, 2025
Merged
Closed
@withsmilo withsmilo force-pushed the implement_message_store branch 3 times, most recently from 95eae7f to d0dd8fa Compare April 25, 2025 13:27
@withsmilo
Copy link
Contributor Author

Hi, @ekzhu .
Is there any update? I'm still waiting for your kind feedbacks on this PR.

@withsmilo withsmilo requested a review from ekzhu April 26, 2025 22:54
@withsmilo withsmilo force-pushed the implement_message_store branch from d0dd8fa to c3a4f12 Compare April 26, 2025 22:57
ekzhu
ekzhu reviewed Apr 29, 2025
View reviewed changes
Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

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

We need to take consideration of how orchestration logics inside group chat managers play with the message store's expiration behavior.

python/packages/autogen-agentchat/src/autogen_agentchat/message_store/_memory_message_store.py Outdated
return

time_threshold = current_ts - self._ttl_sec
self._messages = deque(m for m in self._messages if m.ts > time_threshold)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why iterating over the whole list? Messages are appended in time order; we should just go from the beginning of the list until we find the first message that's within the time window and slice the list to the index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed via 191934f

python/packages/autogen-agentchat/src/autogen_agentchat/message_store/_memory_message_store.py Outdated
component_config_schema = MemoryMessageStoreConfig
component_provider_override = "autogen_agentchat.message_store.MemoryMessageStore"

def __init__(self, message_factory: MessageFactory, ttl_sec: Optional[int] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there needs to be a way to "pin" some messages in the store. Otherwise, the user's task message which is often the first one, and the most recent messages, which are necessary for orchestration logics such as Swarm, they shouldn't be allowed to expire because it will break orchestration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example in Swarm, we need to pin the most recent HandoffMessage. If that message is not pinned, it will introduce a runtime error next time the chat manager selects the next agent if the message is expired.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me think how to pin some messages. Thanks for your detail feedback.

Copy link
Contributor Author

@withsmilo withsmilo May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ekzhu
I think GroupChatManager pinning or unpinning certain messages is beyond the scope of this PR. How does GroupChatManager know which messages to pin/unpin?

So my conclusion is that the "pin" function is only used inside the ListMessageStore class. What do you think of below approach?

class ListMessageStore(MessageStore, Component[ListMessageStoreConfig]):
    def __init__(self,
                 message_factory: MessageFactory,
                 pin_first_message: bool = False,  # <== added
                 pin_last_message: bool = True,  # <== added
                 ttl_sec: Optional[int] = None):
    ...

We can ignore first or last messages in _remove_expired_messages function properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ekzhu
Please refer c75157e, but this is under open discussion now.

...ckages/autogen-agentchat/src/autogen_agentchat/teams/_group_chat/_base_group_chat_manager.py
@@ -157,7 +161,8 @@ async def handle_agent_response(self, message: GroupChatAgentResponse, ctx: Mess
return

# Select a speaker to continue the conversation.
speaker_name_future = asyncio.ensure_future(self.select_speaker(self._message_thread))
messages = list(await self._message_store.get_messages())
Copy link
Collaborator

Choose a reason for hiding this comment

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

See the logic in SwarmGroupChatManager's implementation of select_speaker, it requires the most recent HandoffMessage.

@withsmilo withsmilo force-pushed the implement_message_store branch from c3a4f12 to 191934f Compare May 1, 2025 02:19
@withsmilo withsmilo requested a review from ekzhu May 1, 2025 03:08
@withsmilo withsmilo changed the title feat: Add MessageStore base class abstraction for storing message thread in teams; Implement MemoryMessageStore for internal uses feat: Add MessageStore base class abstraction for storing message thread in teams; Implement ListMessageStore for internal uses May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekzhu ekzhu Awaiting requested review from ekzhu

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add MessageStore base class abstraction for storing message thread in teams
2 participants
@withsmilo @ekzhu