[serve] Migrate parse_uri from _private to _common#64371
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces cross-platform path and URI utilities under ray/_common, including a new is_path helper and a parse_uri function with associated unit tests, and updates ray/serve/schema.py to use the new module. Feedback on the changes includes importing urllib.parse directly to prevent potential runtime errors and refactoring the dynamically constructed Protocol enum into a standard Python class to improve readability, type safety, and IDE autocompletion.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -0,0 +1,32 @@ | |||
| import pathlib | |||
| import urllib | |||
There was a problem hiding this comment.
Importing urllib does not automatically expose the urllib.parse submodule in Python. Relying on other modules to have imported it first can lead to a runtime AttributeError: module 'urllib' has no attribute 'parse'. Please import urllib.parse directly.
| import urllib | |
| import urllib.parse |
| _PROTOCOLS = ( | ||
| # For packages dynamically uploaded and managed by the GCS. | ||
| "gcs", | ||
| # For conda environments installed locally on each node. | ||
| "conda", | ||
| # For pip environments installed locally on each node. | ||
| "pip", | ||
| # For uv environments installed locally on each node. | ||
| "uv", | ||
| # Remote http path, assumes everything packed in one zip file. | ||
| "http", | ||
| # Remote https path, assumes everything packed in one zip file. | ||
| "https", | ||
| # Remote s3 path, assumes everything packed in one zip file. | ||
| "s3", | ||
| # Remote google storage path, assumes everything packed in one zip file. | ||
| "gs", | ||
| # Remote azure blob storage path, assumes everything packed in one zip file. | ||
| "azure", | ||
| # Remote Azure Blob File System Secure path, assumes everything packed in one zip file. | ||
| "abfss", | ||
| # File storage path, assumes everything packed in one zip file. | ||
| "file", | ||
| ) | ||
|
|
||
| _REMOTE_PROTOCOLS = ("http", "https", "s3", "gs", "azure", "abfss", "file") | ||
|
|
||
| Protocol = enum.Enum( | ||
| "Protocol", | ||
| {protocol.upper(): protocol for protocol in _PROTOCOLS}, | ||
| ) | ||
|
|
||
|
|
||
| @classmethod | ||
| def _remote_protocols(cls): | ||
| # Returns a list of protocols that support remote storage. | ||
| # These protocols should only be used with paths that end in | ||
| # ".zip", ".whl", ".tar.gz", or ".tgz". | ||
| return [cls[protocol.upper()] for protocol in _REMOTE_PROTOCOLS] | ||
|
|
||
|
|
||
| Protocol.remote_protocols = _remote_protocols |
There was a problem hiding this comment.
Instead of dynamically constructing the Protocol Enum using the functional API and monkey-patching a classmethod, it is much cleaner, more readable, and type-safe to define it as a standard Python class. This also enables better IDE autocompletion and static type checking.
_REMOTE_PROTOCOLS = ("http", "https", "s3", "gs", "azure", "abfss", "file")
class Protocol(enum.Enum):
# For packages dynamically uploaded and managed by the GCS.
GCS = "gcs"
# For conda environments installed locally on each node.
CONDA = "conda"
# For pip environments installed locally on each node.
PIP = "pip"
# For uv environments installed locally on each node.
UV = "uv"
# Remote http path, assumes everything packed in one zip file.
HTTP = "http"
# Remote https path, assumes everything packed in one zip file.
HTTPS = "https"
# Remote s3 path, assumes everything packed in one zip file.
S3 = "s3"
# Remote google storage path, assumes everything packed in one zip file.
GS = "gs"
# Remote azure blob storage path, assumes everything packed in one zip file.
AZURE = "azure"
# Remote Azure Blob File System Secure path, assumes everything packed in one zip file.
ABFSS = "abfss"
# File storage path, assumes everything packed in one zip file.
FILE = "file"
@classmethod
def remote_protocols(cls):
# Returns a list of protocols that support remote storage.
# These protocols should only be used with paths that end in
# ".zip", ".whl", ".tar.gz", or ".tgz".
return [cls[protocol.upper()] for protocol in _REMOTE_PROTOCOLS]| @@ -19,7 +18,7 @@ | |||
| ) | |||
|
|
|||
| from ray._common.logging_constants import LOGRECORD_STANDARD_ATTRS | |||
There was a problem hiding this comment.
let's drop parse_uri from ray._private.runtime_env.packaging.
There was a problem hiding this comment.
Fixed and updated the PR description.
I have updated the remaining callers in ray._private to import parse_uri from ray._common.runtime_env_uri instead.
| @@ -10,12 +9,11 @@ | |||
| from pathlib import Path | |||
| from tempfile import TemporaryDirectory | |||
| from typing import Callable, List, Optional, Tuple | |||
| from urllib.parse import urlparse | |||
| from zipfile import ZipFile | |||
|
|
|||
| from filelock import FileLock | |||
|
|
|||
There was a problem hiding this comment.
can we drop is_path from path_utils?
There was a problem hiding this comment.
Fixed. I removed ray._common.path_utils and inlined the path check as a private helper in runtime_env_uri.py.
Description
This PR continues the migration from
ray._privatetoray._common. Ray Serve previously importedparse_urifromray._private.runtime_env.packaging. This PR movesparse_uriand its minimal dependencies into_common, updates Serve to import from_common, and removes the privateparse_uriimplementation from runtime-env packaging.Changes:
ray._common.runtime_env_uriProtocol,is_pathandparse_urifor runtime-env URI parsing.Private runtime-env updates
parse_urifromray._private.runtime_env.packaging.parse_urifromray._common.runtime_env_uri.Protocolenum inray._private.runtime_env.protocol, while keeping privatedownload_remote_uribehavior attached there.Ray Serve import update
python/ray/serve/schema.pyto importparse_urifromray._common.runtime_env_uriinstead ofray._private.runtime_env.packaging.Tests
python/ray/_common/tests/test_runtime_env_uri.py.TestParseUritest cases frompython/ray/tests/test_runtime_env_packaging.py.python/ray/_common/tests/BUILD.bazel.Related issues
Related to #53478.
Additional information
parse_urifunction. It does not attempt to migrate or refactor the broaderruntime_envpackage.