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

Add testing for step api compatibility functions and wrapper #3028

Merged

Conversation

pseudo-rnd-thoughts
Copy link
Contributor

@pseudo-rnd-thoughts pseudo-rnd-thoughts commented Aug 16, 2022

Issue #3021 noted an issue with the step api compatibility.
Investigating, I found that there was no full testing for the functions.
This PR adds testing with expected values and roundtripping for both conversion functions.

Using is_vector_env works for both dict and list based info

Changes

  • For the old done step api, in all cases where termination or truncation is true then "TimeLimit.truncation" is added to info
  • As the old done step api can only encode 3 termination and truncation states (we cannot include terminated=True and truncated=True), in cases where terminated=True and truncated=True, we favour terminated=True and set truncated=False.

Copy link
Contributor

@arjun-kg arjun-kg left a comment

Choose a reason for hiding this comment

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

Thank you especially for the super detailed test functions. Looks good except for some minor comments.

gym/wrappers/step_api_compatibility.py Outdated Show resolved Hide resolved
tests/utils/test_step_api_compatibility.py Show resolved Hide resolved
tests/utils/test_step_api_compatibility.py Show resolved Hide resolved
@arjun-kg
Copy link
Contributor

Also in step_api_compatibility.py inside utils, the function names haven't been updated, which seems to be what CI is complaining about.

@vwxyzjn
Copy link
Contributor

vwxyzjn commented Aug 16, 2022

Thank you! I tested on my end, and the scripts run.

@jkterry1 jkterry1 merged commit a8d4dd7 into openai:master Aug 18, 2022
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.

4 participants