-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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] [CI] Deflake longer running RLlib learning tests for off policy algorithms. Fix seeding issue in TransformedAction Environments #21685
Conversation
Hey @avnishn, I'm not understanding why we have to make this change. We've slowly lowered this reward threshold from -300 to -1000 over the last 7 months; is there a reason why it used to work and now it doesn't? |
So here's my best guess @richardliaw. At some point, we probably tried to decrease the runtime of these tests, by looking at successful reward curves, and picking a reward threshold that SAC should have hit for this environment **within a certain number of timesteps -- so in this case, -700 within 10k timesteps. If you remove the 10k timestep stopping criteria, then SAC can achieve a reward of -150 in 30k timesteps. (As of today)** The original regression test had no stopping criteria based on the number of timesteps that we've trained on, so it probably worked, but was slower in its runtime. I just increase the number of timesteps for the stopping threshold, and increased the reward threshold back to -150. Lets see if the BK runner can run for this many timesteps in the allotted 900s. |
I moved the pendulum sac tests to gpu and that seemed to fix the problem. This is ready to merge. |
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.
looks reasonable.
a couple of questions.
rllib/tests/run_regression_tests.py
Outdated
@@ -97,6 +97,9 @@ | |||
if args.framework in ["tf2", "tfe"]: | |||
exp["config"]["eager_tracing"] = True | |||
|
|||
if int(os.environ.get("RLLIB_NUM_GPUS", "0")): | |||
exp["config"]["num_gpus"] = 1 |
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.
why do you want to force this? we should respect users' input.
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.
We only inject this environment variable in our tests. I wanted to add gpu to our yaml file for the tuned example, but couldn't because then it wouldn't run on most users laptops. So instead, I look for the env variable, which is injected by gpu runners on buildkite, so that tests on the gpu runner take advantage of the gpu on the runner.
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.
oh understand why you need to check the env var this way, I am more suggesting the following:
num_gpus = int(os.environ.get("RLLIB_NUM_GPUS", "0"))
if num_gpus:
exp["config"]["num_gpus"] = num_gpus
I was only curious about the hardcoded number 1 there. I don't see why we want to force the test to only use 1 GPU.
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.
Oh silly me I'm dumb. Yeah you're right
n_step: 3 | ||
rollout_fragment_length: 1 | ||
prioritized_replay: true | ||
prioritized_replay: False |
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.
is this necessary? harder to say where the difference comes from if we change multiple variables in a single PR.
I am hoping using prioritized_replay would help learning, but maybe slowing things down a bit?
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 can change it back, but it's possible that this hurts and doesn't help. I'll remove it and ablate the change, then post the learning curve in the channel.
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, I am really curious to see the learning curve. if we need this, we can keep it no problem, but would love to have a bit better understanding.
rllib/BUILD
Outdated
@@ -46,8 +46,7 @@ | |||
# Additional tags are: | |||
# - "team:ml": Indicating that all tests in this file are the responsibility of | |||
# the ML Team. | |||
# - "needs_gpu": Indicating that a test needs to have a GPU in order to run. | |||
# - "gpu": Indicating that a test may (but doesn't have to) be run in the GPU | |||
# - "gpu_rllib_X": Indicating that a test needs to be run in the GPU |
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.
What does the "X" mean?
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.
The gpu runner buildkite tags for rllib are named gpu_rllib_1 and gpu_rllib_2. I thought c would be a nice placeholder for a number.
rllib/BUILD
Outdated
@@ -198,13 +197,33 @@ py_test( | |||
|
|||
# DDPG | |||
py_test( | |||
name = "learning_tests_pendulum_ddpg", | |||
name = "learning_tests_pendulum_ddpg_tf", |
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.
Wait, shouldn't we do the framework setting the same as the non-GPU tests? Meaning, no framework specifier and the buildkite pipeline definitions pick the correct command line options?
I understand we probably need more than one GPU BK job then. Let me know if this is what you tried to avoid. Happy to do it this way for some limited amount of time.
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.
We could do it the old way, but that would require 3 gpu bk runners instead of 2. I'm not opposed, just didn't want to add bk runners unnecessarily.
I could add more runners though. With 4-5 tests on the runner, the setup for gpu was taking like 30 minutes, with 20 minutes of tests. Perhaps it was a one off, but that lead to a 50 minute test runner, which is probably at the limit of what we want in terms of test time.
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, let's add another runner, then. This shouldn't be a problem.
Using @krfricke's awesome tool for reproducing ci runners, I discovered that the flakey tests don't actually run any faster on gpu. I'm going to revert some of my changes here, including adding multi-gpu runners for the flakey tests.
See my new comment below |
So there was also one other h_param changed since the test was created: |
meanwhile @gjoliver running this experiment with prioritized replay vs without prioritized replay seems to not have made a difference. These are 6 runs, where 3 have prioritized replay, and 3 don't. Meanwhile changing n_steps 3 -> 1 has made it so that the number of timesteps needed to meet the stopping criteria from ~30k -> ~10k |
very cool! then let's keep that diff out of this change. |
Here's a google colab describing some changes that I'm going to make to the TransformedActionSpaceEnv Environments in rllib.examples. https://colab.research.google.com/drive/1BhoZNuG-9NDHqZ8bN7BgPJ_9i0jWWEnr?usp=sharing It explains how seeding was not implemented correctly by the This contributed to the flakiness in the tests that use this environment. |
e47bff1
to
498f202
Compare
2ab8e99
to
7fccf62
Compare
Hitting 10k timesteps seems to be not achievable in the 900 second time limit. This means that the reward threshold must be met in order for the experiment to terminate. This algorithm is seed sensitive on this environment, so I toyed with the reward threshold on various seeds, and ended up fixing the reward threshold for a certain seed. This should give us regression information, while eliminating the possiblilty that the test flakes.
7fccf62
to
21996ec
Compare
The final extremely flakey tests were the ddpg continuous learning tests. I updated them after running them each for 12 seeds (had to given our seeding issue, and then found a rough lower bound for the reward after a certain number of ts runnable on the CI. It has been lowered on the fake gpu ddpg pendulum test, but that is because I also reduced the number of timesteps to hit the reward threshold. @richardliaw could you please merge when tests are passing? |
ping me when tests pass.
…On Thu, Jan 27, 2022 at 3:02 PM Avnish Narayan ***@***.***> wrote:
The final extremely flakey tests were the ddpg continuous learning tests.
I updated them after running them each for 12 seeds (had to given our
seeding issue, and then found a rough lower bound for the reward after a
certain number of ts runnable on the CI. It has been lowered on the fake
gpu ddpg pendulum test, but that is because I also reduced the number of
timesteps to hit the reward threshold.
@richardliaw <https://github.com/richardliaw> could you please merge when
tests are passing?
—
Reply to this email directly, view it on GitHub
<#21685 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCRZZPCD7QQ66UHNSZ7HCTUYHFJVANCNFSM5MILB3GQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
…ake_sac_pendulum
…significantly slower runtime with tf2 eager than other frameworks
@sven1977 I ended up adding a new test runner for tf2 eager long learning continuous off policy tests. In that target I lower the reward threshold for tf2 eager pendulum sac and ddpg tests, since on tf2 eager they run at half the speed of their tf1 and torch counterparts. Could you PTAL and merge if you're ok with this? |
What?? Is this a performance regression? |
I think we've been aware for a while that tf2 run in eager execution mode is 1.7 times slower than tf2 graph mode and tf1. But I could be wrong. I'll let @sven1977 comment on that. |
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.
Looks great! Thanks for fixing and stabilizing these!
Yeah, |
Just waiting for tests ... |
Hitting 10k timesteps seems to be not achievable
in the 900 second time limit. This means that the
reward threshold must be met in order for the
experiment to terminate. This algorithm is seed
sensitive on this environment, so I toyed with the reward
threshold on various seeds, and ended up fixing the
reward threshold for a certain seed. This should
give us regression information, while eliminating the
possiblilty that the test flakes.
Why are these changes needed?
Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.