-
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] Fix 2 broken CI test cases: test_learner_group
and cartpole_dqn_envrunner
.
#45110
[RLlib] Fix 2 broken CI test cases: test_learner_group
and cartpole_dqn_envrunner
.
#45110
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.
@@ -274,7 +274,7 @@ def _sample_timesteps( | |||
if explore: | |||
env_steps_lifetime = self.metrics.peek( | |||
NUM_ENV_STEPS_SAMPLED_LIFETIME |
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.
NUM_ENV_STEPS_SAMPLED_LIFETIME
is then update during synching the metrics in between sampling?
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.
Correct, it's always updated anyways (this was a bug) after each single env step. Inside self._increase_sampled_metrics()
.
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.
And it's also hard-overwritten by the Algo itself after each iteration in order to sum up all the steps from all the other EnvRunners (making sure each EnvRunner always has the total steps sampled, not just its own lifetime count).
…e_dqn_envrunner`. (ray-project#45110)
…e_dqn_envrunner`. (ray-project#45110) Signed-off-by: pdmurray <peynmurray@gmail.com>
…e_dqn_envrunner`. (ray-project#45110) Signed-off-by: Ryan O'Leary <ryanaoleary@google.com>
…e_dqn_envrunner`. (ray-project#45110)
Fix 2 broken CI test cases:
test_learner_group
andcartpole_dqn_envrunner
.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.