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

Updates the make and registration functions with better parameters and hints #3041

Merged
merged 11 commits into from
Sep 1, 2022

Conversation

pseudo-rnd-thoughts
Copy link
Contributor

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

This PR originally started with fixing the type hints for EnvSpec entry_points where the value could be None by default however this won't work.
After this I looked through the whole file and updated / removed the following sections

  1. Updated hint types / field for entry_point in EnvSpec
  2. Remove the __relocated__.py file and related code. This error seems old and needed removing at some point
  3. Updated make overloads with all environments.
  4. Removed EnvRegistry as noted to be removed at 1.0
  5. Added all EnvSpec attributes as parameters to gym.register

@pseudo-rnd-thoughts
Copy link
Contributor Author

pseudo-rnd-thoughts commented Aug 28, 2022

@RedTachyon I have readded / kept the make overloads. I investigated testing that the overloads were correct.
However, I found that you need to use the typing_extensions.overload rather than the typing.overload which pycharm doesn't recognise. Therefore, I haven't added testing

def test_make_overloads():
    env_make_hints = {}
    for overloaded_func in typing_extensions.get_overloads(gym.make):
        make_type_hints = typing_extensions.get_type_hints(overloaded_func)
        assert (
            "id" in make_type_hints and "return" in make_type_hints
        ), f"Missing type hints for overloaded `gym.make` id and return, {make_type_hints.get('id', 'unknown env')}"
        if not isinstance(make_type_hints["id"], type):
            for env_id in typing_extensions.get_args(make_type_hints["id"]):
                obs_type, act_type = typing_extensions.get_args(
                    make_type_hints["return"]
                )
                env_make_hints[env_id] = (obs_type, act_type)

    assert (
        env_make_hints.keys() == gym.envs.registry.keys()
    ), f"Missing the make overloads envs: {set(gym.envs.registry.keys()) - set(env_make_hints.keys())}. Additional envs: {set(env_make_hints.keys()) - set(gym.envs.registry.keys())}"
    # todo: add type check on the obs and action type

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.

Looks good apart from a minor change

@@ -49,7 +49,7 @@ def __init__(
render_modes: Optional[List[str]] = None,
render_fps: Optional[int] = None,
render_mode: Optional[str] = None,
spec: EnvSpec = EnvSpec("TestingEnv-v0"),
spec: EnvSpec = EnvSpec("TestingEnv-v0", None),
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this should also be "no-entry-point"?

@jkterry1 jkterry1 merged commit 0bcd49e into openai:master Sep 1, 2022
@pzhokhov
Copy link
Collaborator

pzhokhov commented Sep 7, 2022

@pseudo-rnd-thoughts would it make sense to make a string constant, say, EMPTY_ENTRY_POINT="no-entry-point" instead of repeating the same string?

@pseudo-rnd-thoughts
Copy link
Contributor Author

Yes that probaby would have made sense but this is purely in testing and this file alone I believe

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.

6 participants