search

Farama-Foundation / SuperSuit

A collection of wrappers for Gymnasium and PettingZoo environments (being merged into gymnasium.wrappers and pettingzoo.wrappers
Other
441 stars 56 forks source link

Update sb3_vector_wrapper to fix compatibility issues #206

Closed elliottower closed 1 year ago

elliottower commented 1 year ago

Fix: change step_wait() in SB3VecEnvWrapper to return dones rather than terminations and truncations, as SB3 internally uses dones rather than gymnasium/pettingzoo's terminations and truncations.

See https://github.com/DLR-RM/stable-baselines3/pull/1327 and https://github.com/DLR-RM/stable-baselines3/issues/1356

elliottower commented 1 year ago

@jjshoots @pseudo-rnd-thoughts Not sure why the python 3.7 build failed and the others were cancelled, can they be re-run?

pseudo-rnd-thoughts commented 1 year ago

@elliottower Could you update to master, I have added pre-commit and removed flake8 from CI hopefully removing your issue

jjshoots commented 1 year ago

Just adding on to this, I'm not sure if adapting supersuit back to the done package vs. terminated, truncated makes sense. If I'm not mistaken, we were waiting for SB3 to move over to the new API last time.

elliottower commented 1 year ago

Just adding on to this, I'm not sure if adapting supersuit back to the done package vs. terminated, truncated makes sense. If I'm not mistaken, we were waiting for SB3 to move over to the new API last time. There’s a thread on there but araffin was pretty adamant about keeping their internal use of the done API. In my issue I said they should add this kind of feature internally to convert from gymnasium or pettingzoo to vec env but haven’t gotten a response.

They have a function to create a single env from pettingzoo or gymnasium but he said I should write a custom wrapper to do the vector envs, but I couldn’t get it to work. I could try to create a PR there but I don’t know the codebase that well and struggled to figure this out for 3+ days already, this was the only way I could get it working.

pseudo-rnd-thoughts commented 1 year ago

@jjshoots The issue with flake8 is that it requires python>=3.8, this is the reason why I added pre-commit as a replacement of running flake8 in the build ci

pseudo-rnd-thoughts commented 1 year ago

@elliottower After talking about @jjshoots some more, we are uncertain of this PR. The issue is that supersuit updated to gym v0.26 step api however sb3 has taken a long time to make the change. However this change looks to be happening soon, https://github.com/DLR-RM/stable-baselines3/pull/780 Therefore, if we make this change, as soon as sb3 merge the pr above, we will need to revert this change. As a result, I would use this solution locally or use an old version of supersuit but I don't think we should merge this PR

elliottower commented 1 year ago

@elliottower After talking about @jjshoots some more, we are uncertain of this PR. The issue is that supersuit updated to gym v0.26 step api however sb3 has taken a long time to make the change. However this change looks to be happening soon, DLR-RM/stable-baselines3#780 Therefore, if we make this change, as soon as sb3 merge the pr above, we will need to revert this change. As a result, I would use this solution locally or use an old version of supersuit but I don't think we should merge this PR

Sounds good, thanks for looking into it. It seemed from prior comments that they weren’t going to move to the new API but if you think they will then I agree this PR is unnecessary.