Order enforcing wrapper fix #1205
Conversation
The following were removed: value == "unwrapped" - will never be triggered because the base class has an unwrapped property which returns the same result value == "render_mode" - duplicates the effect of the base class's __getattr__. value == "possible_agents", "observation_spaces", "action_spaces", and "agent_order" - not related to the stated goal of the class
The previous one did not accurately reflect the class behavior.
_has_rendered was never used
The problem stemmed from the iterator being based on AECEnv but accessing env._has_updated which only exists in the OrderEnforcingWrapper. Pyright's error is correct based on the types given in the code. However, the usage of the class is correct. This is a workaround that uses a more precise type and convinces pyright that the code is correct.
| @@ -61,20 +61,6 @@ def warn_action_out_of_bound( | |||
| f"[WARNING]: Received an action {action} that was outside action space {action_space}. Environment is {backup_policy}" | |||
| ) | |||
|
|
|||
| @staticmethod | |||
There was a problem hiding this comment.
Why exactly was this stuff removed? Looks kind of messy code and isn’t done in the other wrappers so can probably see it just want to make sure it’s on purpose that it got removed
There was a problem hiding this comment.
It is dead code that is never called. I think it was maybe intended to be used in the OrderEnfocingWrapper - and maybe was used in an older version of it, but it's not used anymore. The code is specific to that wrapper, if it's not used there, there's no other place it would be useful.
| # strictly true, but it should have OrderEnforcingWrapper somewhere | ||
| # in the wrapper list). This might be better handled by Protocols, | ||
| # but this approach works. | ||
| self.env = env # silence pyright errors |
There was a problem hiding this comment.
Imo we’re better off just ignoring the pyright error directly vs setting a variable too early or having extra variables that aren’t set exactly how they should be, simpler that way
There was a problem hiding this comment.
fair enough. I changed this back.
| assert ( | ||
| self.env._has_updated # pyright: ignore[reportGeneralTypeIssues] | ||
| self.env._has_updated |
There was a problem hiding this comment.
I think we can just do ignore general type issues at the top of the file instead of these ugly in line ones (assuming that’s the main reason you changed this)
There was a problem hiding this comment.
I re-added this per above comment. I think the end of line one is better in this case, even though it is ugly. Ignoring any error that might show up in the file isn't great. This is a generic catch-all error too, so it has a bigger chance of hiding something important that we don't know about.
The issue was mainly that the wrapper wasn't doing what it said it was and was doing things that didn't relate to enforcing order. I think this was all left over from other changes that were made previously. There isn't really an error from that issue that can be tested, it was mostly noticing that the details didn't match the docs. |
Description
Fix several issues related to
OrderEnforcingWrapper__getattr__that are unrelated to the wrapper (see [Bug Report] Incorrect behaviour from __getattr__ in OrderEnforcingWrapper? #1174)AECOrderEnforcingIteratorreturnandelsestatementsFixes #1174
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.