[release auto] support uploading wheels from arbitrary branch #58344
Conversation
Signed-off-by: Lonnie Liu <[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 adds support for downloading wheels from an arbitrary S3 branch path, which is a good enhancement. The changes in ray_wheels_lib.py and the new test case are mostly well-implemented. However, I've identified a critical bug in upload_wheels_pypi.py related to the ordering of command-line arguments that could lead to incorrect behavior. I have also provided a suggestion to improve test maintainability by reducing code duplication. Please address the critical issue before merging.
| def test_download_ray_wheels_from_s3_with_branch( | ||
| mock_get_wheel_names, mock_check_wheels, mock_download_wheel | ||
| ): | ||
| commit_hash = "1234567" | ||
| ray_version = "1.0.0" | ||
|
|
||
| mock_get_wheel_names.return_value = SAMPLE_WHEELS | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp_dir: | ||
| download_ray_wheels_from_s3( | ||
| commit_hash=commit_hash, | ||
| ray_version=ray_version, | ||
| directory_path=tmp_dir, | ||
| branch="custom_branch", | ||
| ) | ||
|
|
||
| mock_get_wheel_names.assert_called_with(ray_version=ray_version) | ||
| assert mock_download_wheel.call_count == len(SAMPLE_WHEELS) | ||
| for i, call_args in enumerate(mock_download_wheel.call_args_list): | ||
| assert ( | ||
| call_args[0][0] == f"custom_branch/{commit_hash}/{SAMPLE_WHEELS[i]}.whl" | ||
| ) | ||
| assert call_args[0][1] == tmp_dir | ||
|
|
||
| mock_check_wheels.assert_called_with(tmp_dir, SAMPLE_WHEELS) |
There was a problem hiding this comment.
This test function is very similar to test_download_ray_wheels_from_s3. This duplication can make the tests harder to maintain. To improve maintainability, consider refactoring this test and test_download_ray_wheels_from_s3 into a single, parameterized test using pytest.mark.parametrize. This would allow you to test both cases (with and without a branch) with a single test function, reducing code duplication.
ray-gardener
bot
added
core
Signed-off-by: Lonnie Liu <[email protected]>
…oject#58344) so that we can perform surgeries in the last minute if required. Signed-off-by: Lonnie Liu <[email protected]>
…oject#58344) so that we can perform surgeries in the last minute if required. Signed-off-by: Lonnie Liu <[email protected]>
…oject#58344) so that we can perform surgeries in the last minute if required. Signed-off-by: Lonnie Liu <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
…oject#58344) so that we can perform surgeries in the last minute if required. Signed-off-by: Lonnie Liu <[email protected]>
so that we can perform surgeries in the last minute if required.