Skip to content

Conversation

aslonnie
Copy link
Collaborator

No description provided.

@aslonnie aslonnie requested a review from a team as a code owner October 12, 2025 20:21
@aslonnie
Copy link
Collaborator Author

@aslonnie aslonnie added the go add ONLY when ready to merge, run all tests label Oct 12, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request is a large-scale refactoring of the ray_release test infrastructure, breaking down a monolithic Bazel target into smaller, more modular libraries. This is a significant improvement for maintainability. My review focuses on the correctness of this refactoring, particularly in the context of the PR's goal to fix Windows builds.

I've identified a bug in a path-joining utility that would cause issues on Windows and have suggested a platform-independent fix. Additionally, I've noted that a file archiving function could pollute the current working directory and have proposed using a temporary directory instead. Lastly, I've pointed out a minor discrepancy in a BUILD.bazel file where a non-existent file is being excluded.

Overall, the refactoring is well-executed, and with these adjustments, the code will be more robust and maintainable.

Comment on lines +43 to +52
def _join_cloud_storage_paths(*paths: str):
paths = list(paths)
if len(paths) > 1:
for i in range(1, len(paths)):
while paths[i][0] == "/":
paths[i] = paths[i][1:]
joined_path = os.path.join(*paths)
while joined_path[-1] == "/":
joined_path = joined_path[:-1]
return joined_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of os.path.join is platform-dependent and will produce incorrect paths with backslashes on Windows for cloud storage URIs. Since this pull request aims to fix Windows builds, this should be corrected to use forward slashes consistently. Here is a platform-independent implementation that correctly joins path components for a URL/URI.

def _join_cloud_storage_paths(*paths: str):
    if not paths:
        return ""

    # os.path.join is platform-dependent. Cloud URIs must use forward slashes.
    # This implementation correctly joins path components for a URL/URI.
    full_path = str(paths[0])
    for part in paths[1:]:
        part = str(part)
        if full_path.endswith('/'):
            full_path += part.lstrip('/')
        else:
            full_path += '/' + part.lstrip('/')

    return full_path.rstrip('/')

"ray_release/retry.py",
"ray_release/aws.py",
"ray_release/test.py",
"ray_release/test_automation.py",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This file ray_release/test_automation.py is listed in the exclude block, but it doesn't seem to exist. The other files in the test_automation directory are correctly excluded by the ray_release/test_automation/*.py glob on line 471. This entry is likely a typo and should be removed to avoid confusion.

Comment on lines 46 to 51
def archive_directory(directory_path: str) -> str:
timestamp = str(int(time.time()))
archived_filename = f"ray_release_{timestamp}.zip"
output_path = os.path.abspath(archived_filename)
shutil.make_archive(output_path[:-4], "zip", directory_path)
return output_path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The archive_directory function creates a zip file in the current working directory. This can pollute the CWD and may lead to unexpected behavior if the directory is not writable. It's better to use a temporary directory for creating the archive.

Additionally, the way the output path is constructed and sliced is a bit brittle. shutil.make_archive returns the full path to the created archive, which can be used directly.

Suggested change
def archive_directory(directory_path: str) -> str:
timestamp = str(int(time.time()))
archived_filename = f"ray_release_{timestamp}.zip"
output_path = os.path.abspath(archived_filename)
shutil.make_archive(output_path[:-4], "zip", directory_path)
return output_path
def archive_directory(directory_path: str) -> str:
timestamp = str(int(time.time()))
# Use a temporary directory to avoid polluting the current working directory.
tmp_dir = tempfile.gettempdir()
archive_base_name = os.path.join(tmp_dir, f"ray_release_{timestamp}")
# shutil.make_archive returns the full path to the created archive.
output_path = shutil.make_archive(archive_base_name, "zip", directory_path)
return output_path

@ray-gardener ray-gardener bot added core Issues that should be addressed in Ray Core devprod labels Oct 13, 2025
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Issues that should be addressed in Ray Core devprod go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant