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.

Sign up for GitHub

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

Jump to bottom

Fix not-registered env #705

Closed
wants to merge 25 commits into from
Closed

Fix not-registered env #705

wants to merge 25 commits into from

Conversation

modanesh
Copy link
Contributor

@modanesh modanesh commented Dec 28, 2021 •
edited
Loading

Description

In the tests/test_run.py, the Pendulum-v0 is changed to Pendulum-v1. Because of the required gym version, there are no Pendulum-v0 registered, so it throws an error. That is also the same case for tests/test_utils.py, where the BreakoutNoFrameskip-v5 is used while the only two valid versions are: BreakoutNoFrameskip-v0 and BreakoutNoFrameskip-v4.

Motivation and Context

Fixing test errors of PR #572

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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 reformatted the code using make format (required)
  • I have checked the codestyle using make check-codestyle and make lint (required)
  • I have ensured make pytest and make type both pass. (required)
  • I have checked that the documentation builds using make doc (required)

Note: You can run most of the checks using make commit-checks.

Note: we are using a maximum length of 127 characters per line

jkterry1 and others added 22 commits September 16, 2021 09:04
@araffin araffin added the PR template not filled Please fill the pull request template label Dec 28, 2021
@Miffyli Miffyli requested a review from araffin December 28, 2021 12:22
@Miffyli
Copy link
Collaborator

Miffyli commented Dec 28, 2021 •
edited
Loading

Hey. For completeness sake, could you fill in the PR template with link to the said PR?

@araffin I am leaving this for you for now as you are more in loop with Gym related updates :)

@modanesh
Copy link
Contributor Author

The PR description is now updated according to the given template.

@araffin araffin removed the PR template not filled Please fill the pull request template label Dec 28, 2021
@araffin
Copy link
Member

araffin commented Dec 28, 2021

Hello,
you should probably do that PR to @jkterry1 fork: https://github.com/jkterry1/stable-baselines3
so we keep only one PR.
I would also wait anyway before merging as doc/tutorial need to be updated too (see comment #572 (comment)) and cap gym version as additional breaking changes have been made (openai/gym#2531).

@jkterry1
Copy link
Contributor

@araffin I asked modanesh to create an updated PR based on mine to take over getting this merged, I closed my PR.

@modanesh can you please update all the docs like antonin requested, update the branch to master, make sure tests pass, and cap the version of gym at the currently release one? (we can do more PRs for future versions of Gym as they release)

@araffin is there anything else that needs to be done here?

@araffin araffin mentioned this pull request Jan 13, 2022
Closed
13 tasks
@vwxyzjn
Copy link
Contributor

vwxyzjn commented Jan 14, 2022 •
edited
Loading

Hey just a quick note: openai/gym#2531 was an issue with the gym's master and I have not encountered any issues with the 0.21.0 release. You can still use things like BreakoutNoFrameskip-v4. One thing you do have to do though is to do pip install AutoROM[accept-rom-license] instead of the atari-py, but that's about it.

Perhaps it's worth merging this PR as is, and putting adopting ALE-v5 environments and docs changes to separate PRs?

vwxyzjn
vwxyzjn suggested changes Jan 14, 2022
View reviewed changes
.github/workflows/ci.yml
@@ -32,8 +32,9 @@ jobs:
pip install .[extra,tests,docs]
# Use headless version
pip install opencv-python-headless
# Tmp fix: ROM missing in the newest atari-py version
pip install atari-py==0.2.5
# Add Atari ROMs
Copy link
Contributor

@vwxyzjn vwxyzjn Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

After the changes to the setup.py as shown below, we should just remove these three lines.

setup.py
@@ -116,7 +116,7 @@
# For render
"opencv-python",
# For atari games,
"atari_py~=0.2.0",
"ale-py~=0.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do

    "ale-py~=0.7.1",
    "autorom[accept-rom-license]~=0.4.2",

Copy link
Member

Choose a reason for hiding this comment

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

I think we should include autorom yes, otherwise it will be confusing.

@araffin araffin added the Maintainers on vacation Maintainers are on vacation so they can recharge their batteries, we will be back soon ;) label Jan 20, 2022
@carlosluis carlosluis mentioned this pull request Jan 22, 2022
Merged
11 tasks
@jkterry1
Copy link
Contributor

@araffin this can be closed in favor of #734, modanesh seems to have quit

@AdamGleave
Copy link
Collaborator

Closing in favor of #734

@AdamGleave AdamGleave closed this Feb 4, 2022
AdamGleave added a commit that referenced this pull request Feb 4, 2022
@carlosluis @jkterry1
@Miffyli @araffin @modanesh @AdamGleave
Gym fixes - Follow up from #705 (#734)
5143cd1 
* fix Atari in CI

* fix dtype and atari extra

* Update setup.py

* remove 3.6

* note about how to install Atari

* pendulum-v1

* atari v5

* black

* fix pendulum capitalization

* add minimum version

* moved things in changelog to breaking changes

* partial v5 fix

* env update to pass tests

* mismatch env version fixed

* Fix tests after merge

* Include autorom in setup.py

* Blacken code

* Fix dtype issue in more robust way

* Fix GitLab CI: switch to Docker container with new black version

* Remove workaround from GitLab. (May need to rebuild Docker for this though.)

* Revert to v4

* Update setup.py

* Apply suggestions from code review

* Remove unnecessary autorom

* Consistent gym versions

Co-authored-by: J K Terry <justinkterry@gmail.com>
Co-authored-by: Anssi <kaneran21@hotmail.com>
Co-authored-by: Antonin RAFFIN <antonin.raffin@ensta.org>
Co-authored-by: modanesh <mohamad4danesh@gmail.com>
Co-authored-by: Adam Gleave <adam@gleave.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vwxyzjn vwxyzjn vwxyzjn requested changes

@araffin araffin Awaiting requested review from araffin

Assignees
No one assigned
Labels
Maintainers on vacation Maintainers are on vacation so they can recharge their batteries, we will be back soon ;)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@modanesh @Miffyli @araffin @jkterry1 @vwxyzjn @AdamGleave