checking success before splitting result in step_wait

[aaronwalsman]

Description

There is a minor bug in AsyncVectorEnv.step_wait. Here's what happens:

1. If an exception is thrown by the step function of an env inside the worker...
2. ...the worker catches the exception and sends (None, False) back through the pipe to the AsyncVectorEnv.
3. AsyncVectorEnv.step_wait receives the message from the pipe and unpacks it as result, success = pipe.recv(). At this point result = None and success = False.
4. AsyncVectorEnv.step_wait attempts to unpack result further using obs, rew, terminated, truncated, info = result
5. But, because result is None, this immediately throws a TypeError and bypasses the normal error handling using _raise_if_errors.
6. This means exceptions thrown inside the step method of vectorized envs do not get caught correctly, do not generate the proper error messages and are not handled by _raise_if_errors.

My change first checks the value of success before attempting to unpack the contents of result. The values are only unpacked and appended to the intermediate lists (observations_list, rewards, terminateds, truncateds and infos) if success.

Note that this may result in temporarily strange but benign behavior. If there are 8 workers and two of them fail inside step, then the intermediate lists will only have 6 elements, and you would have to look at the list of successes to figure out which corresponded to which worker. However, this is irrelevant because as soon as the loop exits, _raise_if_errors is called, which will handle exceptions, raise further exceptions and nothing will be done with those arrays anyway. The alternative would be to generate dummy variables (obs, rew, terminated, truncated, info = None, 0, False, False, {}) to place in those lists temporarily in the case when success == False but this seems unnecessary.

It looks like you guys are currently revamping the VectorEnv stuff, so hopefully this is still relevant. Feel free to ignore this if this chunk of code is already on the chopping block.

Fixes #177

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist:

I have not done these things, sorry! Excuses:
I can't run pre-commit because of a node dependency that won't run on my system
I haven't added comments because the step_wait method had no comments to begin with, and this is a very minor change.
I haven't done anything to do the documentation because this is a small bugfix.
I don't think these changes generate any new warnings, but again, I can't run pre-commit on my system.
I haven't added any tests for this. Again, small bugfix.
I can't run the unit-tests locally.

• 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

Anyway, hopefully this is useful! Sorry for not being able to run the checks!

[pseudo-rnd-thoughts]

Thanks for the PR and spotting the issue.
Would you be interested in contributor more to Gymnasium?

[aaronwalsman]

Hey, thanks for quickly pulling this in! I may be able to contribute a few small things here and there, but my time is a bit limited these days as I'm trying to wrap up my PhD :)