-
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; RLlib contrib] Move tuned_examples
into rllib_contrib and remove CI learning tests for contrib algos.
#40444
[RLlib; RLlib contrib] Move tuned_examples
into rllib_contrib and remove CI learning tests for contrib algos.
#40444
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.
This is a super large and critical change so we have to be careful and systematic about it. Can you use this check list to verify each part?
- Remove deprecated tests from rllib/BUILD
Algorithm | Done? |
---|---|
A2C | [x] |
A3C | [x] |
Alpha Star | [x] |
Alpha Zero | [x] |
APEX DDPG | [x] |
APEX DQN | [x] |
ARS | [x] |
Bandit | [x] |
CRR | [x] |
DDPG | [x] |
DDPO | [x] |
DT | [x] |
ES | [x] |
Leela Chess Zero | [x] |
MADDPG | [x] |
MAML | [x] |
MBMPO | [x] |
PG | [x] |
QMIX | [x] |
R2D2 | [x] |
SimpleQ | [x] |
SlateQ | [x] |
TD3 | [x] |
- Move tuned_examples/algo_x/.yaml to rllib_contrib/algo_x/tuned_examples/.yaml
Algorithm | Done? |
---|---|
A2C | [x] |
A3C | [x] |
Alpha Star | [x] |
Alpha Zero | [x] |
APEX DDPG | [x] |
APEX DQN | [x] |
ARS | [x] |
Bandit | [x] |
CRR | [x] |
DDPG | [x] |
DDPO | [x] |
DT | [x] |
ES | [x] |
Leela Chess Zero | [x] |
MADDPG | [x] |
MAML | [x] |
MBMPO | [x] |
PG | [x] |
QMIX | [x] |
R2D2 | [x] |
SimpleQ | [x] |
SlateQ | [x] |
TD3 | [x] |
- Remove algorithms/algo_x/tests/* (They should already be inside rllib_contrib/algo_x/tests/*)
Algorithm | Done? |
---|---|
A2C | [x] |
A3C | [x] |
Alpha Star | [x] |
Alpha Zero | [x] |
APEX DDPG | [x] |
APEX DQN | [x] |
ARS | [x] |
Bandit | [x] |
CRR | [x] |
DDPG | [x] |
DDPO | [x] |
DT | [x] |
ES | [x] |
Leela Chess Zero | [x] |
MADDPG | [x] |
MAML | [x] |
MBMPO | [x] |
PG | [x] |
QMIX | [x] |
R2D2 | [x] |
SimpleQ | [x] |
SlateQ | [x] |
TD3 | [x] |
- Add the removed deprecated tests to rllib_contrib CI (maybe we should create one BUILD file for each rllib_contrib algo and use BAZEL command?)
Algorithm | Done? |
---|---|
A2C | [ ] |
A3C | [ ] |
Alpha Star | [ ] |
Alpha Zero | [ ] |
APEX DDPG | [ ] |
APEX DQN | [ ] |
ARS | [ ] |
Bandit | [ ] |
CRR | [ ] |
DDPG | [ ] |
DDPO | [ ] |
DT | [ ] |
ES | [ ] |
Leela Chess Zero | [ ] |
MADDPG | [ ] |
MAML | [ ] |
MBMPO | [ ] |
PG | [ ] |
QMIX | [ ] |
R2D2 | [ ] |
SimpleQ | [ ] |
SlateQ | [ ] |
TD3 | [ ] |
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.
only test_dt.py is copied into dt/tests. The other three are missing from rllib_contrib.
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.
good catch, will fix.
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.
done
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.
only test_simple_q.py is copied into simple_q/tests. The other three are missing from rllib_contrib.
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, no there are only two files in the original repo (test_repro_simple_q
and test_simple_q
) and they are both there already in rllib_contrib being covered by the tests.
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.
shouldn't we delete the original of all these files from rllib/tuned_examples?
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.
Yes, we absolutely should.
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.
Can we dry run one of these BAZEL changes on one algorithm and make sure that the release stack works for that one algorithm? Then we can repeat the process for others.
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.
can we avoid repeating copying run_regression_tests.py for every algorithm?
rllib_contrib/a2c/BUILD
Outdated
|
||
py_test( | ||
name = "learning_tests_cartpole_a2c_microbatch", | ||
main = "tests/run_regression_tests.py", |
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.
can this be something like rllib.tests.run_regression_tests
? we need to share this code between algorithms.
The dry run is already done on A2C. It went ok (all green). |
…b_contrib_remove_all_test_cases
…b_contrib_remove_all_test_cases
…b_contrib_remove_all_test_cases
After latest update:
|
cc: @kouroshHakha , this is ready for final review. Also tests pending ... |
👍
All checklist items are addressed. However, I don't see the original tuned_example yamls and folders deleted, is that intentional? You want to do deletion in another round? |
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.
approved subject to tests passing. Thanks @sven1977 Great work!!
.buildkite/pipeline.ml.yml
Outdated
@@ -153,9 +153,12 @@ | |||
- conda create -n rllib_contrib python=3.8 -y | |||
- conda activate rllib_contrib | |||
- (cd rllib_contrib/a2c && pip install -r requirements.txt && pip install -e ".[development]") | |||
- cp rllib/tests/run_regression_tests.py rllib_contrib/a2c/. |
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 smart. So this does not create any issues when the dependencies are installed independently within each algorithm, right? We just have to make sure run_regression_tests.py remains backward compatible (which better be the case :)), right?
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.
Yes, we'll have to make sure this script, which will be "running along" with the evolving RLlib, will remain backward compatible wrt these "old" contrib tuned_examples files. This could potentially become an issue, for example if we decided to get rid of yaml support (and do everything with py config files). Let me think about this some more. Maybe a better alternative would be to copy this file from the actually installed (ray[rllib]) location ...
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 getting this script now via a wget from the 2.5.1 github location. So this will always be constant (backward compatible), no matter what we do with the master version of this script.
.buildkite/pipeline.ml.yml
Outdated
- ./ci/env/env_info.sh | ||
- pytest rllib_contrib/a2c/tests/ | ||
# Examples: | ||
- python rllib_contrib/a2c/examples/a2c_cartpole_v1.py --run-as-test |
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.
should we move examples to the bazel BUILD file?
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.
Great idea, was thinking about this, too. Will make it cleaner. 👍
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.
done
…b_contrib_remove_all_test_cases
This PR:
learning_tests
from the CI for those algorithms (which use these tuned_examples files). Note that each rllib_contrib algo already has a learning example run by the CI anyways.rllib_contrib
algos. Note that these tests are already run inside the rllib_contrib's own CI.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.