Repository: skypilot-org/skypilot
Pull request: Fix defunct job after cancelling
We are currently using `subprocess` to run the command line. The child process may be defunct when the python program is killed. Fixed by creating a subprocess to kill the defuncted child process with `try ... finally`. Fixed #233 

Tested:
1. Launch a cluster with `sky launch -c test ./examples/job_queue/cluster.yaml`
2. Run a GPU program `sky exec -c test ./examples/job_queue/job_gpu.yaml`
3. Check the gpustat `sky exec -c test gpustat`
3. Cancel the job `sky cancel -c test 1`
4. Check the gpustat again `sky exec -c test gpustat -cpu -i 1`

Before the fix, the gpu will remain occupied after canceling. After the fix, the problem is solved.

Problem statement:
Failed to kill a sky job with sky cancel
I failed to cancel my job with `sky cancel`. I had to manually ssh in and kill it.
@Michaelvll have you met this issue before?
```
(sky) weichiang@blaze:~/repos/bert-sign/sky-experiments/prototype/examples$ sky cancel -c sky-2920-weichiang 9
Cancelling jobs (9) on cluster sky-2920-weichiang...
(sky) weichiang@blaze:~/repos/bert-sign/sky-experiments/prototype/examples$ sky queue sky-2920-weichiang
Fetching and parsing job queue...

Sky Job Queue of Cluster sky-2920-weichiang
ID  NAME       USER       SUBMITTED   STATUS     LOG
9   bert_sign  weichiang  9 mins ago  CANCELLED  sky_logs/sky-2022-01-22-00-35-33-025250
8   bert_sign  weichiang  4 hrs ago   FAILED     sky_logs/sky-2022-01-21-20-18-28-505572
7   bert_sign  weichiang  8 hrs ago   FAILED     sky_logs/sky-2022-01-21-16-21-57-228872
6   <cmd>      weichiang  8 hrs ago   FAILED     sky_logs/sky-2022-01-21-16-01-15-015994
5   bert_sign  weichiang  8 hrs ago   FAILED     sky_logs/sky-2022-01-21-15-51-10-260086
4   bert_sign  weichiang  9 hrs ago   FAILED     sky_logs/sky-2022-01-21-15-42-01-661227
3   bert_sign  weichiang  9 hrs ago   FAILED     sky_logs/sky-2022-01-21-15-40-42-390030
2   bert_sign  weichiang  9 hrs ago   FAILED     sky_logs/sky-2022-01-21-15-37-32-004498
1   bert_sign  weichiang  9 hrs ago   FAILED     sky_logs/sky-2022-01-21-15-32-41-239102
```
On the VM. the job on GPU-1 (submitted by Sky) is still running.
```
(flax) ubuntu@ip-172-31-14-71:~/tensorflow_datasets/lm1b$ gpustat
ip-172-31-14-71          Sat Jan 22 08:51:30 2022  450.142.00
[0] Tesla V100-SXM2-16GB | 49°C,   3 % | 15180 / 16160 MB | ubuntu(15177M)
[1] Tesla V100-SXM2-16GB | 63°C, 100 % | 16022 / 16160 MB | ubuntu(16011M)
[2] Tesla V100-SXM2-16GB | 39°C,   0 % |     0 / 16160 MB |
[3] Tesla V100-SXM2-16GB | 42°C,   0 % |     0 / 16160 MB |
[4] Tesla V100-SXM2-16GB | 41°C,   0 % |     0 / 16160 MB |
[5] Tesla V100-SXM2-16GB | 42°C,   0 % |     0 / 16160 MB |
[6] Tesla V100-SXM2-16GB | 40°C,   0 % |     0 / 16160 MB |
[7] Tesla V100-SXM2-16GB | 41°C,   0 % |     0 / 16160 MB |
```
```
106906 ubuntu     20   0  4648   836   760 S  0.0  0.0  0:00.00 ├─ /bin/sh -c /bin/bash /tmp/sky_app_kabqgmyi
106913 ubuntu     20   0 13444  3532  3104 S  0.0  0.0  0:00.00 │  └─ /bin/bash /tmp/sky_app_kabqgmyi                                                                                                              
106935 ubuntu     20   0 66.4G 40.6G 5192M S  0.0  8.4 13:10.59 │     └─ python main.py --configs configs/products/bert-finetune/sbert-large-tmp2-mixup-deep.yml --data_root /home/ubuntu/dataset/
```

Hints from issue discussion:
Yeah, that happens to me sometimes. Is it possible to create a small reproduction code? I suspect it is caused by the ray's stale actors.

Commit message: Change comment

Patch to review:
diff --git a/prototype/examples/job_queue/job_gpu.yaml b/prototype/examples/job_queue/job_gpu.yaml
new file mode 100644
index 00000000000..985847b68cd
--- /dev/null
+++ b/prototype/examples/job_queue/job_gpu.yaml
@@ -0,0 +1,28 @@
+# A task submitted to an existing cluster.
+#
+# Runs a task on a existing cluster.
+#
+# Usage:
+#   sky launch -c jq cluster.yaml
+#   sky exec -c jq job.yaml
+
+name: job
+
+resources:
+  accelerators:
+    K80: 0.5
+
+# setup: |
+#   conda create -n test python=3.7 -y
+#   conda activate test
+#   conda install pytorch torchvision torchaudio cudatoolkit=10.2 -c pytorch
+
+
+
+run: |
+  timestamp=$(date +%s)
+  conda activate test
+  echo "started"
+  python -u -c "import torch; a = torch.randn(10000, 10000).cuda(); b = torch.randn(10000, 10000).cuda(); [print((a @ b).sum()) for _ in range(10000000000)]"
+  echo "ended"
+
diff --git a/prototype/sky/skylet/log_lib.py b/prototype/sky/skylet/log_lib.py
index 1ec60227673..2de096b5761 100644
--- a/prototype/sky/skylet/log_lib.py
+++ b/prototype/sky/skylet/log_lib.py
@@ -76,18 +76,35 @@ def run_with_log(
     Retruns the process, stdout and stderr of the command.
       Note that the stdout and stderr is already decoded.
     """
-    with subprocess.Popen(cmd,
-                          stdout=subprocess.PIPE,
-                          stderr=subprocess.PIPE,
-                          **kwargs) as proc:
-        stdout, stderr = redirect_process_output(
-            proc, log_path, stream_logs, start_streaming_at=start_streaming_at)
-        proc.wait()
-        if proc.returncode != 0 and check:
-            raise RuntimeError('Command failed, please check the logs.')
-        if return_none:
-            return None
-        return proc, stdout, stderr
+    try:
+        with subprocess.Popen(cmd,
+                              stdout=subprocess.PIPE,
+                              stderr=subprocess.PIPE,
+                              start_new_session=True,
+                              **kwargs) as proc:
+            proc_pid = os.getpgid(proc.pid)
+            stdout, stderr = redirect_process_output(
+                proc,
+                log_path,
+                stream_logs,
+                start_streaming_at=start_streaming_at)
+            proc.wait()
+            if proc.returncode != 0 and check:
+                raise RuntimeError('Command failed, please check the logs.')
+            if return_none:
+                return None
+            return proc, stdout, stderr
+    finally:
+        # The proc can be defunct if the python program is killed. Here we
+        # open a new subprocess to kill the process, SIGKILL the process group.
+        # Adapted from ray/dashboard/modules/job/job_manager.py#L154
+        subprocess.Popen(
+            f'kill -9 -{proc_pid}',
+            shell=True,
+            # Suppress output
+            stdout=subprocess.DEVNULL,
+            stderr=subprocess.DEVNULL,
+        )
 
 
 def run_bash_command_with_log(bash_command: str,


Code review comments:
diff_hunk: @@ -76,18 +76,35 @@ def run_with_log(
     Retruns the process, stdout and stderr of the command.
       Note that the stdout and stderr is already decoded.
     """
-    with subprocess.Popen(cmd,
-                          stdout=subprocess.PIPE,
-                          stderr=subprocess.PIPE,
-                          **kwargs) as proc:
-        stdout, stderr = redirect_process_output(
-            proc, log_path, stream_logs, start_streaming_at=start_streaming_at)
-        proc.wait()
-        if proc.returncode != 0 and check:
-            raise RuntimeError('Command failed, please check the logs.')
-        if return_none:
-            return None
-        return proc, stdout, stderr
+    try:
+        with subprocess.Popen(cmd,
+                              stdout=subprocess.PIPE,
+                              stderr=subprocess.PIPE,
+                              start_new_session=True,
+                              **kwargs) as proc:
+            proc_pid = os.getpgid(proc.pid)
path: prototype/sky/skylet/log_lib.py
text: @user1:
nit: pid = process id, here I think you mean `proc_pgid`?