[RLlib; Pre-checks/better failure behavior] Env Checker for Gym Environments #20481
Conversation
rllib/utils/pre_checks.py
Outdated
| Raises: | ||
| ValueError: If env is not one of the supported types. | ||
| """ | ||
| supported_types = (BaseEnv, gym.Env, MultiAgentEnv, RemoteVectorEnv, |
There was a problem hiding this comment.
define this as a constant at the top of the file, and use it also in the type annotation?
can we also start a simple unit test file with this PR? basically try to fail whatever tests we are checking here.
There was a problem hiding this comment.
of course! I will add a constant and add tests here for every one of the checks
There was a problem hiding this comment.
I ended not being able to add this as a constant in this file because of circular imports. Not sure where else I would put it -- maybe in rllib/env/init.py but im not sure if thats necessary. Lmk what you think @gjoliver
rllib/utils/pre_checks.py
Outdated
| def check_env( | ||
| env: [BaseEnv, gym.Env, MultiAgentEnv, RemoteVectorEnv, | ||
| VectorEnv]) -> None: | ||
| def check_env(env: ["BaseEnv", "gym.Env", "MultiAgentEnv", "RemoteVectorEnv", |
There was a problem hiding this comment.
Does anyone know if this will fail sphinx build?
I need to do this because I need to local import these packages otherwise I have a circular import issue.
There was a problem hiding this comment.
Can you use Union["BaseEnv", ...] instead?
Quotes are fine if you want to avoid circular imports.
What also may help is to use from typing import TYPE_CHECKING (search for this in rllib/ to see examples of how imports can be conditioned on whether we are currently type checking or not). But this may not always help.
There was a problem hiding this comment.
Looks great!
Just a few nits (please keep these in mind for future PRs):
- Use unittest.TestCase for your tests (see other test cases on how we do this).
- sort imports in two blocks a) external and b) ray imports.
- within each import block, sort the individual imports alphabetically.
- Try to avoid yellow highlighted code in PyCharm, e.g.:
better:
assert isinstance(reward, (float, int)), ...
instead of:
assert isinstance(reward, [float, int]), ...
|
Let's wait for tests to pass, then merge! |
|
Nice! |
sven1977
changed the title
Why are these changes needed?
Evaluating and checking envs is buried in the codebase, making error messages hard to parse.
I'm going to keep updating this PR, and asking for reviews @sven1977, @richard4912, @gjoliver
It might be best to filter by most recent commits when looking at this pr.
Related issue number
Checks
scripts/format.shto lint the changes in this PR.