Permit AgentIDs other than str
#1071
Conversation
|
Hi, @elliottower. I believe this PR is ready to be reviewed. For testing, I copied the environments in example_env, but made them use non-string AgentIDs, since those demonstrate termination, addition, and other behaviors. Let me know if that testing is sufficient, or if there are other tests you would like to see. |
|
I wonder if there's a way to make a wrapper to change the agent ID's to be something custom like a string? That seems more elegant than rewriting an entire environment changing like a single attribute. Then we could have it be similar to the unwrapped or pickle tests where we can easily iterate over different envs and ensure different ones work with the changes. But for now this seems pretty good. Looks like the tutorial tests and pre-commit failed when I ran it this first time, can you run |
|
Another potential idea although it wouldn't be as clean, would be to have a wrapper that converts into string agent IDs so that the highest level user facing environment would be interactable using integers for agent id, but then in every subsequent level beneath that it would be cast to a string so the other wrappers and functionality works normally. Only problem is certain things you may not want to convert to a string, but inherently string types are sort of the type which everything can be converted into so I don't see it being a huge problem |
|
Also I'm going to be very busy with things this week so I won't be able to help much with this |
pettingzoo/test/example_envs/generated_agents_parallel_cust_agentid_v0.py
Outdated
Show resolved
Hide resolved
| ) -> tuple[ | ||
| ObsDict, dict[str, float], dict[str, bool], dict[str, bool], dict[str, dict] | ||
| dict[AgentID, ObsType], |
There was a problem hiding this comment.
This seems cleaner than ObsDict type
There was a problem hiding this comment.
I guess the consequence of this is that it's now somewhat backwards incompatible, but I guess even I agreed that it seemed cleaner to be explicit that it's a dict of agentID and obstype
There was a problem hiding this comment.
Yeah okay looking back at that full file I think you're probably right that we should just make a shimmy update and try to be more clear, we don't have RewardDict or anything like that anyways, so it's simplest to make all of these types fully clear when you see them in the type hint and know what to expect without having to cross reference 'oh what is an ObsDict'. And I think I'm probably one of the only people who uses ObsDict anyways
|
Looks like it's saying |
|
I believe the type errors will go away if you add this line: |
|
I've added annotations on all files where I saw the error occur. |
Sounds good, thanks |
|
It may not be that ideal to have to import annotations on any internal file which deals with calling these environments, but that's a cost I'm willing to pay. Plus I believe in 3.9 you don't need to import them anymore, so once 3.12 is out next year and 3.8 is phased out I think it will no longer be necessary. |
|
Tag me when you want me to re-run the tests or review again, this looks almost perfect but just a few little nitpicks I noted above. Thanks again for this, great work paying such close attention to detail and fixing all of the little issues. |
|
It looks like all tests have passed. From my end, I'm ready to merge. |
|
@elliottower Following up on this. |
|
@pimpale could you look into this? Looks like your removal of obs dict caused an issue with shimmy. Maybe it should be added back in a patch? Farama-Foundation/Shimmy#106 |
| @@ -177,7 +173,7 @@ def agent_iter(self, max_iter: int = 2**63) -> AECIterable: | |||
|
|
|||
| def last( | |||
| self, observe: bool = True | |||
| ) -> tuple[ObsType | None, float, bool, bool, dict[AgentID, dict[str, Any]]]: | |||
| ) -> tuple[ObsType | None, float, bool, bool, dict[str, Any]]: | |||
There was a problem hiding this comment.
Also looking at before and after this line looks incorrect, it should use AgentID, but also simpler to just keep the ObsDict type at the top of the file and define it to use AgentID, which you changed ofc. That way if agent id gets changed again this will not need to be changed as it just references it. And prevents breaking code like the shimmy bug.
There was a problem hiding this comment.
The reason I changed that line was because we return self.infos[agent] for the info dict, and self.infos has the type dict[AgentID, dict[str, Any]].
That line was producing a type error previously.
There was a problem hiding this comment.
I'm a little confused why it needs to be dict[str] here though instead of agent ID
There was a problem hiding this comment.
I guess this has to do with the other PR up right now which talks about infos and the correct syntax for it. What I gather is that parallel envs need to have the dict be indexed by agent ID and have sub dicts with info for each agent, but I guess in AEC environments you just need any dict and it's assumed that it's the info only for that agent on that timestep. So it shouldn't be nested dict like the original code, but I think it should still be dict[AgentID, Any]
There was a problem hiding this comment.
Well in this case, (last()) we're only returning the info for the current agent, so I don't understand why the it should be indexed by AgentID.
If someone wants to use int to index their agents, but still have an info that looks like { "extra_info": 1, "extra_info2": 2 }, (per agent) then that seems fine to me.
|
Can we instead create a PR to shimmy to fix the issue? To me it seems like ObsDict and ActionDict were just type aliases, and it should be fairly straightforward to replace. |
|
Is there a principled reason that ObsDict and ActionDict need to be removed though? I don't really see a reason why we shouldn't include them just in case people already use them and like the convenience or whatever |
|
Just looked and it seems like 21 different repositories use the ObsDict type... guess I'm not the only one. I can see an argument for keeping the ObsDict defined for backwards compatibility but internally still doing your changes which I think are more clean. (TLDR: just re-add the two lines defining ActionDict and ObsDict but we don't need to use them internally) |
|
Hmm. In that case I agree with you, I'll add them back for backwards compatibility. Although technically, since pettingzoo released a new major version, breaking changes are to be expected. |
|
Yeah but we want to minimize them when possible and make the upgrade process as easy as can be |
Description
Adds the feature described in #1070, by making ParallelEnv and AECEnv generic over AgentID.
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.