Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix play assertion error #132

Merged
merged 2 commits into from
Nov 16, 2022

Conversation

Markus28
Copy link
Contributor

This PR addresses #126
I just replaced the error message with an actual exception.
I guess logger.error can be a somewhat confusing name for a function that only does logging. Searching through the repo,

logger.error(f"Expected a list of frames, got a {type(frames)} instead.")
seems to have the same problem.

@Markus28 Markus28 changed the title Fix play assertion errror Fix play assertion error Nov 15, 2022
@pseudo-rnd-thoughts
Copy link
Member

Can we have a short test that this works as expected?

@axb2035
Copy link
Contributor

axb2035 commented Nov 15, 2022

@pseudo-rnd-thoughts
Hi. For 'short test' do you mean 1. adding an assert before the conditional i.e.

assert env.render_mode is not None
if env.render_mode not in {"rgb_array", "rgb_array_list"}:

or 2. attaching a script to the PR like:

import gymnasium as gym
from gymnasium.utils.play import play

mapping = {"2": 1,  # Down.
           "4": 0,  # Left.
           "6": 2,  # Right.
           "8": 3,  # Up.
           }

# Break by passing non-supported render_mode.
try:
    play(gym.make('FrozenLake-v1', render_mode='human'), keys_to_action=mapping)
except Exception as e:
    print("Unsupported render_mode test:", str(e))

# Confirm by passing supported render_mode.
try:
    play(gym.make('FrozenLake-v1', render_mode='rgb_array'), keys_to_action=mapping)
    print("No exceptions.")
except Exception as e:
    print("Unexpected exception:", str(e))

@axb2035
Copy link
Contributor

axb2035 commented Nov 15, 2022

BTW: logger does send a error to the console as well so it is providing some feedback on the error to the user running it. Also, if for some reason you were running it remotely you might not have a console, and want to see the output of logs if it fails. Though I appreciate this is probably an extreme edge case.

@pseudo-rnd-thoughts
Copy link
Member

In tests we have a large collection of tests for the whole project using pytest.
In tests/utils/test_play.py, add option 2 in there

@pseudo-rnd-thoughts pseudo-rnd-thoughts merged commit 92d3fa3 into Farama-Foundation:main Nov 16, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants