Add __setattr__ to wrappers. Fixes #1176#1180
Add __setattr__ to wrappers. Fixes #1176#1180dm-ackerman wants to merge 2 commits intoFarama-Foundation:masterfrom
Conversation
Adding this ensures that variables get set to the appropriate location. By default, any public value set by the wrapper is sent to the env to be set there instead of on the wrapper. If a variable is meant to be set by the wrapper, it should be listed in the _local_vars class variable of the wrapper. This is not ideal, but seems to be the most reasonable design. An example of needing to specify which vars to keep locally is here: https://python-patterns.guide/gang-of-four/decorator-pattern/#implementing-dynamic-wrapper The solution is to list which vars should be in the wrapper and check them when setting a value. That is the approach used in this commit, but more generalized. In line with __getattr__, private values cannot be set on underlying envs. There are two exceptions: _cumulative_rewards was previously exempted in __getattr__ because it is used by many envs. _skip_agent_selection is added because is used byt the dead step handling. If a wrapper can't set this, that functionality will break.
These check that the wrapper functions of __getattr__ and __setattr__ work as intended
|
I can't block the automated tests from running. I'm expecting them to fail because #1177 has not be merged yet. The failures should be resolved with that PR. |
|
Apparently this breaks a couple tutorials. The problem for a couple seems to be the use of supersuit wrappers that don't work with this change |
|
I’m not a big fan of using _local_vars, I think we should do however Gymnasium wrappers are done with inheriting attributes and such, to ensure consistency between the two packages. They also have put a lot more thought into these kinds of decisions and have way more users who will point out design flaws so I trust their decisions |
|
Looks like everything is done based off env.property instead of defining new properties (don’t do self.action space = self.env.action space for example) |
|
I'm looking at the gymnasium approach. PZ has different considerations so there are some complications. But may be able to pull their idea. I'll have to try that. |
|
I would do some performance testing but I believe this might impact performance massively as this is run on literally every variable update If you need to add this, I think something else has gone horribly wrong and would only do this as a last, last, last solution Context is that I looked at adding this to Gymnasium and decided not due to both performance and implementation details, so was less painful to remove other code |
|
Yeah, you may be right on performance. I saw the threads on Gymnasium mentioning that. I don't think this will be the final version. May do something more in line with Gymnasium, but PettingZoo wrappers have more issues than Gymnasium ones so there's more needed (I think) than just copying over the solution. I don't have access to test things until next week so can't do anything on this right now. |
|
@dm-ackerman let me know if you're planning to do more work on this, otherwise I will close the PR before the next release |
|
I need to look back at this. the issue it's trying to solve is real and important, but as someone else mentioned, there is a performance hit that is significant. The approach gymanasium uses to solve this doesn't work here for reasons I can't recall. I am swamped with deadlines but i will try to look at this in the next couple weeks. |
|
@dm-ackerman sounds good, I'll leave it open then. Good luck with your deadlines! |
4 participants
Description
Adding
__setattr__ensures that variables get set to the appropriate location. By default, any public value set by the wrapper is sent to the env to be set there instead of on the wrapper. If a variable is meant to be set by the wrapper, it should be listed in the_local_varsclass variable of the wrapper. This is not ideal, but seems to be the most reasonable design.An example of needing to specify which vars to keep locally is here:
https://python-patterns.guide/gang-of-four/decorator-pattern/#implementing-dynamic-wrapper
The solution is to list which vars should be in the wrapper and check them when setting a value. That is the approach used in this commit, but more generalized.
In line with
__getattr__, private values cannot be set on underlying envs. There are two exceptions:_cumulative_rewardswas previously exempted in__getattr__because it is used by many envs._skip_agent_selectionis added because it is used by the dead step handling. If a wrapper can't set this, that functionality will break.Fixes #1176, Depends on #1177 (for tests to pass)
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.