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

Change usages of reset() to obs, info = reset() #214

Merged
merged 22 commits into from
May 15, 2023

Conversation

elliottower
Copy link
Contributor

@elliottower elliottower commented May 14, 2023

This PR fixes a number of issues with both PettingZoo and current Gymnasium versions. Much of the code in SuperSuit uses obs = env.reset() rather than obs, info = env.reset(), so I had to adapt things to work.

setup.py Outdated Show resolved Hide resolved
@@ -183,6 +183,7 @@ def reset(self, seed=None, options=None):

Copy link
Contributor Author

@elliottower elliottower May 15, 2023

Choose a reason for hiding this comment

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

Don't know much about how this wrapper works but it seems to pass the tests despite only returning the observation, so maybe it's fine. Didn't try adding info but could in the future if we want everything to be consistent (this was one of only cases left of not returning info)

Copy link
Contributor Author

@elliottower elliottower May 15, 2023

Choose a reason for hiding this comment

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

I looked at the git blame and I believe there was never any returning of info in this function

@@ -14,7 +14,9 @@ def reset(self, seed=None, options=None):
return self.venv.reset()

def step_wait(self):
return self.venv.step_wait()
obss, rews, terms, truncs, infos = self.venv.step_wait()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change wasn't intentional, I think it must have been due to using an older version of master. I'm going to try removing it and seeing if the tests pass still

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed it, will check back if it passed later

Copy link
Contributor Author

@elliottower elliottower May 15, 2023

Choose a reason for hiding this comment

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

CI passed but we don't have CI testing it with SB3, and looking at the docs they are keeping the old gym v21 API with done instead of truncations/terminations, so I'm going to keep this.
https://stable-baselines3.readthedocs.io/en/master/guide/vec_envs.html

@elliottower
Copy link
Contributor Author

If you look at the 'disable gen env tests which fail' CI the only thing that fails is pre-commit

@elliottower elliottower merged commit 2a60ab1 into Farama-Foundation:master May 15, 2023
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.

1 participant