-
Notifications
You must be signed in to change notification settings - Fork 40
Feature/multi agent framework #329
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
Feature/multi agent framework #329
Conversation
Refactored negotiation arena example scripts to use more explicit typing, improved agent profile retrieval logic, and removed unused OpenAI API key loading. Updated .gitignore to exclude negotiation_arena redis-data directory.
XuhuiZhou
left a comment
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.
Good job! Overall, I think we need to condense things a bit and make trackable changes
see my comments
sotopia/envs/evaluators.py
Outdated
| model_name: str, | ||
| response_format_class: type[EvaluationForTwoAgents[T_eval_dim]], | ||
| response_format_class: Union[ | ||
| type[EvaluationForTwoAgents[T_eval_dim]], |
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.
Could we merge EvaluationForTwoAgents and EvaluationForMultipleAgents together? those two classes should basically serve as the same functionality?
sotopia/envs/multi_agent_parallel.py
Outdated
| from sotopia.envs.evaluators import unweighted_aggregate_evaluate | ||
|
|
||
|
|
||
| class MultiAgentSotopiaEnv(ParallelSotopiaEnv): |
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.
Same here, let's just merge ParallelSotopiaEnv and MultiAgentSotopiaEnv together? If there's logic that we think needs separation, we could do that. However, instead of rewriting the function, we should do super. etc.
sotopia/messages/message_classes.py
Outdated
| ) | ||
|
|
||
|
|
||
| class MultiAgentBackground(ScriptBackground): |
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.
same 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.
Ok done! Thanks for pointing this out!
Remove separation between 2-agent and multi-agent cases by consolidating duplicate classes and logic paths into unified implementations
XuhuiZhou
left a comment
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 job!
I left some nitpick comments
one thing to think carefully about is how to unify different scenarios
rn the negotiation arena seems to have its own logic of doing data
rather than use their own logic, we should think carefully and design our own way.
|
|
||
| sotopia/cli/install/redis-data/* | ||
| redis-stack-server-*/ | ||
| examples/experimental/negotiation_arena/redis-data/* |
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's this? do we need to add this line?
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 add redis-data to that folder for testing the negotiation scenarios (see /Users/keyuhe/sotopia/examples/experimental/negotiation_arena/README.md) but I assume redis-data should not be pushed
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 have folder level ignore, we might not need global level gitignore?
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.
oh I will keep the global level gitignore and remove the folder level ones
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.
this is not really aligned with the repo level setup?
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.
will move this to tests/experimental and modify correspondingly
sotopia/envs/evaluators.py
Outdated
| break | ||
| stale_too_long = stale_count > self.max_stale_turn | ||
| terminated = conversation_too_long or p1_leaving or p2_leaving or stale_too_long | ||
| terminated = conversation_too_long or players_leaving or stale_too_long |
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.
the logic here should at least two agents are in the playground i guess? (for werewolf games, some agents could leave/die?)
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.
changed to if too_few_agents (# agents < 2). But I think in the future, this maybe very depending on the game/scenario (e.g. in werewolf, we consider end even if 5 agents are still on (god + 2 villagers + seer + witch))
sotopia/messages/message_classes.py
Outdated
| agent_goals: list[str], | ||
| ) -> "ScriptBackground": | ||
| """Create a ScriptBackground for multi-agent scenarios.""" | ||
| return cls( |
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.
this is not really multi agent?
we should change the ScriptBackground dataclass.
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.
ok!
resolve comments
also deleted temperature (to support gpt-5)
|
I will begin reviewing this PR when tests are successful. |
|
@ProKil Hi! now the tests are passed |
ProKil
left a comment
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.
LGTM now
📑 Description
Add scenario examples from NegotiationArena and implement comprehensive multi-agent support for 3+ agents.
✅ Checks
type/descript(e.g.feature/add-llm-agents)ℹ Additional Information