feature/provide access to wrapped attr

[ffelten]

Description

This fixes duplicated attributes in base wrapper classes and adds forwards through __getattr__ in BaseWrapper. I also made possible to get env-specific attributes through OrderEnforcing, which was preventing all API extensions.
See: https://discord.com/channels/961771112864313344/1176822393910612038

Note: conversions.py are still a mess but it's more difficult to define a getattr since you convert API at the same time... So I left them untouched. It is however possible to retrieve env-specific attributes in converted env by using .unwrapped.

Example

from pettingzoo.butterfly import knights_archers_zombies_v10
env = knights_archers_zombies_v10.env(render_mode=None)
converted_env = knights_archers_zombies_v10.parallel_env(render_mode=None)
print(env.max_cycles) # works
print(converted_env.max_cycles) # conversion shadows attr :(
print(converted_env.unwrapped.max_cycles) # this works though

Note2: I don't have the ROM for ALE installed locally so I couldn't run these on my machine.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)

Checklist:

• I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
• I have run pytest -v and no errors are present.
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I solved any possible warnings that pytest -v has generated that are related to my code to the best of my knowledge.
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[elliottower]

Looks like a weird error with one of the tests I can take a look at some point if you can’t figure it out. Those tests are a bit wonky because they have variable number of agents and different obs spaces and such (to test things work in those cases)

[elliottower]

Overall looks good just some questions and tests to fix

[jjshoots]

The changes look like a much needed cleanup to me. Hats off to you for having the balls to handle something potentially lethal as this. That said, I can't figure out why the tests are failing too and have spent whatever bandwidth I have available in solving it. On my local machine, the tests pass 4/5 times but fail for that 1/5 time, so I'm just rerunning tests and hoping for the best unless someone else wants to have a crack at the bug. 💀

[ffelten]

The changes look like a much needed cleanup to me. Hats off to you for having the balls to handle something potentially lethal as this. That said, I can't figure out why the tests are failing too and have spent whatever bandwidth I have available in solving it. On my local machine, the tests pass 4/5 times but fail for that 1/5 time, so I'm just rerunning tests and hoping for the best unless someone else wants to have a crack at the bug. 💀

There is one bug in the tutorials caused by a SS wrapper. I made a PR there: https://github.com/Farama-Foundation/SuperSuit/pull/235/files. Hopefully fixes this.

Regarding the non-deterministic test, @elliottower said he might take a look. I cannot really make sense of the test scenario tbh.

[ffelten]

@pseudo-rnd-thoughts @jjshoots what do you guys think if we instead make this like Gymnasium 1.0 instead of using __getattr__? How painful would that be?

[jjshoots]

I'm not quite aware of how Gymnasium 1.0 handles it. Could I get a TLDR?

[ffelten]

I'm not quite aware of how Gymnasium 1.0 handles it. Could I get a TLDR?

Farama-Foundation/Gymnasium#535 first point.

[jjshoots]

That seems reasonable. I'm not a huge advocate for that idea, especially the whole "anywhere in the wrapper stack" thing, but I reckon lots of discussion has happened over on Gymnasium's end to conclude that that's the best way to do things. For that reason, I think it's OK.

As to how painful it will be, probably quite painful. Unlike Gymnasium which has very good in-code documentation, PZ has minimal. Lots of things will break that will require stack dives to fix.

Don't get me wrong though, I think this is a worthwhile and needed fix.

[ffelten]

That seems reasonable. I'm not a huge advocate for that idea, especially the whole "anywhere in the wrapper stack" thing, but I reckon lots of discussion has happened over on Gymnasium's end to conclude that that's the best way to do things. For that reason, I think it's OK.

As to how painful it will be, probably quite painful. Unlike Gymnasium which has very good in-code documentation, PZ has minimal. Lots of things will break that will require stack dives to fix.

Well to summarize a bit all: This PR introduces the ability to access attributes from the underlying env.

1. Current version: The breaking changes I foresee on the user side will probably be involving the duplicated variables formerly in the base wrappers that I removed here when using conversions (see the note in the PR description).
2. Gymnasium 1.0 version: If we add this feature, it would be nice to get it right at first and avoid committing the same mistakes as in Gym? But yeah, this will be breaking then actually.

EDIT: I auto-convinced myself of not doing 2. Right now, it seems indeed like quite a lot of changes involved. Let's keep this PR simple and add the feature, plan for the proper Gym-like implementation in the future?

[elliottower]

Yeah makes sense to do it the simple way first then in the future can make it match Gymnasium if need be

[elliottower]

Also I'll have to take a look at the test action mask thing, not sure why it's stochastic seeming but I may just simplify it so the action spaces are always the same or something like that.

[pseudo-rnd-thoughts]

I'm a bit confused by the changes as they don't seem all related to me.
To me, this should be a holistic thing for the whole of PZ, either you remove all __getattr__ implementations everywhere or keep the code the same. This is the painful decision that we made in Gymnasium as we believed (while helpful) causes significant unknown issues for users

[jjshoots]

@pseudo-rnd-thoughts Basically, PZ is transitioning from env.env.env.attribute to the idea of __get_attr__, but the natural question before merge is 'is this the right thing to do or should we be doing what Gymnasium does'.

This is to simplify things like the need to copy termination and truncation signals through all the wrappers.

[elliottower]

Did the SuperSuit release incorporating the wrapper fixes, and looks like the tests pass now.

There's an issue with the Ray CI but that's going to be fixed in another PR, so I'm going to merge this.