[ci] Add docstrings to raydepsets CLI functions #59909
Conversation
Add documentation to 20 functions in ci/raydepsets/cli.py that were missing docstrings, improving code readability and maintainability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
![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 docstrings to many functions in ci/raydepsets/cli.py, which is a great step towards improving code readability and maintainability. I've reviewed the new docstrings and they are generally well-written and accurate.
My feedback focuses on making the docstrings more complete by documenting the exceptions that can be raised by the functions. This is important for developers using these functions to handle potential errors correctly. I've provided suggestions to add Raises sections to several docstrings, following the Google Python Style Guide.
| return (self.get_path(output_path), (Path(self.temp_dir) / output_path)) | ||
|
|
||
| def _build(self, build_all_configs: Optional[bool] = False): | ||
| """Build the dependency graph from config depsets.""" |
There was a problem hiding this comment.
The docstring is a good start, but it's incomplete. This function can raise a ValueError if an invalid operation is found in a depset. It's important to document this behavior in the docstring for better maintainability and to help other developers understand the function's contract.
"""Build the dependency graph from config depsets.
Raises:
ValueError: If a depset has an invalid operation.
"""| self.build_graph = self.build_graph.subgraph(nodes).copy() | ||
|
|
||
| def execute(self, single_depset_name: Optional[str] = None): | ||
| """Execute all depsets in topological order, optionally limited to a single depset.""" |
There was a problem hiding this comment.
The docstring is helpful, but it's missing information about potential exceptions. This function calls _get_depset, which can raise a KeyError if single_depset_name is provided but not found. This should be documented.
"""Execute all depsets in topological order, optionally limited to a single depset.
Raises:
KeyError: If `single_depset_name` is provided but not found.
"""| def exec_uv_cmd( | ||
| self, cmd: str, args: List[str], stdin: Optional[bytes] = None | ||
| ) -> str: | ||
| """Execute a uv pip command with the given arguments.""" |
There was a problem hiding this comment.
The docstring should mention that this function can raise a RuntimeError if the uv pip command fails. Documenting potential exceptions is crucial for robust error handling.
"""Execute a uv pip command with the given arguments.
Raises:
RuntimeError: If the uv pip command fails.
"""| return status.stdout.decode("utf-8") | ||
|
|
||
| def execute_pre_hook(self, pre_hook: str): | ||
| """Execute a pre-hook shell command.""" |
There was a problem hiding this comment.
The docstring is missing information about exceptions. This function can raise a RuntimeError if the pre-hook command fails. Please add this to the docstring.
"""Execute a pre-hook shell command.
Raises:
RuntimeError: If the pre-hook command fails.
"""| ) | ||
|
|
||
| def read_lock_file(self, file_path: Path) -> List[str]: | ||
| """Read and return the contents of a lock file as a list of lines.""" |
There was a problem hiding this comment.
The docstring should be expanded to include the Raises section. This function raises a RuntimeError if the lock file does not exist.
"""Read and return the contents of a lock file as a list of lines.
Raises:
RuntimeError: If the lock file does not exist.
"""| return Path(self.workspace.dir) / path | ||
|
|
||
| def check_subset_exists(self, source_depset: Depset, requirements: List[str]): | ||
| """Verify that all requirements exist in the source depset.""" |
There was a problem hiding this comment.
The docstring should document that this function can raise a RuntimeError. This happens when a requirement is not found in the source depset.
"""Verify that all requirements exist in the source depset.
Raises:
RuntimeError: If a requirement is not found in the source depset.
"""|
|
||
|
|
||
| def _get_depset(depsets: List[Depset], name: str) -> Depset: | ||
| """Find and return a depset by name from a list of depsets.""" |
There was a problem hiding this comment.
This docstring is a good summary, but it would be more helpful if it followed the Google Python Style Guide format, including Args, Returns, and Raises sections. This function raises a KeyError if the depset is not found, which is important to document.
"""Find and return a depset by name from a list of depsets.
Args:
depsets: A list of depset objects to search through.
name: The name of the depset to find.
Returns:
The depset object with the matching name.
Raises:
KeyError: If no depset with the given name is found.
"""|
|
||
|
|
||
| def _uv_binary(): | ||
| """Get the path to the uv binary for the current platform.""" |
There was a problem hiding this comment.
The docstring is good, but it's incomplete. This function can raise a RuntimeError for unsupported platforms. It's important to document this behavior.
"""Get the path to the uv binary for the current platform.
Returns:
The path to the uv binary.
Raises:
RuntimeError: If the current platform/processor is not supported.
"""
Add documentation to 20 functions in ci/raydepsets/cli.py that were missing docstrings, improving code readability and maintainability. 🤖 Generated with [Claude Code] Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: jasonwrwang <jasonwrwang@tencent.com>
Add documentation to 20 functions in ci/raydepsets/cli.py that were missing docstrings, improving code readability and maintainability.
🤖 Generated with [Claude Code]