Repository: skypilot-org/skypilot
Pull request: [UX] Make `sky logs <nonexistent job id>` not error out.
<!-- Describe the changes in this PR -->

Fixes #1607.

<!-- 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] Any manual or new tests for this PR (please specify below)
```
» sky logs sky-spot-controller-8a3968f2  1099
Tailing logs of job 1099 on cluster 'sky-spot-controller-8a3968f2'...
Job 1099 not found (see `sky queue`).
Shared connection to 34.27.229.46 closed.
```

Problem statement:
[UX] sky logs throws unnecessary errors when job id doesn't exist.
`sky logs` throws errors when the job id doesn't exist. We should catch the error instead and show `Job not found` or things like that.
```
(sky-tmp) weichiang@mbp sky % sky logs cpu 1
Tailing logs of job 1 on cluster 'cpu'...
Warning: Permanently added '35.202.33.57' (ED25519) to the list of known hosts.
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/opt/conda/lib/python3.7/posixpath.py", line 94, in join
    genericpath._check_arg_types('join', a, *p)
  File "/opt/conda/lib/python3.7/genericpath.py", line 153, in _check_arg_types
    (funcname, s.__class__.__name__)) from None
TypeError: join() argument must be str or bytes, not 'NoneType'
Shared connection to 35.202.33.57 closed.
```

Hints from issue discussion:

Commit message: Make `sky logs <nonexistent job id>` not error out.

Patch to review:
diff --git a/sky/skylet/job_lib.py b/sky/skylet/job_lib.py
index 62909440621..17aa608fb54 100644
--- a/sky/skylet/job_lib.py
+++ b/sky/skylet/job_lib.py
@@ -362,17 +362,14 @@ def _get_jobs_by_ids(job_ids: List[int]) -> List[Dict[str, Any]]:
 def update_job_status(job_owner: str,
                       job_ids: List[int],
                       silent: bool = False) -> List[JobStatus]:
-    """Updates and returns the job statuses matching our `JobStatus` semantics
+    """Updates and returns the job statuses matching our `JobStatus` semantics.
 
-    "True" statuses: this function queries `ray job status` and processes
-    those results to match our semantics.
+    This function queries `ray job status` and processes those results to match
+    our semantics.
 
-    This function queries `ray job status` and processes those results to
-    match our semantics.
-
-    Though we update job status actively in ray program and job cancelling,
-    we still need this to handle staleness problem, caused by instance
-    restarting and other corner cases (if any).
+    Though we update job status actively in the generated ray program and
+    during job cancelling, we still need this to handle the staleness problem,
+    caused by instance restarting and other corner cases (if any).
 
     This function should only be run on the remote instance with ray==2.0.1.
     """
@@ -413,7 +410,8 @@ def update_job_status(job_owner: str,
             if status is None:
                 original_status = get_status_no_lock(job_id)
                 status = original_status
-                if not original_status.is_terminal():
+                if (original_status is not None and
+                        not original_status.is_terminal()):
                     # The job may be stale, when the instance is restarted
                     # (the ray redis is volatile). We need to reset the
                     # status of the task to FAILED if its original status
@@ -670,9 +668,10 @@ def tail_logs(cls,
             'else job_lib.get_latest_job_id()',
             'run_timestamp = job_lib.get_run_timestamp(job_id)',
             (f'log_dir = os.path.join({constants.SKY_LOGS_DIRECTORY!r}, '
-             'run_timestamp)'),
+             '"" if run_timestamp is None else run_timestamp)'),
             (f'log_lib.tail_logs({job_owner!r},'
-             f'job_id, log_dir, {spot_job_id!r}, follow={follow})'),
+             f'job_id, None if run_timestamp is None else log_dir, '
+             f'{spot_job_id!r}, follow={follow})'),
         ]
         return cls._build(code)
 
diff --git a/sky/skylet/log_lib.py b/sky/skylet/log_lib.py
index 6ab16dcf0c3..afc8220930b 100644
--- a/sky/skylet/log_lib.py
+++ b/sky/skylet/log_lib.py
@@ -383,14 +383,14 @@ def tail_logs(job_owner: str,
     job_str = f'job {job_id}'
     if spot_job_id is not None:
         job_str = f'spot job {spot_job_id}'
-    logger.debug(f'Tailing logs for job, real job_id {job_id}, spot_job_id '
-                 f'{spot_job_id}.')
-    logger.info(f'{colorama.Fore.YELLOW}Start streaming logs for {job_str}.'
-                f'{colorama.Style.RESET_ALL}')
     if log_dir is None:
         print(f'{job_str.capitalize()} not found (see `sky queue`).',
               file=sys.stderr)
         return
+    logger.debug(f'Tailing logs for job, real job_id {job_id}, spot_job_id '
+                 f'{spot_job_id}.')
+    logger.info(f'{colorama.Fore.YELLOW}Start streaming logs for {job_str}.'
+                f'{colorama.Style.RESET_ALL}')
     log_path = os.path.join(log_dir, 'run.log')
     log_path = os.path.expanduser(log_path)
 


Code review comments:
diff_hunk: @@ -670,9 +668,10 @@ def tail_logs(cls,
             'else job_lib.get_latest_job_id()',
             'run_timestamp = job_lib.get_run_timestamp(job_id)',
             (f'log_dir = os.path.join({constants.SKY_LOGS_DIRECTORY!r}, '
-             'run_timestamp)'),
+             '"" if run_timestamp is None else run_timestamp)'),
             (f'log_lib.tail_logs({job_owner!r},'
-             f'job_id, log_dir, {spot_job_id!r}, follow={follow})'),
+             f'job_id, None if run_timestamp is None else log_dir, '
+             f'{spot_job_id!r}, follow={follow})'),
path: sky/skylet/job_lib.py
text: @user1:
nit: this logic looks good to me, but it seems the inline commands can cause readability issues. Should we consider changing the code generation to generate the multi-line python commands with https://www.gnu.org/savannah-checkouts/gnu/bash/manual/bash.html#Here-Documents?
```
python - <<EOF
import os
from sky.skylet import job_lib, log_lib

# generated code for the functionalities
...
EOF
```
Reference: https://stackoverflow.com/questions/2043453/executing-multi-line-statements-in-the-one-line-command-line

@user1:
Another way would be wrap the logics in a function and directly call the function in the generated code, instead of having the logic written as strings.

@author:
Thanks. 

1. Did not get EOF to work. Many variations of 
```
        return f"""python3 - <<EOF\n{code}\nEOF"""
```
with `code` being `\n`-joined returns
```
bash: warning: here-document at line 0 delimited by end-of-file (wanted `EOF')
bash: syntax error: unexpected end of file
logout
```

2. We could do this, but I slightly worry about the skylet on an existing cluster being outdated and users failed to run `sky logs` after upgrading. 

@author:
How does it look now? Same approach, reformatted to be more readable.

@user1:
It looks much better to me! Thanks for reformating it!