[release] upgrading 2 release data tests #58419
Conversation
Signed-off-by: elliot-barn <[email protected]>
![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 upgrades two release data tests, read_tfrecords and read_images_comparison_microbenchmark_single_node, to use Python 3.10. The changes are straightforward, adding python: "3.10" to the respective test configurations in release/release_data_tests.yaml. The modifications are consistent with other tests in the file and appear correct. Given the passing CI run, the changes look good to merge.
There was a problem hiding this comment.
Code Review
This pull request updates two release data tests, read_tfrecords and read_images_comparison_microbenchmark_single_node, to use Python 3.10. The changes are straightforward and correct. To improve long-term maintainability, I've suggested using YAML anchors for the Python version. This will help in keeping Python versions consistent across tests and simplify future updates.
| s3://anyscale-imagenet/ILSVRC/Data/CLS-LOC/ --format image --iter-bundles | ||
| - name: read_tfrecords | ||
| python: "3.10" |
There was a problem hiding this comment.
To improve maintainability and ensure consistency, you can define a YAML anchor for the Python version. This makes it easier to update the version across multiple tests in the future. You can then use this anchor in other tests being updated in this PR.
python: &python-3-10 "3.10"| script: python read_from_uris_benchmark.py | ||
|
|
||
| - name: read_images_comparison_microbenchmark_single_node | ||
| python: "3.10" |
There was a problem hiding this comment.
To improve maintainability, you can use the YAML anchor python-3-10 defined for the read_tfrecords test. This ensures that the Python version is consistent and easy to update in one place.
python: *python-3-10
There was a problem hiding this comment.
Code Review
This pull request correctly updates two release data tests, read_tfrecords and read_images_comparison_microbenchmark_single_node, to explicitly use Python 3.10. The changes are straightforward and look good. I have added one comment regarding a potential inconsistency with other tests in the same file that could be addressed for better maintainability.
| s3://anyscale-imagenet/ILSVRC/Data/CLS-LOC/ --format image --iter-bundles | ||
| - name: read_tfrecords | ||
| python: "3.10" |
There was a problem hiding this comment.
While adding python: "3.10" here is correct for the read_tfrecords test, it creates an inconsistency with other tests. Several other tests in this file also use the read_and_consume_benchmark.py script (e.g., read_parquet_{{scaling}}, read_images_{{scaling}}, write_parquet) but do not have an explicit Python version specified. To ensure consistent behavior and improve maintainability, consider updating all tests that use this script to Python 3.10, either in this PR or in a follow-up.
|
why are the other ones failing? |
aslonnie
left a comment
aslonnie
left a comment
There was a problem hiding this comment.
maybe investigate what is happening with the other tests.
Updating 2 read related release data tests:
read_tfrecords
read_images_comparison_microbenchmark_single_node
Passing run here: https://buildkite.com/ray-project/release/builds/65424#_