Full type hinting #2942
Full type hinting #2942
Conversation
… to fix bug if environment doesn't use np_random in reset
…n the opposite case than was intended to (openai#2871)" This reverts commit 519dfd9.
| obs, info = self.env.reset(**kwargs) | ||
|
|
||
| if self.is_vector_env: |
There was a problem hiding this comment.
This also looks like a change of behavior, is this intended? Can you explain it?
There was a problem hiding this comment.
I believe that it is not a change in behaviour. The problem previous was that info could have been unbounded. To avoid the issue, I have all of the return info code in one part of the if statement and without info is the second part. This has the cost of copying the if vector code twice but it shouldn't be a issue
| finally: | ||
| env.close() | ||
|
|
||
| env = AsyncVectorEnv(env_fns, shared_memory=shared_memory) |
There was a problem hiding this comment.
This is one of the situations where it might be better to do a pyright ignore, or maybe something more clever.
Basically what might (maybe?) happen here is that something crashes inside the environment, the test gets aborted, and then the environment is never properly closed, which can lead to a memory leak, because there are some hanging processes. The finally clause prevents that.
It might be possible by using a feature that's existed in gym very quietly, which is using an env as a context manager:
with AsyncVectorEnv(env_fns, shared_memory=shared_memory) as env:
... # do whateverI'm not 100% sure, but I think this will make sure to close the environment even if it crashes
There was a problem hiding this comment.
(on second thought, I'm not sure if this is implemented for VectorEnv, but it definitely is for normal envs)
There was a problem hiding this comment.
The with style would work as VectorEnv inherit from gym.Env.
However, if the crash is intended then the pytest.raises will catch the error.
Otherwise, the test will fail prevent the CI from passing. Therefore, I don't think that we are losing much
In either case, there won't be a long lasting issue.
# Conflicts: # tests/utils/test_play.py
| env_spec | ||
| for env_spec in all_testing_env_specs | ||
| if any( | ||
| f"gym.{ep}" in env_spec.entry_point | ||
| f"gym.envs.{ep}" in env_spec.entry_point |
There was a problem hiding this comment.
Why was this changed? Is this a bugfix unrelated to the typing?
There was a problem hiding this comment.
It is unrelated to type hinting but it fixes a previous bug
A follow up PR to #2939 that was inspired by #2918 to fully type check the whole gym project (along with tests).
This is removes all files and folders from the exlude folder however more work is required to add the core API to the strict parameter.
However, this should catch any weird or incorrect type hints but at this time not force PRs to include type hints
In the process, I discovered a couple of strange tests for async vector envs which needs fixing however it was outside the scope of this PR