[core] Unify RuntimeEnv archive validation and support tar.xz - #65742
[core] Unify RuntimeEnv archive validation and support tar.xz#65742zhjwpku wants to merge 2 commits into
Conversation
Centralize package format capabilities for working_dir and py_modules. Preserve and unpack tar.xz packages across local, remote, GCS, and Jobs paths. Closes ray-project#65738 Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request adds support for .tar.xz archives in Ray's runtime_env fields (working_dir and py_modules) and centralizes package extension validation logic in a new runtime_env_package.py module. The reviewer feedback highlights several key improvements: parsing the URI before checking its extension in common.py to handle query parameters correctly, sorting supported extensions by length in get_package_extension to prevent incorrect matching of compound extensions, and passing the original user-provided URI to validate_package_extension to ensure clearer error messages when validation fails.
| if not has_package_extension(package_uri, PACKAGE_UPLOAD_EXTENSIONS): | ||
| formats = ", ".join(PACKAGE_UPLOAD_EXTENSIONS) | ||
| raise ValueError( | ||
| f"package_uri ({package_uri}) does not end in a supported format: " | ||
| f"{formats}" | ||
| ) | ||
| # We need to strip the <protocol>:// prefix to make it possible to pass | ||
| # the package_uri over HTTP. | ||
| protocol, package_name = parse_uri(package_uri) |
There was a problem hiding this comment.
The current implementation checks package_uri directly using has_package_extension. If the URI contains query parameters or fragments (such as presigned S3/GCS URLs or SAS tokens), has_package_extension will return False because the string does not end with the extension.\n\nSince parse_uri already correctly handles stripping query parameters and fragments to extract the clean package_name, we should parse the URI first and then validate the extension on the parsed package_name.
# We need to strip the <protocol>:// prefix to make it possible to pass\n # the package_uri over HTTP.\n protocol, package_name = parse_uri(package_uri)\n if not has_package_extension(package_name, PACKAGE_UPLOAD_EXTENSIONS):\n formats = ', '.join(PACKAGE_UPLOAD_EXTENSIONS)\n raise ValueError(\n f'package_uri ({package_uri}) does not end in a supported format: '\n f'{formats}'\n )There was a problem hiding this comment.
Addressed together with the validation error fix. This call site now passes the query-free original URI as display_path.
| for extension in supported_extensions: | ||
| if path.endswith(extension): | ||
| return extension |
There was a problem hiding this comment.
To make get_package_extension robust against the order of extensions in supported_extensions, we should sort them by length in descending order. This ensures that compound extensions (like .tar.gz) are matched before their single-extension counterparts (like .gz) if both are present in the tuple.
for extension in sorted(supported_extensions, key=len, reverse=True):\n if path.endswith(extension):\n return extensionThere was a problem hiding this comment.
Done. get_package_extension() now sorts the supported extensions by length in descending order before matching.
| def validate_package_extension(path: str, field: str) -> None: | ||
| """Validate a RuntimeEnv package path using the field's capabilities. | ||
|
|
||
| Args: | ||
| path: Package path to validate. | ||
| field: RuntimeEnv field whose format capabilities apply. | ||
|
|
||
| Raises: | ||
| ValueError: If the path does not have an extension supported by the field. | ||
| """ | ||
| supported_extensions = RUNTIME_ENV_PACKAGE_EXTENSIONS[field] | ||
| if has_package_extension(path, supported_extensions): | ||
| return | ||
|
|
||
| formats = ", ".join(supported_extensions) | ||
| raise ValueError( | ||
| f"Only {formats} files are supported for {field} URIs; got {path}." | ||
| ) |
There was a problem hiding this comment.
When validation fails, validate_package_extension raises a ValueError displaying the path argument. However, path is often the internal hashed package name (e.g., gcs_digest.txt) returned by parse_uri, rather than the original URI provided by the user (e.g., gcs://package.txt). This makes the error message confusing for users.\n\nWe can add an optional original_path parameter to validate_package_extension to display the original user-provided URI in the error message when available.
def validate_package_extension(\n path: str, field: str, original_path: Optional[str] = None\n) -> None:\n """Validate a RuntimeEnv package path using the field's capabilities.\n\n Args:\n path: Package path to validate.\n field: RuntimeEnv field whose format capabilities apply.\n original_path: Original user-facing path or URI for error messages.\n\n Raises:\n ValueError: If the path does not have an extension supported by the field.\n """\n supported_extensions = RUNTIME_ENV_PACKAGE_EXTENSIONS[field]\n if has_package_extension(path, supported_extensions):\n return\n\n formats = ', '.join(supported_extensions)\n display_path = original_path or path\n raise ValueError(\n f'Only {formats} files are supported for {field} URIs; got {display_path}.'\n )There was a problem hiding this comment.
Ok, I added an optional display_path to validate_package_extension(). The parsed path is still used for validation, while callers provide the original URI with its query removed for the error message.
| if protocol == Protocol.GCS or protocol in Protocol.remote_protocols(): | ||
| validate_package_extension(path, field) |
There was a problem hiding this comment.
Pass the original uri as original_path to validate_package_extension so that the user-facing error message displays the original URI instead of the internal hashed package name.
if protocol == Protocol.GCS or protocol in Protocol.remote_protocols():\n validate_package_extension(path, field, original_path=uri)There was a problem hiding this comment.
Addressed together with the validation error fix. This call site now passes the query-free original URI as display_path.
| if protocol == Protocol.GCS or protocol in Protocol.remote_protocols(): | ||
| validate_package_extension(path, PY_MODULES) |
There was a problem hiding this comment.
Pass the original URI s as original_path to validate_package_extension so that the user-facing error message displays the original URI instead of the internal hashed package name.
if protocol == Protocol.GCS or protocol in Protocol.remote_protocols():\n validate_package_extension(path, PY_MODULES, original_path=s)| if protocol == Protocol.GCS or protocol in Protocol.remote_protocols(): | ||
| validate_package_extension(path, WORKING_DIR) |
There was a problem hiding this comment.
Pass the original URI working_dir as original_path to validate_package_extension so that the user-facing error message displays the original URI instead of the internal hashed package name.
if protocol == Protocol.GCS or protocol in Protocol.remote_protocols():\n validate_package_extension(path, WORKING_DIR, original_path=working_dir)There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c952912ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 7c95291. Configure here.
|
@edoakes Please take a look if you have time. |
Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>

Description
RuntimeEnv archive handling currently has duplicated and inconsistent suffix checks across validation, local uploads, package caching, downloading, and the Jobs API. In particular, GCS URIs bypass some validation,
working_diraccepts.whltoo early, compound suffixes are not always preserved, and.tar.xzis not supported consistently.Related issues
Closes #65738