Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sb3_vector_wrapper to fix compatibility issues #206

Closed
wants to merge 2 commits into from

Conversation

elliottower
Copy link
Member

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 DLR-RM/stable-baselines3#1327
and DLR-RM/stable-baselines3#1356

@elliottower
Copy link
Member Author

@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
Copy link
Member

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

@jjshoots
Copy link
Member

jjshoots commented Mar 5, 2023

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
Copy link
Member Author

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.

Copy link
Member

@jjshoots jjshoots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp, in that case I think this wrapper is fine given Antonin's stance. Could you figure out the flake8 error on Py3.7? Then we can merge.

@pseudo-rnd-thoughts
Copy link
Member

@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

Copy link
Member

@jjshoots jjshoots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elliottower Given Mark's response, I think you just need to update to master for tests to pass. After that, LGTM.

@pseudo-rnd-thoughts
Copy link
Member

@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

@elliottower
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants