SkyBenchmark: fix job_status is None for failed candidates.

[concretevitamin]

Attempt to fix #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 [Benchmark] TypeError: '<' not supported between instances of 'NoneType' and 'JobStatus' #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    -       -         -       -        -

Tested (run the relevant ones):

• Code formatting: bash format.sh
• Any manual or new tests for this PR (please specify below)
• All smoke tests: pytest tests/test_smoke.py
  • 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

[Michaelvll]

Thanks for fixing this @concretevitamin! Left a question.

[Michaelvll]

Suggested change

	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):
	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.

[concretevitamin]

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?

[Michaelvll]

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.

[concretevitamin]

Updated logic and PR description, PTAL.

    #   cluster_status None (e.g., preempted/never launched), job_status None
    #     --> BenchmarkStatus TERMINATED
    #   cluster_status INIT (e.g., something's wrong, or still launching), job_status None
    #     --> BenchmarkStatus INIT
    #   cluster_status STOPPED (e.g., auto-stopped), job_status None
    #     --> _determine_finished_or_terminated()
    #   cluster_status UP, job_status None
    #     --> BenchmarkStatus.INIT
    #   cluster_status UP, job_status not-None
    #     --> if job_status < RUNNING: BenchmarkStatus.INIT
    #     --> if job_status == RUNNING: BenchmarkStatus.RUNNING
    #     --> if job_status.is_terminal(): _determine_finished_or_terminated()

For the first case, it's ok to send it to the sink state TERMINATED, since there's no way to "rerun" the same benchmark name to retry the launch. Wdyt?

[Michaelvll]

Thanks for the fix @concretevitamin! LGTM.

[Michaelvll]

This seems will never happen as we have checked end_time in L348?

[concretevitamin]

Good catch! Simplified the logic now.

[concretevitamin]

Simplified the logic + added a basic smoke test. PTAL.

[concretevitamin]

Good catch! Simplified the logic now.