Fix get_wrapper_attr / set_wrapper_attr.

[duburcqa]

Description

IMHO, the current implementation of get_wrapper_attr and set_wrapper_attr is flawed because they act inconsistently wrt each other. For instance, these two snippets is failing:

import gymnasium as gym
env = gym.make("CartPole-v1")
env.MY_ATTRIBUTE = True
assert env.get_wrapper_attr("MY_ATTRIBUTE")
env.set_wrapper_attr("MY_ATTRIBUTE", False)
assert not env.get_wrapper_attr("MY_ATTRIBUTE")

import gymnasium as gym
env = gym.make("CartPole-v1")
env.set_wrapper_attr("MY_ATTRIBUTE", False)
assert hasattr(env, "MY_ATTRIBUTE")

First, I think that if the top-most wrapper has the attribute, set_wrapper_attr should set it (by the way, this is what the documentation is stating). When it does not, then it is not clear whether it should raise an exception (basically the same than get_wrapper_attr or create the attribute (at a level to be determined). In the latter case, I think it definitely makes more sense to add the attribute to the top-most wrapper, otherwise the second snippet right above would still be failing. Since has_wrapper_attr is provided, it would still be possible for the user to implement its custom logic to rather add the attribute to the unwrapped environment, but I think in most cases env.unwrapped.MY_ATTRIBUTE = False would do fine. Besides, I think that adding the attribute to the top-most wrapper makes more sense, because it means that set_wrapper_attr and get_wrapper_attr behave exactly as set_attr, get_attr when an attribute exists for a given wrapper, which is what one would expect intuitively I think. Regarding whether set_wrapper_attr should raise an exception when an attribute does not exist at any level in the stack, I think it is more appropriate to add the missing attribute to the top-most layer because it maintains consistency with both set_attr and get_attr once again.

PS: I also made get_wrapper_attr faster by avoiding calling has_attr in conjunction with get_attr. This trick is quite common in the standard library. set_wrapper_attr should also be slightly faster.

Type of change

Please delete options that are not relevant.

• (SILENTLY) Breaking change (fix or feature that would cause some existing functionality that were untested and undocumented to not work as expected)

Checklist:

• I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• 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

[pseudo-rnd-thoughts]

Thanks for the PR

Well done for spotting the bug, its subtle.
I like your improvement to get_wrapper_attr.
I've slightly modified your updated set_wrapper_attr with some comments as I don't like while True loops.

Looking at the tests, I thought we might need to add some more but I think your new ones should fix the edge case that we missed

[duburcqa]

No problem. Indeed, additional tests could be added.

I like your improvement to get_wrapper_attr.

Glad you like it, but why did you remove it from the PR then ? x)

PS: After benchmarking. It was not faster in practice. I don't really know why. I will have a look at it later.

[pseudo-rnd-thoughts]

Glad you like it, but why did you remove it from the PR then ? x)

I'm confused, I thought I was editing a copy of your PR that included it.
If you find a way that is faster then I'm happy to add it

[duburcqa]

I'm confused, I thought I was editing a copy of your PR that included it.

Apparently no !

If you find a way that is faster then I'm happy to add it

After checking, I don't think it will happen. In the particular case where storing the value before returning can be avoided, some kind of copy-elision mechanism is making the function much faster, which ultimately beats the advantage of skipping the use of hasattr. I was aware of this property but I thought the speed up it induces would not be that big. So in the end the current implementation is better.

[pseudo-rnd-thoughts]

I guess the mistake was worth it then 😅