-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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] Metrics do-over 06: Remove render_images
/with_render_images
from MAEps and -runner and deprecate env_render
config option.
#45109
Conversation
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.
LGTM. A single question in regard to simplifying rendering for users.
# with the data to be learned from, which should be the only thing an episode | ||
# has to be concerned with). | ||
# RGB uint8 images from rendering the env. | ||
assert render_images is None or observations is not None |
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 like that this goes away from the MAE. On the opposite: the user has now to take care of rendering in a custom Callback, doesn't she? Could we create maybe a callback that is appended as soon as a user wants to render?
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.
Valid point. My take on this is that RLlib with its distributed nature is too complicated to make this a simple config flag (like we tried this on the old stack). With the custom callback example, the user has all the possibilities now to write their own filtering logic: Do I only want eval episodes? Do I want all EnvRunners to log videos or just index=2?
Do I want all episodes or just the best performing ones? Etc..
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, I wanted to go away from the notion that rendering always means having a live video play out in front of you. Videos are huge things, they should be logged safely to something like WandB and not written en masse to local disk or displayed on a screen and then disappear again.
…s` from MAEps and -runner and deprecate `env_render` config option. (ray-project#45109)
…s` from MAEps and -runner and deprecate `env_render` config option. (ray-project#45109) Signed-off-by: pdmurray <peynmurray@gmail.com>
…s` from MAEps and -runner and deprecate `env_render` config option. (ray-project#45109) Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
…s` from MAEps and -runner and deprecate `env_render` config option. (ray-project#45109)
Metrics do-over 06:
render_images
/with_render_images
from MAEps and MAEnvRunnerrender_env
algo config option (in favor of new WandB and MetricsLogger based examples).Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.