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

Fix TimeLimit wrapper and add tests #2888

Merged
merged 2 commits into from Jun 15, 2022

Conversation

araffin
Copy link
Contributor

@araffin araffin commented Jun 13, 2022

Description

Fix TimeLimit wrapper to allow setting the timeout key inside the env.
Add basic testing that was missing before... (I was surprised there were none :/)

Related to #2752

Note: this fix is needed for properly fixing the car racing env too

Type of change

Please delete options that are not relevant.

  • 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 not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Note: the test generate a warning but due to the passive env checker on the Pendulul env.

@pseudo-rnd-thoughts
Copy link
Contributor

The bug fix looks good however the tests look like two tests in one.
In addition, it may be easier to use a generic testing environment like the one proposed in #2873 instead of having to override the environment step for the test.

@araffin
Copy link
Contributor Author

araffin commented Jun 13, 2022

The bug fix looks good however the tests look like two tests in one.

Do you want me to split it into two? one for normal usage and double wrap and one for termination on the last step?

it may be easier to use a generic testing environment like

Feel free to update the test as soon this testing env is merged.
I wrote it that way as it was fast and easy to read (not the cleanest way though).

@pseudo-rnd-thoughts
Copy link
Contributor

Yes, would you be able to split the problem into two
Sure, when the other PR is finalised, I will try to remember to change the environments across

@pseudo-rnd-thoughts
Copy link
Contributor

LGTM

@araffin araffin mentioned this pull request Jun 14, 2022
10 tasks
@jkterry1 jkterry1 merged commit 3498617 into openai:master Jun 15, 2022
@araffin araffin deleted the fix/timeout-wrapper branch September 27, 2022 21:57
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.

None yet

3 participants