-
Couldn't load subscription status.
- Fork 450
feat: add multiagent session/repository management. #1071
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
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| self, | ||
| session_id: str, | ||
| storage_dir: Optional[str] = None, | ||
| session_type: SessionType = SessionType.AGENT, |
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 think we need to require that this is a named parameter - @dbschmigelski can you confirm - I recall you we discussed a similar situation before? (if so, is there anywhere we can document/write this down)?
| session_type: SessionType = SessionType.AGENT, | |
| *, | |
| session_type: SessionType = SessionType.AGENT, |
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.
Out of curiosity, why does this need to be a named parameter?
|
answer this question: #1071 (comment) I don't think this is a new issue—it's an aspect we overlooked. Even in the single-agent case, we face this file completeness problem. Talked with Patrick and Dean about this change. We don't perform any validation or defensive checks when reading the session file, ensuring its completeness is crucial. |
| self, | ||
| session_id: str, | ||
| storage_dir: Optional[str] = None, | ||
| session_type: SessionType = SessionType.AGENT, |
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.
Out of curiosity, why does this need to be a named parameter?
| self._write_file(multi_agent_file, multi_agent_state) | ||
|
|
||
| # Update session.update_at | ||
| self.update_session(session_id) |
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.
Do we also need to call self.update_session for update_agent() calls?
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 see SessionAgent has filed updated_at but yes, even for sinlge agent we should update matadata.
| session_data = multi_agent.serialize_state() | ||
| self._write_file(multi_agent_file, session_data) | ||
|
|
||
| def read_multi_agent(self, session_id: str, multi_agent_id: str, **kwargs: Any) -> Optional[dict[str, Any]]: |
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.
For read_agent and update_agent, we pass around a SessionAgent instance. For consistency, do we want to setup a SessionMultiAgent type?
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.
Initially I introduced a data class MultiagentState then we decided to remove that, so now, I don't think we should do that for update
For read we only have def read_agent(self, session_id: str, agent_id: str, **kwargs: Any) -> Optional[SessionAgent]
| boto_session: Optional[boto3.Session] = None, | ||
| boto_client_config: Optional[BotocoreConfig] = None, | ||
| region_name: Optional[str] = None, | ||
| session_type: SessionType = SessionType.AGENT, |
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.
Why do we have to make session_type a named parameter on FileSessionManager but not here?
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.
yeah it should, I had a bad rebase
| raise SessionException(f"MultiAgent state {multi_agent_id} in session {session_id} does not exist") | ||
|
|
||
| multi_agent_key = f"{self._get_multi_agent_path(session_id, multi_agent_id)}multi_agent.json" | ||
| self._write_s3_object(multi_agent_key, multi_agent_state) |
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.
Do we need an update_session call here also as we did in `FileSessionManager?
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.
Function wise no, s3 files has update at in console.
For file itself, yes. I had this conversation with @dbschmigelski in previous large PR. The conclusion is not updating it in S3
| Multi-agent state dictionary or empty dict if not found | ||
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.
| Multi-agent state dictionary or empty dict if not found | |
| Multi-agent state dictionary or empty dict if not found. |
| ) -> list[SessionMessage]: | ||
| """List Messages from an Agent with pagination.""" | ||
|
|
||
| @abstractmethod |
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.
Using @abstractmethod will immediately break anyone who derives a custom SessionRepository even if they don't use multi-agents. We should instead use the NotImplementedError approach.
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.
If we want to introduce new interface it has to be non-breaking?
…ultiagent base & agent result
…e, rename deserialize function # Conflicts: # src/strands/experimental/agent_config.py
1e4682b to
dfd776c
Compare
Description
This is a split from multiagent session persistent part 2.
For more context, see origin: #900
Overview
This PR extends the session management system to support multi-agent workflows alongside existing single-agent functionality.
New APIs Added
SessionType Enum Extension
SessionType.MULTI_AGENTto distinguish multi-agent sessions from single-agent sessionsSessionRepository Interface
SessionManager Base Class
Implementation Changes
FileSessionManager
multi_agents/instead ofagents/directorymulti_agent_<id>/multi_agent.jsonS3SessionManager
multi_agents/multi_agent_<id>/multi_agent.jsonRepositorySessionManager
Backward Compatibility
session_type=SessionType.AGENTmaintains existing behaviorIntegrate with Graph
Can correctly write to session , fire hooks on initializating, node finished, failed. Restore from saved session, start over if completed.
example state json:
Related Issues
Documentation PR
Type of Change
Bug fix
New feature
Breaking change
Documentation update
Other (please describe):
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce warnings in consuming repositories: agents-docs, agents-tools, agents-cli
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.