Rewriting of the registration mechanism

[RedTachyon]

Description

This is a total rewrite of gym.envs.regitration as proposed in #2738.

It technically doesn't really add any new features beyond fixing a few things that used not to work, but they weren't necessarily documented, so it's hard to call it a bug fix either.

Instead, this significantly simplifies the implementation of registration so that we can actually start adding features to it

The current version implements all significant* behaviors from the old registration mechanism. The intended mechanism is as follows:

1. The registry is a global dictionary sitting in gym.envs.registration
2. When registering an environment, you create EnvSpec and add it to the registry
3. When creating an environment, you get the appropriate EnvSpec from the registry
4. If the passed environment name is invalid, try to give an extra hint on what went wrong ("You tried to create "cartploe", did you mean "CartPole"?)
5. If the passed environment name does not specify the version, create the newest version
6. There is a support for namespaces in the format namespace/EnvName-vX (this already existed, but was undocumented, so I'll add it to the docs somewhere later)

One thing I have yet to add is adding an optional minimal env checker on make to check if the environment uses the up-to-date API.

*Some low-level details (like the EnvSpecTree) are removed. Also, the error messages may be a bit different, but following the same spirit, and passing the previous tests for where errors and warnings are expected. This is with the exception of one test which was just bugged.

Highlights

To justify this change, here's a comparison of how make changed

In the old implementation, you have gym.make which takes an environment name and any kwargs, and instantly just calls registry.make. This finds the right EnvSpec, and then calls spec.make. The actual env creation logic lives in spec.make.

Now, it's all handled by gym.make -- it finds the right spec in the registry, initializes it, applies whatever wrappers necessary, and returns the environment.

This is significant when trying to change something about gym.make. Previously, you'd have to propagate any changes across the three functions. Now you just change the one function responsible for that.

An extra bug that this accidentally solves is something that lived in the register function. In the old code, all of its logic was in a try-except-finally block, where the actual registration happened in finally. So even if you completely malformed your environment registration, it will throw an exception and then... still register it.

Notably, EnvSpecTree has been removed and replaced with a dictionary and a few functions. This might cause a worse asymptotical lookup complexity if someone has thousands of environments and namespaces registered, and tries to create an incorrectly named environment, but that's about it (if you have the right environment name, the lookup is still just a dictionary lookup)

Note:

This is a very core part of the code, so will require significant review. I'm going for full backwards compatibility in terms of make, register and spec.

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

[pseudo-rnd-thoughts]

Thanks for the changes, I can't see any obvious issues with the code though I haven't done a thorough review
You say that you want to preserve full backward compatibility of the core feature but some of the low-level features have been removed.
Can you think of a reasonable case where removing the low-level features could break someone's code? I.e., should this be released as part of 1.0 or can this be pre 1.0?

[RedTachyon]

I don't think there are reasonable cases where this would break someone code. The removed features are undocumented implementation details and weren't intended for end-user usage as far as I know.

But you never know

[ikamensh]

Left some generic software quality comments. Re: docstrings, even a one-liner re-affirming what you would guess from the name can help reader be confident they didn't misunderstand the purpose of a method.

In general I'm also a fan of adding one-liner comments to unit test to say what it's intended to verify, but this is not a core contribution of this PR anyway.

[younik]

Well done! LGTM

[pseudo-rnd-thoughts]

As we build for python 3.7 and greater then this is still needed?

[RedTachyon]

Yea? We're still building for 3.7, so that will still have the else condition, unless I'm misunderstanding something

[pseudo-rnd-thoughts]

I tested for python 3.7 and without the from __future__ import annotation then the import fails however with the __future__ I didnt have an issue

[arjun-kg]

Registering the exact same ID again now raises an error as opposed to overriding with a warning before. Is this intentional? (Would this be in issue for backward compatibility or would noone ever intentionally do this?)

Also, couple of things not related to the PR changes, but part of registration -

1. The malformed env error is not human readable - Currently all IDs must be of the form re.compile('^(?:(?P<namespace>[\\w:-]+)\\/)?(?:(?P<name>[\\w:.-]+?))(?:-v(?P<version>\\d+))?$').
2. The parser allows the id even if it's something like Env-v1.3, or Env-v1-v1.2 putting everything into name with version=None . This might be an issue when someone registers envs as - Env-v1.1 , Env-v1.2, Env-v1.3 these all get registered as different names with no version without warning.

[RedTachyon]

@arjun-kg Thanks, I changed back the error-warning thing during registration. I changed the error message because that's fairly straight-forward and doesn't actually change functionality. Updating the parser would be a whole other mess, so keeping this outside the scope of this

[RedTachyon]

I think I addressed everything, a few things that I'm deliberately keeping out of scope for now (so that the basic functionality can be merged faster) are:

• The parser regex might be a bit iffy with weirdly written versions
• Plugin system might require an extra warning when failing to import a plugin
• Some tests use a weird construct to emulate with pytest.raises, so they might need a revamp
• Add an extra env checker when doing gym.make (this would be a completely new feature, so I'd keep it for another PR - for now this just replicates the existing features)

[Markus28]

@RedTachyon what about the comment about check_name_exists? Why don't we need to filter the specs by namespace?

[RedTachyon]

@Markus28 I don't think I see the comment?

[Markus28]

@RedTachyon Hmm idk, maybe something weird is going on with GitHub, I can't link the comments and it doesn't show up in the review section. These were my comments, I am mostly concerned about the second one:

[RedTachyon]

@Markus28 that's like the second time I'm seeing the same issue in a week (from another person), you entered these comments as a review and need to click a green button somewhere on top of the page to submit the review for them to actually show up

[Markus28]

Thanks, forgot that

[Markus28]

I'm pretty sure this will not work for ALE! In __relocation__.py, the package is called "ale-py", which is indeed what needs to be used for pip install, but it has to be imported as ale_py. Even if ale-py is installed, this branch will be taken (which it shouldn't imo).

[RedTachyon]

So I think this was copy-pasted verbatim from the previous version of the code, it's possible it doesn't work, but that's out of scope here

[Markus28]

I'm not completely sure whether I really understand what this is supposed to do. It is my understanding that it checks

• the existence of the namespace
• the existence of the name

separately.
If we assume we had a Box2D namespace something like check_name_exists("Box2D", "AirRaid") would pass this test, even though there is no environment that matches the pattern "Box2D/AirRaid-v?", is that correct? Do we really want this behavior?
I would have expected something like if spec_.namespace == ns in line 137.

[RedTachyon]

Good catch, I'll add the if spec_.namespace == ns condition

[Markus28]

Three uniformed questions:

• I can't parse the regex where at the beginning of the file, but we expect version to be of the form v<integer>, right? Maybe that should be mentioned in the docstring, otherwise people will try to use integers
• If namespace is not specified, do we expect id to be of the form /<env-name>-v<version>? The line

  gym/gym/envs/registration.py

  Line 399 in 4cc6edf

  full_id = (current_namespace or "") + id

  seems to indicate so. I guess I'm just confused where the slash between namespace and env-name comes from in that line current_namespace is not None?
• If namespace is specified and current_namespace is not None, the old implementation would override the namespace from the id. There is also a corresponding warning in this line but I don't see how anything is being overwritten at any point? Just looking at the code, it seems to me that we would just get a malformed id?

[RedTachyon]

• Updated
• Good point, this actually breaks with namespace, I'll fix it
• Yep, I'll fix that too

[Markus28]

Unless I am misunderstanding the point of this section, I would advise replacing unversioned_spec with unversinoed_spec is not None or putting is not None after the line above. It will work like this but I don't really like the mechanism because it causes bugs on a regular basis (especially when working with integers that might be 0) and it's harder to parse.

[RedTachyon]

Fixed