[RLlib] Base env checker, First see #21476 #21569
Conversation
avnishn
force-pushed
the
base_env_checker
branch
from
5aff719
to
0fece34
Compare
avnishn
force-pushed
the
base_env_checker
branch
from
0ea958e
to
e5412e9
Compare
| @@ -37,6 +38,7 @@ def test_obs_and_action_spaces_are_gym_spaces(self): | |||
| with pytest.raises( | |||
| ValueError, match="Action space must be a gym.space"): | |||
| check_env(env) | |||
| del env | |||
There was a problem hiding this comment.
the old review comments are lost (I can't find it anywhere). so start a new thread.
I wonder if this is necessary. not saying one way or another, but we should be consistent.
if there is a real need for explicitly deleting 3rd party env, we should do this everywhere.
There was a problem hiding this comment.
whoops sorry about that -- I resolved it because I thought I had answered your question.
My answer was this:
"no magic here. Just habit, for preventing memory leaks in tests, a habit I formed when developing Metaworld"
There was a problem hiding this comment.
sure, do you want to keep this habit?
I kind of think consistency is very important for a codebase. if there's actual benefit, we should use this convention everywhere.
otherwise, maybe we shouldn't del it explicit here either.
There was a problem hiding this comment.
Yeah, doing this doesn't make sense to me either. I mean, even if there was a memory leak in a test case like this, it wouldn't matter, right?
There was a problem hiding this comment.
Ok I'll go ahead and remove it. Thanks for the feedback.
There was a problem hiding this comment.
Let's leave it for now and merge this! :)
We can discuss offline:
- Do or not do
delin test cases. - Whether we should consistently use
unittestorpytest. I heavily preferunittestas it's much easier to debug with in PyCharm (pytest does not produce output in the interactive shell in the debugger, etc..). But happy to learn otherwise.
There was a problem hiding this comment.
if you use "pytest -s ...", it will stream the output while running the test.
Why are these changes needed?
Add BaseEnvChecker for BaseEnvs. Please first review #21476
Related issue number
Checks
scripts/format.shto lint the changes in this PR.