[ci][deps] raydepsets build all depsets option #58065
Conversation
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.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 introduces an --all-configs option to build all dependency sets at once. The implementation correctly adds the CLI flag and modifies the DependencySetManager to handle building from multiple configuration files by skipping the graph-subsetting logic when the flag is present. The changes also include new unit tests for the added functionality.
My review focuses on a potential issue with the new default configuration path, which could lead to unexpected behavior, and a minor typing improvement in the test suite. Overall, the changes are in the right direction.
|
|
||
| @cli.command() | ||
| @click.argument("config_path", default="ci/raydepsets/configs/ray.depsets.yaml") | ||
| @click.argument("config_path", default="ci/raydepsets/configs/*.depsets.yaml") |
There was a problem hiding this comment.
Changing the default config_path to a wildcard ci/raydepsets/configs/*.depsets.yaml introduces an issue with the default behavior. When raydepsets build is run without arguments, config_path will be the wildcard, but the --all-configs flag will be False. This leads to subgraph_config_nodes() being called with self.config_name as '*.depsets.yaml', which doesn't match any specific config file names (e.g., 'ray.depsets.yaml'). As a result, the build graph will be empty, and no dependency sets will be built, which is likely not the intended default behavior.
To ensure a sensible default and make the behavior more explicit, I recommend reverting the default path to a single, specific configuration file. Users can then explicitly provide a wildcard path along with the --all-configs flag to build all configurations.
| @click.argument("config_path", default="ci/raydepsets/configs/*.depsets.yaml") | |
| @click.argument("config_path", default="ci/raydepsets/configs/ray.depsets.yaml") |
| def _create_test_manager( | ||
| tmpdir: str, config_path: Optional[str] = None, check: bool = False | ||
| tmpdir: str, | ||
| config_path: Optional[str] = "test.depsets.yaml", |
There was a problem hiding this comment.
The type hint for config_path is Optional[str], which implies it can be None. However, None is not a valid value for this parameter, as it would cause a TypeError in DependencySetManager which expects a str. To accurately reflect that config_path is a required string, please change the type hint to str.
| config_path: Optional[str] = "test.depsets.yaml", | |
| config_path: str = "test.depsets.yaml", |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
adding --all-configs option to raydepsets to build all configs. Added cli and workspace unit tests --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
adding --all-configs option to raydepsets to build all configs. Added cli and workspace unit tests --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
adding --all-configs option to raydepsets to build all configs. Added cli and workspace unit tests --------- Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>
adding --all-configs option to raydepsets to build all configs.
Added cli and workspace unit tests