[RLlib][CI] Tag those RLlib tests that needs to be tiggered upon Ray data changes#33488
Conversation
kouroshHakha
requested review from
ArturNiederfahrenhorst,
avnishn,
gjoliver,
krfricke,
maxpumperla,
smorad and
sven1977
as code owners
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
| rllib/... | ||
|
|
||
| - label: ":brain: RLlib: RLlib tests on ray data" | ||
| conditions: ["NO_WHEELS_REQUIRED", "RAY_CI_DATA_AFFECTED"] |
There was a problem hiding this comment.
also needs RAY_CI_RLLIB_AFFECTED?
There was a problem hiding this comment.
There is a separate test suite for all of these. This is suite is gonna be specific only to data changes?
There was a problem hiding this comment.
But if rllib is changed, you still want to run these tests, right?
The filters are OR filters, so if one of the conditions applies, the test suite will be run
| - ./ci/env/env_info.sh | ||
| - bazel test --config=ci $(./ci/run/bazel_export_options) | ||
| --build_tests_only | ||
| --test_tag_filters=ray_data, |
There was a problem hiding this comment.
we may want to exclude ray_data tagged tests from the other test suites so they don't get run twice on master
There was a problem hiding this comment.
I see your concern.
I can make this change so that this test suite gets run only once on master and on ci during any data or rllib changes
There was a problem hiding this comment.
+1 excluding the data tests in the other rllib test suites
Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
|
Fixed @amogkam 's concern by excluding the test suite from master builds as they are already run via RLlib's regular test suites. |
krfricke
left a comment
krfricke
left a comment
There was a problem hiding this comment.
Let's definitely exclude the ray_data tag from the other test suites
| - ./ci/env/env_info.sh | ||
| - bazel test --config=ci $(./ci/run/bazel_export_options) | ||
| --build_tests_only | ||
| --test_tag_filters=ray_data, |
There was a problem hiding this comment.
+1 excluding the data tests in the other rllib test suites
| rllib/... | ||
|
|
||
| - label: ":brain: RLlib: RLlib tests on ray data" | ||
| conditions: ["NO_WHEELS_REQUIRED", "RAY_CI_DATA_AFFECTED"] |
There was a problem hiding this comment.
But if rllib is changed, you still want to run these tests, right?
The filters are OR filters, so if one of the conditions applies, the test suite will be run
|
@krfricke So we want the new test suite to only run on ray_data changes. If there are ray_data changes we run the tests under this test suite. If rllib changes we run those tests but they are scattered across different rllib tests suites. To avoid running twice on master, the new rllib + ray data test suite is excluded. Why do you still want to exclude ray_data tag from rllib tests? |
|
If a PR changes both data and rllib, it will run it twice, right? Same on branch commits? Because there is no exclusion policy for the ray_data tests |
…data changes (ray-project#33488) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com> Signed-off-by: elliottower <elliot@elliottower.com>
…data changes (ray-project#33488) Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com> Signed-off-by: Jack He <jackhe2345@gmail.com>
4 participants
Why are these changes needed?
We will add more tests as they get more surfaced up later down the line.
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.