[core] add "custom_setup" tag to all python cgroup tests #59231
Conversation
avoid adding more tags in `--except-tags` when using `custom_setup` already Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to simplify CI configuration by adding the custom_setup tag to Python cgroup tests and then removing the cgroup tag from the exclusion list in Buildkite test jobs. While the intent is good, this change might have unintended side effects. Specifically, it could cause non-Python tests that are only tagged with cgroup to run in Python-specific test jobs. I've added comments to highlight this potential issue and suggest a more robust approach.
| --install-mask all-ray-libraries | ||
| --workers "$${BUILDKITE_PARALLEL_JOB_COUNT}" --worker-id "$${BUILDKITE_PARALLEL_JOB}" --parallelism-per-worker 3 | ||
| --except-tags custom_setup,cgroup | ||
| --except-tags custom_setup |
There was a problem hiding this comment.
This change simplifies the tag exclusion, which is a good goal. However, by removing cgroup from except-tags, any test tagged solely with cgroup (and not custom_setup) will now be executed by this job. This PR handles this for Python cgroup tests, but other tests (e.g., C++ tests under the core target) might only have the cgroup tag. This could lead to unintended tests running in this Python test job. A safer approach would be to add the custom_setup tag to all cgroup tests that could be picked up by this job's targets, not just the Python ones.
| --workers 4 --worker-id "{{matrix.worker_id}}" --parallelism-per-worker 3 | ||
| --python-version {{matrix.python}} | ||
| --except-tags custom_setup,cgroup | ||
| --except-tags custom_setup |
There was a problem hiding this comment.
Similar to my other comment, removing cgroup from except-tags here could cause non-Python tests that are only tagged with cgroup to run as part of this Python test job. This could be an unintended side effect.
| --workers "$${BUILDKITE_PARALLEL_JOB_COUNT}" --worker-id "$${BUILDKITE_PARALLEL_JOB}" --parallelism-per-worker 3 | ||
| --test-env=TEST_EXTERNAL_REDIS=1 | ||
| --except-tags custom_setup,cgroup | ||
| --except-tags custom_setup |
There was a problem hiding this comment.
The same concern about unintentionally running non-Python cgroup tests applies to this job as well. Removing cgroup from the exclusion list without ensuring all cgroup tests under this job's targets also have custom_setup could lead to unexpected test executions.
avoid adding more tags in
--except-tagswhen usingcustom_setupalready