Improve 'set_wrapper_attr'.

[duburcqa]

Description

I propose to further improve 'set_wrapper_attr' (as a follow-up of PR #1293).

I added the optional keyword argument 'force'. Its role is to make sure that the attribute exists before setting it, so that no attribute while be create on the top-level wrapper. While this could already be done using 'has_wrapper_attr', but specifying the new optional argument is not only more convenient but also faster. Moreover, adding this extra argument enables to refactor the implementation of 'set_wrapper_attr' as a recursive function calling itself on the lower-level environment (in case of wrappers). This is not only more than twice faster (because isinstance is very slow...), but also more robust, as it allows anybody to derive its own wrapper from gym.Env and provides its own implementation of 'set_wrapper_attr' (this aspect is very important to me, because my library is doing exactly this). Finally, the implementation is easier to understand and closely align with get_wrapper_attr.

Type of change

Please delete options that are not relevant.

• New feature (non-breaking change which adds functionality)
• This change requires a documentation update

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
• 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

[duburcqa]

@pseudo-rnd-thoughts Any thought on this PR ?

[pseudo-rnd-thoughts]

Apologies for taking a while to look at it @duburcqa

I like the idea, though I dislike returning an error.
Could we use a return True if the attribute has been updated, then we are checking at the "top" wrapper if the lower wrappers have returned True, else set the attribute there?

This seems like a better way and possibly faster because there is no error checking

[duburcqa]

What I like with the implementation I suggest, is especially the ability to raise an exception. I think it adding an attribute in the base environment is rarely the intended objective. Raising an exception is a nice way to catch silly mistakes for the practitioner. Personally I would even prefer to set force to False by default, but I kept it to True for backward compatibility.

Besides, error CATCHING is slow, but error checking is free of cost (since Python 3.11). Since adding a new attribute is very unlikely, this approach is overall cheaper than if ... else ... causes

[pseudo-rnd-thoughts]

Besides, error CATCHING is slow, but error checking is free of cost (since Python 3.11). Since adding a new attribute is very unlikely, this approach is overall cheaper than if ... else ... causes

Oh, I've learnt something new, but aren't we doing error-catching multiple times if none of the wrappers have the attribute. Also return True / False seems more elegant therefore, I still want to change it

[duburcqa]

I don't like the idea of silently NOT adding the attribute if the user specifies force=False manually. It is just extremely misleading compared to raising an exception, because nobody would expect that the return value should be checked to raise an exception by itself would reading the documentation.

At this point, having to do the check manually and raise an exception does not bring any improvement compared to calling has_wrapper_attr and raising an exception if necessary, which was already supported before this MR.

I just do not see any issue regarding raising an exception as it point out a bug in 99.9% of the cases. And if it was intended, the user can just write env.unwrapped.MY_ATTR = 1 if necessary. This way, everything is very clear.