-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[RLlib; Docs overhaul] Docstring cleanup: Environments. #19784
[RLlib; Docs overhaul] Docstring cleanup: Environments. #19784
Conversation
…_docstring_cleanup_envs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also a few minor comments.
rllib/env/base_env.py
Outdated
|
||
Args: | ||
env: The environment type to convert/wrap. | ||
make_env: A callable taking an int as input (number of sub-envs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this reads a bit weird, return a list of sub-envs of type env
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, yeah, I'll try to clarify a bit better.
rllib/env/base_env.py
Outdated
env = _ExternalEnvToBaseEnv(env) | ||
elif isinstance(env, VectorEnv): | ||
env = _VectorEnvToBaseEnv(env) | ||
# `env` is already a BaseEnv -> Return as is. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this PR does have some logic changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, I just cleaned up the if block with a "early-out" -> Less indentation.
rllib/env/env_context.py
Outdated
vector_index: When there are multiple envs per worker, this | ||
uniquely identifies the env index within the worker. | ||
Starts from 0. | ||
remote: Whether single sub-environments (in a vectorized env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe individual sub-environments (...) sounds a bit better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I should probably explain better, what a sub-env is :)
Aka "underlying" envs -> The n single (cloned) envs within a vectorized one.
I'll fix this. We should use the same terminology throughout (I like sub-envs as it hints that there are many sub-envs within the actual env, rather than "unwrapped" which could be interpreted as a single env that's just wrapped and there is no vectorization).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using "sub environments" now across the board, sometimes: "underlying sub environments". Also I added explanations as to what that means and made sure it's changes everywhere we used to have "underlying" or "wrapped". I also changed the method: get_underlying
into get_sub_environment
using soft-deprecation.
"""Initializes a RemoteVectorEnv instance. | ||
|
||
Args: | ||
make_env: Callable that produces a single (non-vectorized) env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is interesting. so here we only want make_env to return a single non-vectoriezed env? not like above.
…_docstring_cleanup_envs
Hey @gjoliver , could you take another look? I addressed all your questions and change requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool. looks good. thanks a ton.
…_docstring_cleanup_envs
…_docstring_cleanup_envs # Conflicts: # rllib/agents/callbacks.py
…otherwise, but seem to fail nevertheless with this PR's changes.
…_docstring_cleanup_envs
…_docstring_cleanup_envs
Docstring cleanup: rllib/env folder.
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.