[runtime env] Add garbage collection for conda envs #20072
Conversation
|
@edoakes I'm working on getting tests to pass, but it would be good to get a review of the overall approach. Once this is working, we can get started on unifying all the "Managers" into the plugin API. |
| @pytest.fixture(scope="function", params=["ray_client", "no_ray_client"]) | ||
| def start_cluster(ray_start_cluster, request): |
There was a problem hiding this comment.
should we make this shared between the tests somehow?
|
@edoakes Tests are passing locally! |
| conda_dir, f"requirements-{pip_hash_str}.txt") | ||
| resources_dir, f"requirements-{pip_hash_str}.txt") |
There was a problem hiding this comment.
do we clean this file up btw?
There was a problem hiding this comment.
Currently no, but we should
| create_conda_env( | ||
| conda_yaml_file, prefix=conda_env_name, logger=logger) | ||
| self._created_envs.add(conda_env_name) | ||
| os.remove(conda_yaml_file) |
There was a problem hiding this comment.
in general we should have these cleanups in a finally block so they run even if the creation fails
There was a problem hiding this comment.
I see, makes sense
| delete_cmd = [conda_path, "remove", "-p", prefix, "--all", "-y"] | ||
| exit_code, output = exec_cmd_stream_to_logger(delete_cmd, logger) |
There was a problem hiding this comment.
out of curiosity, could we also just do rm -f the directory? or this there some special cleanup that this does?
There was a problem hiding this comment.
I took a look at the source https://github.com/conda/conda/blob/master/conda/cli/main_remove.py and yeah, it looks like the main ingredient is rm rf. It looks like it edits some metadata too though, but not sure how important that is
Why are these changes needed?
Adds garbage collection for conda envs in the following two cases:
In a followup PR we will add GC for per-task and per-actor runtime envs. The issue tracking this is here: #19602
Related issue number
Closes #19958
Checks
scripts/format.shto lint the changes in this PR.