[RLlib] Add eval worker sub-env fault tolerance test. #26276
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sven1977
requested review from
gjoliver,
avnishn,
ArturNiederfahrenhorst,
smorad,
maxpumperla,
kouroshHakha and
krfricke
as code owners
kouroshHakha
reviewed
Jul 5, 2022
rllib/evaluation/sampler.py
Outdated
| @@ -1082,6 +1081,7 @@ def _process_observations( | |||
| # If reset is async, we will get its result in some future poll. | |||
| elif resetted_obs != ASYNC_RESET_RETURN: | |||
| new_episode: Episode = active_episodes[env_id] | |||
| assert not new_episode.is_faulty | |||
There was a problem hiding this comment.
suggestion: Add an informative error message here.
kouroshHakha
reviewed
Jul 5, 2022
…eval_worker_sub_env_failure_tests
sven1977
commented
Jul 8, 2022
| @@ -2,8 +2,8 @@ cartpole-crashing-pg: | |||
| env: ray.rllib.examples.env.cartpole_crashing.CartPoleCrashing | |||
| run: PG | |||
| stop: | |||
| episode_reward_mean: 150.0 | |||
| timesteps_total: 120000 | |||
| evaluation/episode_reward_mean: 180.0 | |||
There was a problem hiding this comment.
Make sure, learning success is measurable by eval workers.
sven1977
commented
Jul 8, 2022
| @@ -83,13 +83,16 @@ def check_memory_leaks( | |||
| action_sample = action_space.sample() | |||
|
|
|||
| def code(): | |||
| horizon = algorithm.config["horizon"] or float("inf") | |||
There was a problem hiding this comment.
Respect horizon setting in this utility.
sven1977
commented
Jul 8, 2022
| @@ -76,6 +76,7 @@ | |||
| from ray.rllib.utils.test_utils import check_learning_achieved | |||
|
|
|||
| parser = argparse.ArgumentParser() | |||
| parser.add_argument("--evaluation-parallel-to-training", action="store_true") | |||
There was a problem hiding this comment.
Add parallel option to this script and add setting this option to tests in BUILD.
sven1977
commented
Jul 8, 2022
| @@ -347,6 +350,30 @@ def test_no_env_but_eval_workers_do_have_env(self): | |||
| bc.train() | |||
| bc.stop() | |||
|
|
|||
| def test_eval_workers_on_infinite_episodes(self): | |||
There was a problem hiding this comment.
New test to check for proper behavior if eval workers are set to run by episodes, but episodes dont terminate.
sven1977
commented
Jul 8, 2022
| @@ -175,6 +175,7 @@ def __init__(self, algo_class=None): | |||
| self.evaluation_interval = None | |||
| self.evaluation_duration = 10 | |||
| self.evaluation_duration_unit = "episodes" | |||
| self.evaluation_sample_timeout_s = 180.0 | |||
There was a problem hiding this comment.
New config value. Triggers a meaningful warning if breached with various things to try to fix the timeouts.
sven1977
commented
Jul 8, 2022
| @@ -2339,6 +2358,14 @@ def _run_one_evaluation( | |||
| "recreate_failed_workers" | |||
| ), | |||
| ) | |||
|
|
|||
| # Add number of healthy evaluation workers after this iteration. | |||
There was a problem hiding this comment.
This metric was missing.
sven1977
commented
Jul 8, 2022
| @@ -753,7 +753,8 @@ def duration_fn(num_units_done): | |||
| env_steps_this_iter += batch.env_steps() | |||
| metrics = collect_metrics( | |||
| self.workers.local_worker(), | |||
| keep_custom_metrics=self.config["keep_per_episode_custom_metrics"], | |||
| keep_custom_metrics=eval_cfg["keep_per_episode_custom_metrics"], | |||
| timeout_seconds=eval_cfg["metrics_episode_collection_timeout_s"], | |||
There was a problem hiding this comment.
bug: timeout setting was not passed to eval collect-metrics call.
kouroshHakha
approved these changes
Jul 8, 2022
…eval_worker_sub_env_failure_tests
…eval_worker_sub_env_failure_tests
…eval_worker_sub_env_failure_tests
…eval_worker_sub_env_failure_tests
Signed-off-by: sven1977 <[email protected]>
…eval_worker_sub_env_failure_tests
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
Signed-off-by: sven1977 <[email protected]>
xwjiang2010
pushed a commit
to xwjiang2010/ray
that referenced
this pull request
Jul 19, 2022
…crashes during `reset()`; +more tests and logging; add eval worker sub-env fault tolerance test. (ray-project#26276) Signed-off-by: Xiaowei Jiang <[email protected]>
Stefan-1313
pushed a commit
to Stefan-1313/ray_mod
that referenced
this pull request
Aug 18, 2022
…crashes during `reset()`; +more tests and logging; add eval worker sub-env fault tolerance test. (ray-project#26276) Signed-off-by: Stefan van der Kleij <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
restart_failed_sub_environmentsfeature for multi-agent settings (incl. multi-agent w/remote_worker_env=True).restart_failed_sub_environmentsin case a sub-environment fails during itsreset()call (this was previously not supported).restart_failed_sub_environmentsfor multi-agent cartpole (crashing once in a while) and multi-agent cartpole with remote worker envs (crashing once in a while).TODO: Documentation PR that better explains what happens under the hood in case
restart_failed_sub_environments=Trueand an environment fails during step or reset.Why are these changes needed?
Related issue number
Checks
scripts/format.shto lint the changes in this PR.