[Tune; RLlib] Move Tune tests that use RLlib into separate buildkite job. #20016

sven1977 · 2021-11-03T07:53:00Z

Move Tune tests that use RLlib into separate buildkite (BK) job.

Created a tag "rllib" in the tune BUILD file.
Added a new Tune BK job that only runs those tune tests that depend on RLlib.

Why are these changes needed?

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

gjoliver · 2021-11-03T08:05:35Z

.buildkite/pipeline.yml

+    - bazel test --config=ci $(./scripts/bazel_export_options) --build_tests_only --test_tag_filters=-tf,pytorch,-py37,-flaky,-soft_imports,-gpu_only,-rllib python/ray/tune/...
+
+- label: ":octopus: Tune tests and examples {using RLlib}"
+  conditions: ["RAY_CI_TUNE_AFFECTED"]


should you add RAY_CI_RLLIB_AFECTED here, so that these will run whenever rllib is changed?

+1, it may also be worthwhile to take another pass over the condition definitions in determine_tests_to_run.py to double check we're not over/under-testing. Interestingly enough I stumbled upon a recent PR that removes an unused RAY_CI_ONLY_RLLIB_AFFECTED condition, not sure if we might actually want to add it back and use it for other RLLib test suites.

Great point!
I re-instated the RLLIB_AFFECTED vs RLLIB_DIRECTLY_AFFECTED differentiation.

RLLIB_AFFECTED runs a sub-set of RLlib tests (the high-level learning tests + examples) and is triggered by changes in tune, etc.., non-RLlib sub-folders.

RLLIB_DIRECTLY_AFFECTED runs all RLlib tests and is only triggered upon changes in the rllib source files

matthewdeng · 2021-11-03T16:16:58Z

.buildkite/pipeline.yml

+    - bazel test --config=ci $(./scripts/bazel_export_options) --build_tests_only --test_tag_filters=-tf,pytorch,-py37,-flaky,-soft_imports,-gpu_only,-rllib python/ray/tune/...
+
+- label: ":octopus: Tune tests and examples {using RLlib}"
+  conditions: ["RAY_CI_TUNE_AFFECTED"]


+1, it may also be worthwhile to take another pass over the condition definitions in determine_tests_to_run.py to double check we're not over/under-testing. Interestingly enough I stumbled upon a recent PR that removes an unused RAY_CI_ONLY_RLLIB_AFFECTED condition, not sure if we might actually want to add it back and use it for other RLLib test suites.

.buildkite/pipeline.yml

Co-authored-by: matthewdeng <[email protected]>

…o_own_bk_job

sven1977 · 2021-11-03T18:44:56Z

Thanks for reviewing. @matthewdeng , please take another look.

matthewdeng · 2021-11-03T19:01:19Z

.buildkite/pipeline.yml

+    - TUNE_TESTING=1 PYTHON=3.7 ./ci/travis/install-dependencies.sh
+    # Because Python version changed, we need to re-install Ray here
+    - rm -rf ./python/ray/thirdparty_files; rm -rf ./python/ray/pickle5_files; ./ci/travis/ci.sh build
+    - bazel test --config=ci $(./scripts/bazel_export_options) --test_tag_filters=-example,-flaky,-py37,-soft_imports,-gpu_only,-rllib python/ray/tune/...
+    - bazel test --config=ci $(./scripts/bazel_export_options) --build_tests_only --test_tag_filters=example,-tf,-pytorch,-py37,-flaky,-soft_imports,-gpu_only,-rllib python/ray/tune/...


These tests explicitly have -py37, if we upgrade to PYTHON=3.7 can we remove the filter or are these already being tested with 3.7 in a different step?

I believe the py37-tagged tests are run in a separate job here:

- label: ":octopus: Tune/SGD/Modin/Dask tests and examples. Python 3.7" ... - TUNE_TESTING=1 PYTHON=3.7 INSTALL_HOROVOD=1 ./ci/travis/install-dependencies.sh ... HERE ->> - bazel test --config=ci $(./scripts/bazel_export_options) --build_tests_only --test_tag_filters=py37,-flaky,-client python/ray/tune/...

Yeah I think this is outside the scope of this PR but the differentiation was there because the other tests were run on Python 3.6. Now the one marked as "Python 3.7" is running on 3.7 but so are the ones that aren't marked.

matthewdeng · 2021-11-03T19:15:59Z

ci/travis/determine_tests_to_run.py

+    # Whether only the most important (high-level) RLlib tests should
+    # be run.
+    RAY_CI_RLLIB_AFFECTED = 0


nit: It's not clear when reading this when a test should be run with this condition. Is there some rule of thumb that we can describe here so that it's clear when this should be used as a condition instead of RAY_CI_RLLIB_DIRECTLY_AFFECTED?

good point. I think RAY_CI_RLLIB_AFFECTED should focus on high level integration type of tests, since code outside of rllib shouldn't break individual functionalities.
DIRECTLY_AFFECTED should just run everything.

Correct, @gjoliver , that's how I changed it:

Where "high-level tests" == RLlib learning tests.

…ing_separate_tune_plus_rllib_tests_into_own_bk_job # Conflicts: # .buildkite/pipeline.yml

sven1977 · 2021-11-04T10:12:31Z

@gjoliver @matthewdeng , please take another look. Addressed all remaining questions.

gjoliver

Thanks

wip.

b3aa728

sven1977 requested a review from matthewdeng November 3, 2021 07:53

sven1977 assigned matthewdeng Nov 3, 2021

gjoliver reviewed Nov 3, 2021

View reviewed changes

matthewdeng reviewed Nov 3, 2021

View reviewed changes

sven1977 and others added 4 commits November 3, 2021 19:27

Update .buildkite/pipeline.yml

9acfeaf

Co-authored-by: matthewdeng <[email protected]>

Update .buildkite/pipeline.yml

2c7f0da

Co-authored-by: matthewdeng <[email protected]>

Merge branch 'master' into testing_separate_tune_plus_rllib_tests_int…

851ec82

…o_own_bk_job

wip.

ec44a00

matthewdeng reviewed Nov 3, 2021

View reviewed changes

sven1977 added 2 commits November 4, 2021 11:02

Merge branch 'master' of https://github.com/ray-project/ray into test…

6b43376

…ing_separate_tune_plus_rllib_tests_into_own_bk_job # Conflicts: # .buildkite/pipeline.yml

wip

ef13a35

sven1977 added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Nov 4, 2021

matthewdeng approved these changes Nov 4, 2021

View reviewed changes

gjoliver approved these changes Nov 4, 2021

View reviewed changes

sven1977 merged commit 50c30f8 into ray-project:master Nov 4, 2021

sven1977 deleted the testing_separate_tune_plus_rllib_tests_into_own_bk_job branch June 2, 2023 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tune; RLlib] Move Tune tests that use RLlib into separate buildkite job. #20016

[Tune; RLlib] Move Tune tests that use RLlib into separate buildkite job. #20016

sven1977 commented Nov 3, 2021 •

edited

Loading

gjoliver Nov 3, 2021

matthewdeng Nov 3, 2021

sven1977 Nov 3, 2021

matthewdeng Nov 3, 2021

sven1977 commented Nov 3, 2021

matthewdeng Nov 3, 2021

sven1977 Nov 4, 2021

matthewdeng Nov 4, 2021

matthewdeng Nov 3, 2021

gjoliver Nov 3, 2021

sven1977 Nov 4, 2021

sven1977 commented Nov 4, 2021

gjoliver left a comment

[Tune; RLlib] Move Tune tests that use RLlib into separate buildkite job. #20016

[Tune; RLlib] Move Tune tests that use RLlib into separate buildkite job. #20016

Conversation

sven1977 commented Nov 3, 2021 • edited Loading

Why are these changes needed?

Related issue number

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sven1977 commented Nov 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sven1977 commented Nov 4, 2021

gjoliver left a comment

Choose a reason for hiding this comment

sven1977 commented Nov 3, 2021 •

edited

Loading