Repository: skypilot-org/skypilot
Pull request: SkyBenchmark: fix job_status is None for failed candidates.
<!-- Describe the changes in this PR -->

Attempt to fix https://github.com/skypilot-org/skypilot/issues/2765.

Repro:
```
» cat test.yaml
run: |
  nvidia-smi

resources:
  cloud: gcp
  region: us-central1
  zone: us-central1-f

sky bench launch test.yaml --gpus A100:8,T4 --benchmark test1
```

Before: failed to parse `--gpus`; after fixing parsing and waiting for bench finished launching (A100:8)
- `sky bench show mybench` reproduces the error in #2765


With this PR: no error occurs
```
» sky bench  show test1
Legend:
- #STEPS: Number of steps taken.
- SEC/STEP, $/STEP: Average time (cost) per step.
- EST(hr), EST($): Estimated total time (cost) to complete the benchmark.

CLUSTER            RESOURCES                        STATUS    DURATION  SPENT($)  #STEPS  SEC/STEP  $/STEP  EST(hr)  EST($)
sky-bench-test1-1  1x GCP(n1-highmem-4, {'T4': 1})  FINISHED  < 1s      0.0001    -       -         -       -        -
```

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [x] Code formatting: `bash format.sh`
- [ ] Any manual or new tests for this PR (please specify below)
- [ ] All smoke tests: `pytest tests/test_smoke.py` 
  - [x] `pytest tests/test_smoke.py::test_sky_bench --generic-cloud gcp`: passed on this PR; failed on master
- [ ] Relevant individual smoke tests: `pytest tests/test_smoke.py::test_fill_in_the_name` 
- [ ] Backward compatibility tests: `bash tests/backward_comaptibility_tests.sh`


Problem statement:
[Benchmark] TypeError: '<' not supported between instances of 'NoneType' and 'JobStatus'
I created a benchmark that included a candidate `- {accelerators: A100:8}`.

```bash
sky bench launch benchmark.yaml --benchmark hydra-3eps
```

At the time, no nodes were available in GCP that could satisfy that candidate, so it wasn't provisioned.

Unfortunately, when I went to check the results of the benchmark, I received the following error:

```bash
$ sky bench show hydra-3eps                               
Traceback (most recent call last):
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/bin/sky", line 8, in <module>
    sys.exit(cli())
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/utils/common_utils.py", line 315, in _record
    return f(*args, **kwargs)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/cli.py", line 1162, in invoke
    return super().invoke(ctx)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/utils/common_utils.py", line 315, in _record
    return f(*args, **kwargs)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/cli.py", line 1162, in invoke
    return super().invoke(ctx)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 1657, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/utils/common_utils.py", line 336, in _record
    return f(*args, **kwargs)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/cli.py", line 4368, in benchmark_show
    benchmark_utils.update_benchmark_state(benchmark)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/benchmark/benchmark_utils.py", line 595, in update_benchmark_state
    _parallel_run_with_interrupt_handling(_update_with_progress_bar,
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/benchmark/benchmark_utils.py", line 202, in _parallel_run_with_interrupt_handling
    return list(p.imap(func, args))
  File "/opt/homebrew/Cellar/python@3.9/3.9.17_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/pool.py", line 870, in next
    raise value
  File "/opt/homebrew/Cellar/python@3.9/3.9.17_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/benchmark/benchmark_utils.py", line 585, in _update_with_progress_bar
    message = _update_benchmark_result(arg)
  File "/Users/csaroff/code/bioticsai/ultrasound_classifier/venv/lib/python3.9/site-packages/sky/benchmark/benchmark_utils.py", line 325, in _update_benchmark_result
    job_status < job_lib.JobStatus.RUNNING):
TypeError: '<' not supported between instances of 'NoneType' and 'JobStatus'
```

Hints from issue discussion:
I'm not sure if it's related to the failed candidate, but it also appears that all of the benchmark clusters were kept running throughout the entire benchmark(even though some should have definitely completed sooner). They all have a duration of exactly `2h 2m 2s`
Thanks for the report @csaroff. Could you help test if #2767 fixes this TypeError?

Commit message: Fix --gpus parsing

Patch to review:
diff --git a/sky/benchmark/benchmark_utils.py b/sky/benchmark/benchmark_utils.py
index 7db60783747..80cd3692398 100644
--- a/sky/benchmark/benchmark_utils.py
+++ b/sky/benchmark/benchmark_utils.py
@@ -321,8 +321,10 @@ def _update_benchmark_result(benchmark_result: Dict[str, Any]) -> Optional[str]:
                                                 stream_logs=False)[1]
 
     # Update the benchmark status.
-    if (cluster_status == status_lib.ClusterStatus.INIT or
-            job_status < job_lib.JobStatus.RUNNING):
+    if job_status is None:
+        benchmark_status = benchmark_state.BenchmarkStatus.TERMINATED
+    elif (cluster_status == status_lib.ClusterStatus.INIT or
+          job_status < job_lib.JobStatus.RUNNING):
         benchmark_status = benchmark_state.BenchmarkStatus.INIT
     elif job_status == job_lib.JobStatus.RUNNING:
         benchmark_status = benchmark_state.BenchmarkStatus.RUNNING
diff --git a/sky/cli.py b/sky/cli.py
index 33bfc27b232..f1ea5840143 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -4209,13 +4209,13 @@ def benchmark_launch(
     if gpus is not None:
         gpu_list = gpus.split(',')
         gpu_list = [gpu.strip() for gpu in gpu_list]
-        if '' in gpus:
+        if ' ' in gpus:
             raise click.BadParameter('Remove blanks in --gpus.')
 
         if len(gpu_list) == 1:
             override_gpu = gpu_list[0]
         else:
-            # If len(gpus) > 1, gpus is intrepreted
+            # If len(gpu_list) > 1, gpus is interpreted
             # as a list of benchmark candidates.
             if candidates is None:
                 candidates = [{'accelerators': gpu} for gpu in gpu_list]


Code review comments:
diff_hunk: @@ -321,8 +321,10 @@ def _update_benchmark_result(benchmark_result: Dict[str, Any]) -> Optional[str]:
                                                 stream_logs=False)[1]
 
     # Update the benchmark status.
-    if (cluster_status == status_lib.ClusterStatus.INIT or
-            job_status < job_lib.JobStatus.RUNNING):
+    if job_status is None:
+        benchmark_status = benchmark_state.BenchmarkStatus.TERMINATED
+    elif (cluster_status == status_lib.ClusterStatus.INIT or
+          job_status < job_lib.JobStatus.RUNNING):
path: sky/benchmark/benchmark_utils.py
text: @user1:
```suggestion
    if (cluster_status == status_lib.ClusterStatus.INIT or job_status is None or
          job_status < job_lib.JobStatus.RUNNING):
```

Reason: when the cluster is being provisioned, the `job_status` will be None and the benchmark task should be initializing.

@author:
There seems to be a few cases
```
    # 'record' is not None:
    #   cluster_status None (e.g., preempted/never launched), job_status None
    #     --> BenchmarkStatus.TERMINATED or INIT
    #   cluster_status INIT (e.g., something's wrong), job_status None
    #     --> BenchmarkStatus ??
    #   cluster_status STOPPED (e.g., manually stopped or auto-stopped), job_status None
    #     --> BenchmarkStatus ??
    #   cluster_status UP (e.g., manually stopped or auto-stopped), job_status None
    #     --> BenchmarkStatus.INIT
    #   cluster_status UP (e.g., manually stopped or auto-stopped), job_status not-None
    #     --> handled below
```
The problem is BenchmarkStatus's definition doesn't seem too clear (e.g., for the first case, should we set it to TERMINATED or INIT; does it matter?).  Wdyt?

@user1:
IMO, it would be better to set the BenchmarkStatus to INIT for all the cases mentinoed, as it aligns with the semantic of `INIT`, i.e., the status is UNKNOWN or abnormal or initializing. I would not set it to TERMINATED as it may cause a transition from TERMINATED to a non-terminated state in the case `cluster_status is None` or `cluster_status is UP` while `job_status is None`, which can be quite suprising. The TERMINATED state should be a sink state. 
Maybe we can specially handle the case where the `cluster_status` is STOPPED, but I think it should be fine to set all of the to BenchmarkStatus.INIT.