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

Gym Env Checker #615

Merged
merged 22 commits into from
Dec 16, 2019
Merged

Gym Env Checker #615

merged 22 commits into from
Dec 16, 2019

Conversation

araffin
Copy link
Collaborator

@araffin araffin commented Dec 12, 2019

Description

Many people have errors because of a custom gym environment, this should address most issues
related to a wrong implementation of the Gym interface.

Motivation and Context

  • I have raised an issue to propose this change (required for new features and bug fixes)

closes #601
related to #595

Types of changes

  • 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 change)
  • Documentation (update in the documentation)

Checklist:

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have ensured pytest and pytype both pass.

@araffin araffin changed the title Feat/env checker Gym Env Checker Dec 12, 2019
docs/guide/rl_tips.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

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

Great feature, glad we have this, will catch a lot of common problems.

High-level feedback is I think this would be clearer if check_env (and tests) were split up into smaller helper methods. Right now if I wanted to add a check I'm not sure it'd take me a while to figure out the right place.

Otherwise looks good, made a few minor comments.

docs/guide/rl_tips.rst Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
tests/test_envs.py Show resolved Hide resolved
tests/test_envs.py Outdated Show resolved Hide resolved
tests/test_envs.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
@araffin
Copy link
Collaborator Author

araffin commented Dec 15, 2019

High-level feedback is I think this would be clearer if check_env (and tests) were split up into smaller helper methods.

Good point, I will do that, thanks for taking the time to review =)

will catch a lot of common problems.

In fact, we should have created that way earlier ( we have more that 50 issues related to custom environments...)

@araffin araffin added this to the v2.9.0 milestone Dec 15, 2019
Copy link
Collaborator

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, it's much clearer now that things have been split into smaller methods.

Made some minor suggestions (mostly explicitly annotating None return type). One thing I'm confused by: why do we need to check method_name == 'reset'?

Otherwise LGTM.

stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
stable_baselines/common/env_checker.py Outdated Show resolved Hide resolved
@araffin
Copy link
Collaborator Author

araffin commented Dec 16, 2019

I reformated the files using pycharm + addressed the comments ;) (+ fix the type in check_image)

Copy link
Collaborator

@AdamGleave AdamGleave left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@araffin araffin merged commit ba51e25 into master Dec 16, 2019
@araffin araffin deleted the feat/env-checker branch December 16, 2019 22:42
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.

2 participants