Skip to content

[CI] Add README for raydepsets dependency lock file management tool#60841

Draft
elliot-barn wants to merge 1 commit intomasterfrom
elliot-barn/raydepsets-readme
Draft

[CI] Add README for raydepsets dependency lock file management tool#60841
elliot-barn wants to merge 1 commit intomasterfrom
elliot-barn/raydepsets-readme

Conversation

@elliot-barn
Copy link
Contributor

Adding read me for raydepsets

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@elliot-barn elliot-barn force-pushed the elliot-barn/raydepsets-readme branch from 868bb29 to 79734eb Compare February 8, 2026 01:01
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 introduces a comprehensive README file for the raydepsets tool. The README is well-structured and covers the tool's purpose, usage, configuration, and internal workings. My review focuses on improving the clarity and precision of the documentation to ensure it's as helpful as possible for users. I've suggested a few minor text changes to clarify the behavior of some CLI options, the subset operation, and to make an example more illustrative. I also pointed out a potential point of confusion regarding a CUDA version string.


## Why

Ray's CI builds containers and test environments for many combinations of Python version (3.10-3.13), platform (Linux x86_64, macOS ARM), and GPU support (CPU, CUDA 12.8). Each combination needs a locked, reproducible set of dependencies. Managing these by hand is error-prone and doesn't scale.
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 README mentions CUDA 12.8, but the configuration files (e.g., rayimg.depsets.yaml) use cu128 in PyTorch index URLs. This can be confusing as PyTorch's official naming scheme for CUDA 12.1 is cu121. To improve clarity and avoid potential confusion for users trying to understand or extend the configurations, please clarify if cu128 is a typo, a custom identifier, or if it indeed corresponds to CUDA 12.8. If it's a custom identifier, a brief note might be helpful.


| Option | Description |
|---|---|
| `CONFIG_PATH` | Path to a `.depsets.yaml` config file (default: `ci/raydepsets/configs/*.depsets.yaml`) |
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 description for CONFIG_PATH states it's a "Path to a .depsets.yaml config file". This is slightly misleading as the tool actually processes all .depsets.yaml files in the directory of the provided path. To make this clearer for users, I suggest updating the description to better reflect this behavior.

Suggested change
| `CONFIG_PATH` | Path to a `.depsets.yaml` config file (default: `ci/raydepsets/configs/*.depsets.yaml`) |
| `CONFIG_PATH` | Path to a config file. All `.depsets.yaml` files in the path's directory will be loaded. (default: `ci/raydepsets/configs/*.depsets.yaml`) |

| `--name` | Build only this depset and its dependencies |
| `--uv-cache-dir` | Cache directory for uv |
| `--check` | Validate lock files match what would be generated; exit non-zero on diff |
| `--all-configs` | Build depsets from all config files, not just the specified one |
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 description for --all-configs is "Build depsets from all config files, not just the specified one". This could be clearer about which "all config files" it refers to. Based on the implementation, it builds depsets from all config files found in the same directory as the CONFIG_PATH argument. I suggest clarifying this to avoid ambiguity.

Suggested change
| `--all-configs` | Build depsets from all config files, not just the specified one |
| `--all-configs` | Build depsets from all config files in the `CONFIG_PATH` directory, not just the one specified in the path. |


#### Operation: `subset`

Extracts a subset of already-resolved dependencies from another depset's lock file. Validates that all requested requirements exist in the source.
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 description for the subset operation says it "Validates that all requested requirements exist in the source." This is slightly ambiguous and could be interpreted as checking for individual packages. The implementation actually verifies that the specified requirement files are part of the source depset's definition. To improve clarity, I suggest making this explicit.

Suggested change
Extracts a subset of already-resolved dependencies from another depset's lock file. Validates that all requested requirements exist in the source.
Extracts a subset of already-resolved dependencies from another depset's lock file. Validates that all requested requirement files exist in the source depset's definition.

Comment on lines +173 to +179
depsets:
- name: my_depset
<<: *common_settings
operation: compile
requirements:
- requirements.txt
output: output.lock
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 example for YAML anchors is helpful. To better illustrate the benefit of using anchors for keeping definitions DRY, consider showing two depsets that reuse the common settings. This would make the advantage of using anchors more apparent.

Suggested change
depsets:
- name: my_depset
<<: *common_settings
operation: compile
requirements:
- requirements.txt
output: output.lock
depsets:
- name: my_depset_1
<<: *common_settings
operation: compile
requirements:
- requirements1.txt
output: output1.lock
- name: my_depset_2
<<: *common_settings
operation: compile
requirements:
- requirements2.txt
output: output2.lock

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant