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

Added tests for environment checker and passive environment checker #2873

Conversation

pseudo-rnd-thoughts
Copy link
Contributor

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Jun 8, 2022

A number of issues have been found the environment checker and passive environment checker.
Due to the critical nature of this feature in gym with it being run on a majority of gym.make, this feature should have as minimal bugs as possible.
This PR adds almost complete coverage of the respective functions ensuring that our environments pass while possible mistakes in environments do not.

Example bugs:
@LucasAlegre found that the invasive environment checker would raise an error if the environment did not use np_random during reset.
@ttran2 integer rewards were not allowed #2878

Code changes:

  • Removed the invasive render checker as it wasn't actually invasive and has just been moved to the passive env checker
  • To fix @LucasAlegre that the RNG are the same and different as a result of reset(seed)
  • Updated a most of the error messages to make them more helpful to debug by specifying the actual error
  • Enabled the render check by default
  • Fixed bug where reward which it can now be a python float / int, np.integer or np.floating
  • Fixed several bugs in check observation space and check obs for non regular observations
  • remove test_core.py this seems largely unnecessary as all tests that more completely done in the rest of the project
  • Added a generic testing environment that allows custom reset, step and render functions for testing
  • Added parameterized testing for almost every warning and error that the user can cause in both environment checker and passive environment checker
  • Pinned pytest to the last python 3.6 version

To do

  • Add tests for environment checker wrapper
  • Run environment check for popular non-gym environments, gym-robotics, sumo-rl, etc

@RedTachyon
Copy link
Contributor

Can you add some tests to make sure this works now? Probably one test env which is deterministic (i.e. doesn't explicitly use RNG, but is implemented correctly), and one which forgets to call super().reset(...)

Also would be nice to have a more descriptive error message. Right now it's just an assertion error, so at the very least there should be a message as to what it tested directly, and ideally there should be some diagnosis (e.g. you probably forgot to call super, if that is the case)

@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Fixed env checker reset seed Added tests for environment checker and passive environment chcker Jun 12, 2022
# Conflicts:
#	gym/utils/passive_env_checker.py
@pseudo-rnd-thoughts
Copy link
Contributor Author

@RedTachyon So I have gone a bit overkill with your comment but I think it has helped find and improve the environment checker
I have updated to the top comment with the changes

@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Added tests for environment checker and passive environment chcker Added tests for environment checker and passive environment checker Jun 13, 2022
@qgallouedec
Copy link
Contributor

I spotted a similar issue in gym.wrappers.env_checker

Once fixed, there is no more undue warning with my env.

@pseudo-rnd-thoughts
Copy link
Contributor Author

Apologies, I thought that in the main env checker. That is now fixed in 28ba9ee

@RedTachyon
Copy link
Contributor

This seems to be a combination of several different PRs at this point? Please try to keep different things separate, this will be very hard to review, and therefore will likely introduce new bugs. This PR should just focus on the env checker fixes and tests, separately there seem to be (1) typo fixes, (2) adding mujoco metadata, (3) plans to change dtype of discrete space samples, (4) an addition to the time limit wrapper, (5) some change in the dockerfile and setup, removing a dependency? (6) removing test_core.py (7) changes to many, many other tests

I'd even go as far as to suggest that you make an effort to split this PR into these separate parts (more or less every bullet point should be a separate PR imo), as frustrating as that might be. Right now there's just so much stuff that's not been discussed tangled together in a single PR that it will cause new bugs. Each of those points will also need to be discussed in terms of "should we do it?", notably I don't see a good reason to remove test_core.py

@pseudo-rnd-thoughts
Copy link
Contributor Author

@RedTachyon I will try and get this to pass then I will close this PR and open a PR for each of the change ideas

@pseudo-rnd-thoughts
Copy link
Contributor Author

Closing this PR, for a number of new PRs that achieve all of the individual parts of this PR

eleurent added a commit to Farama-Foundation/HighwayEnv that referenced this pull request Jun 29, 2022
Some environments use Dict observations, which are not supported by
the env_checker in the latest gym versions (0.24.0/1)

The bug has been fixed in openai/gym#2873
but is yet to be released.
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