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

Test refactoring #2427

Merged
merged 7 commits into from
Sep 28, 2021
Merged

Test refactoring #2427

merged 7 commits into from
Sep 28, 2021

Conversation

RedTachyon
Copy link
Contributor

@RedTachyon RedTachyon commented Sep 28, 2021

This is regarding #2426 and possibly about some other aspects of refactoring the tests.

I won't necessarily have time to finish all of this right now/very fast, but we'll see how much work this needs.

So very much WIP

@RedTachyon
Copy link
Contributor Author

Well this is spicy, tests pass for 3.7 but the imports go out of whack in 3.8

…ortlib.metadata. Also requiring installing importlib_metadata for python 3.8 now.

???????????
@RedTachyon
Copy link
Contributor Author

RedTachyon commented Sep 28, 2021

DO NOT MERGE YET

Soooo I'm super confused. I think I tracked down the error, but I have no clue why it appeared here of all places. @jkterry1 was there any change to the default test environment, dockerfile, anything, during the last few hours? The bug seems to have been lying in the codebase forever if I'm understanding it correctly, so I guess it's possible that moving the tests somehow exposed it?

Basically what happens is:
In Python 3.7, importlib.metadata doesn't exist, so you use importlib_metadata instead (see https://github.com/openai/gym/blob/master/gym/envs/registration.py#L7)
In Python 3.8, importlib.metadata was added, but the EntryPoint class does not have an attr attribute
In Python 3.9, importlib.metadata.EntryPoint gains the attr attribute

The weird part - the registration code uses the attr attribute in every version. Including 3.8, where it doesn't exist. The only way I see this not causing crashes every time you run anything whatsoever is if the loop in https://github.com/openai/gym/blob/master/gym/envs/registration.py#L250 is empty. Which feels odd if this is what happens every single time - why even have that function?

I'm guessing the difference is that before, the only usage of this is in gym.envs.__init__.py#L10, and it probably happens that when it's only called with environments all placed neatly in gym.envs, that loop is somehow empty.

With the tests moved, there is the first environment ever in this codebase whose path does not begin with gym but rather tests. Which somehow makes that loop non-empty, and therefore crashing.


The way I fixed this is basically forcing python 3.8 to still use importlib_metadata instead of importlib.metadata. I don't think this is bad in of itself, but I still feel like I stumbled upon something incredibly cursed here that needs more attention.

Update:

The same issue appeared in #2428 , so I suspect that my guess was, in fact, wrong. I have even less of an idea how this happened.

@jkterry1 jkterry1 merged commit 947b857 into openai:master Sep 28, 2021
@RedTachyon RedTachyon deleted the test-refactor branch October 1, 2021 21:19
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.

2 participants