fix: give AgentSelector ownership of its agent order - #1400
Conversation
AgentSelector.reinit() stored the caller's list by reference, so env._agent_selector.agent_order was the same object as env.agents for every env that passed self.agents. AECEnv._was_dead_step() then mutated the selector's cycle in place via self.agents.remove(agent), and dynamic-agent envs grew it in place via self.agents.append(agent). The coupling was invisible and left the selector with an empty agent_order at the end of an episode, where next() raises ZeroDivisionError. reinit() now copies, and the two mutations are explicit: - AECEnv._was_dead_step() calls AgentSelector.remove_agent() - the generated_agents example envs call AgentSelector.add_agent() This surfaced a latent bug in cooperative_pong, which built its selector once in __init__ and never refreshed it on reset(); it worked only because the selector's stale list happened to hold the same agents. It now reinits against the fresh agent list. Refs Farama-Foundation#1332.
|
@teddytennant your PR is failing CI |
_was_dead_step now removes agents from the selector's cycle, so by the end of an episode agent_order can be empty. reset() was reiniting from that drained list, which left envs that only call reset() (notably mpe2, used by the AgileRL tutorials) dead on the second episode with ZeroDivisionError. Keep the order last passed to reinit as a baseline, restore it on reset(), and treat add_agent/remove_agent as episode-local. generated agents envs start the cycle with next() after building via add_agent so reset no longer wipes their setup.
|
Fixed the AgileRL CI failure. Root cause: Fix: New unit tests cover restore-after-removals, drop-episode-local-additions, and reinit-updates-baseline. mpe2 multi-episode (AEC + parallel) and the existing agent_selector suite pass locally. |
dm-ackerman
left a comment
dm-ackerman
left a comment
There was a problem hiding this comment.
a few minor-ish things - a couple are just things to think about, but assuming the naming of the selector really ought to be addressed
| def reset(self) -> Any: | ||
| """Reset to the original order.""" | ||
| self.reinit(self.agent_order) | ||
| """Reset to the order last passed to :meth:`reinit`. |
There was a problem hiding this comment.
This should document the return value - it's (maybe) not obvious that reset steps through the agent list
| self.agent_order = list(self._original_agent_order) | ||
| self._current_agent = 0 | ||
| self.selected_agent = 0 |
There was a problem hiding this comment.
I believe this can be implemented as self.reinit(self._original_agent_order) for less duplication at a slight performance cost
| # Envs used to get this for free, because the selector aliased the very | ||
| # list we just mutated. It owns a copy now, so drop the agent explicitly. | ||
| agent_selector = getattr(self, "_agent_selector", None) | ||
| if agent_selector is not None: | ||
| agent_selector.remove_agent(agent) | ||
|
|
There was a problem hiding this comment.
This adds an undocumented expectation that environments have defined their agent selector as _agent_selector. If they haven't, this will potentially yield bugs.
i think this needs to be handled somehow
| self._current_agent = 0 | ||
| self.selected_agent = 0 | ||
|
|
||
| def add_agent(self, agent: Any) -> None: |
There was a problem hiding this comment.
weird things can happen if there are duplicate agents added (due to removal and is first/last checks). There was never a check for that before, I don't know if it makes sense to add one now?
| Does nothing if the agent is not in the cycle, so that an env which has | ||
| already dropped the agent itself can still call this unconditionally. |
There was a problem hiding this comment.
I'm not sure having remove_agent silently ignore the agent not being present is the best option. It's not in line with existing remove() behavior in the python library which raises errors. It's not clear to me that there is a need for supporting duplicate calls to remove_agent(). What is the use case for supporting that?
|
I made a couple comments on the code - mostly minor things and at least one more than minor thing However, thinking more on this, I don't think this should be accepted. This is a breaking change that is likely to be overlooked by most users. We're probably not going to tag this version 2.0.0 so we're breaking other peoples code without telling them, which is not lining up with best practices or the project standards (https://farama.org/project_standards) As for breaking changes, I'm not claiming this is a bad change, just premature. If this is the way to go, it makes more sense in PZ 2.0 than right now. Is there an actual bug that this is fixing? If not, it seems questionable to knowingly break a bunch of existing environments and add potential bugs for something that is not needed. |
Address review feedback: reset() steps through the cycle rather than only rewinding it, which was not obvious from the docstring, and it can restore the baseline order through reinit() instead of repeating its body.
|
Thanks, both good points. I pushed fixes for the three code comments:
On whether this should land at all, to answer your question directly: yes, there are two real bugs.
Worth noting this is option B from #1332, which was asked for there. If the breakage still feels too early for a 1.x release I'm happy to split it: the |
Address the remaining review comments: - add_agent now rejects an agent already in the cycle. Duplicates break remove_agent, which drops only the first copy, and is_first/is_last, which compare by value. - remove_agent raises ValueError for an agent that is not in the cycle, matching list.remove instead of failing quietly. _was_dead_step, the one caller that cannot know whether the agent is still there, checks membership itself. - api_test now asserts that no AgentSelector held by the env still lists an agent after that agent's final step(None). It searches by type rather than by name, so an env that stores its selector somewhere other than _agent_selector fails the test instead of silently keeping a stale cycle. The _agent_selector convention is documented on _was_dead_step.
This is option B from #1332, as requested. It also folds in option A's regression test for the #1229 case.
The problem
AgentSelector.reinit()stored the caller's list by reference:So for every env that passes
self.agents— tictactoe, chess, go, connect_four, rps, hanabi, pistonball, pursuit, multiwalker —env._agent_selector.agent_orderisenv.agents, the same list object. Two pieces of core behavior were quietly riding on that aliasing:AECEnv._was_dead_step()shrinks the cycle viaself.agents.remove(agent)generated_agentsexample envs grow it viaself.agents.append(agent)inadd_agent()Neither ever names the selector, so the coupling is invisible at both ends. It also leaves the selector with
agent_order == []at the end of an episode, wherenext()raisesZeroDivisionError: integer modulo by zero.The change
reinit()now copies, and both mutations become explicit:AgentSelector.add_agent()/AgentSelector.remove_agent()AECEnv._was_dead_step()callsremove_agent()— this is where envs previously got removal for freegenerated_agentsexample envs calladd_agent()remove_agent()is a no-op for an agent that isn't in the cycle, so an env that already dropped the agent itself (KAZ maintains its own_live_agentscopy) can call it unconditionally.It surfaced a real bug in cooperative_pong
cooperative_pongbuilds its selector once in__init__and never refreshes it onreset(). Under aliasing that was masked:reset()rebindsself.agentsto a fresh list, so the selector's original list was never the one_was_dead_step()drained, and it happened to hold the right agents forever. With the selector owning its order, that stale list does get drained, and the env dies on the second episode. Fixed by re-initing against the fresh agent list on reset, which is what every other env already does.I'd argue that's the point of the change — the aliasing was hiding it.
Breaking change
A third-party AEC env with a dynamic agent set that adds agents by mutating
self.agentsand expects the selector to notice will need to calladd_agent(). Removal is unaffected, since_was_dead_step()handles it. Envs with a fixed agent set (i.e. nearly all of them) need no changes.Testing
New
test/agent_selector_test.py, 10 tests: thatreinit()copies,add_agent/remove_agent, the no-op removal, that_was_dead_step()keeps the cycle in sync, that agents added mid-episode get selected, that the order doesn't leak across resets, and a full dynamic episode staying consistent.It includes the #1229 regression test: reset while an agent is still missing from
.agentsrestores the whole cycle. (As noted in #1332, that case already worked — this pins it so it doesn't get re-reported.)Full suite: no newly-failing tests vs
main.Type of change
Checklist:
pre-commitchecks withpre-commit run --all-files(seeCONTRIBUTING.mdinstructions to set it up)pytest -vand no errors are present.pytest -vhas generated that are related to my code to the best of my knowledge.