[deps] raydepsets import all config (1/3) #57581
Conversation
Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
![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.
![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 is a good step in refactoring the dependency set management. The changes to include config_name in Depset and remove build_arg_sets from the Config class are logical and well-implemented. I've found one area for improvement in the test code for better efficiency and readability. Overall, the changes are clean and the tests are updated accordingly.
ci/raydepsets/workspace.py
Outdated
| config_name = os.path.basename(config_path) | ||
| config = Config.from_dict(data, config_name) |
There was a problem hiding this comment.
nit: could you move these two lines out of the with block (by deindenting it), so that the file can be closed earlier?
|
|
||
| @classmethod | ||
| def from_dict(cls, data: dict) -> "Config": | ||
| def from_dict(cls, data: dict, config_name: str) -> "Config": |
There was a problem hiding this comment.
could you explain to me again why build_arg_sets are removed now?
There was a problem hiding this comment.
its not necessary once the depsets have been expanded per build arg set
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Elliot Barnwell <elliot.barnwell@anyscale.com>
… of github.com:ray-project/ray into elliot-barn/raydepsets-including-config-name-in-depset
2 participants
Including config_name in depsets
Remove build_arg_sets from config class