Repository: skypilot-org/skypilot
Pull request: Centralize sky_logs, polish logging, and update `sky start` documentation
Closes #406.

Problem statement:
`sky_logs/` should be put at a central place, not under current working directory
Central place can be, for example, `~/sky_logs/<unique across user/run>` or `/tmp/sky_logs/<unique across user/run>`.  This avoids polluting many directories.

+1: @romilbhardwaj, me

systems that use this strategy: Ray, Spark, ...

Hints from issue discussion:
Another example where this is a blocker is when I run sky cpunode/gpunode in a path where I don't have permissions (e.g. `/`) - sky fails because it's unable to create `sky_logs` dir.
+1. We could put this under `~/.sky/logs` or `~/sky_logs` (like Slurm) and make `sky logs` the main entry point for accessing all sky-related logs (like `sx` in Slurm).
+1 for `~/.sky/logs`. It is a consistent place to put Sky metadata in. (i.e. does ray stores any other results outside of `/tmp/ray_results`, i think it is all in one place)
Should not be under dot as logs should be discoverable and are not
dotfiles.

On Tue, Feb 22, 2022 at 11:40 Michael Luo ***@***.***> wrote:

> +1 for ~/.sky/logs. It is a consistent place to put Sky metadata in.
> (i.e. does ray stores any other results outside of /tmp/ray_results, i
> think it is all in one place)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/sky-proj/sky/issues/406#issuecomment-1048146912>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAEQWHUOUWN5ZIWQ6K2L3TDU4PRBXANCNFSM5PAJEHWQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>

+1 from @dhroth offline
`~/.sky_logs` can be annoying for some users. They want their home directory to be clean. Maybe `/tmp/sky_logs`

Commit message: fix verb

Patch to review:
diff --git a/docs/source/reference/interactive-nodes.rst b/docs/source/reference/interactive-nodes.rst
index 78d7ca130ee..3d80856c412 100644
--- a/docs/source/reference/interactive-nodes.rst
+++ b/docs/source/reference/interactive-nodes.rst
@@ -58,6 +58,12 @@ Interactive nodes can be started and stopped like any other cluster:
     # restart the cluster
     $ sky start my-cpu
 
+.. note::
+
+    Since :code:`sky start` is used to restart a stopped cluster, auto-failover provisioning
+    is not used and the cluster will be started on the same cloud and region that it was
+    originally provisioned on.
+
 
 Advanced configuration
 ----------------------
diff --git a/examples/job_queue/cluster.yaml b/examples/job_queue/cluster.yaml
index 6b5b352d7aa..fbb097f2275 100644
--- a/examples/job_queue/cluster.yaml
+++ b/examples/job_queue/cluster.yaml
@@ -7,5 +7,5 @@
 #   sky exec jq job.yaml
 
 resources:
-  cloud: gcp
+  cloud: aws
   accelerators: K80
diff --git a/sky/backends/cloud_vm_ray_backend.py b/sky/backends/cloud_vm_ray_backend.py
index 552bb1d2eb1..1c641a7de87 100644
--- a/sky/backends/cloud_vm_ray_backend.py
+++ b/sky/backends/cloud_vm_ray_backend.py
@@ -43,7 +43,6 @@
 SKY_REMOTE_APP_DIR = backend_utils.SKY_REMOTE_APP_DIR
 SKY_REMOTE_WORKDIR = backend_utils.SKY_REMOTE_WORKDIR
 SKY_LOGS_DIRECTORY = job_lib.SKY_LOGS_DIRECTORY
-SKY_REMOTE_LOGS_ROOT = job_lib.SKY_REMOTE_LOGS_ROOT
 SKY_REMOTE_RAY_VERSION = backend_utils.SKY_REMOTE_RAY_VERSION
 SKYLET_REMOTE_PATH = backend_utils.SKY_REMOTE_PATH
 
@@ -380,7 +379,7 @@ def __init__(self, log_dir: str, dag: Dag, optimize_target: OptimizeTarget):
         self._blocked_zones = set()
         self._blocked_launchable_resources = set()
 
-        self.log_dir = log_dir
+        self.log_dir = os.path.expanduser(log_dir)
         self._dag = dag
         self._optimize_target = optimize_target
 
@@ -679,8 +678,6 @@ def _retry_region_zones(self, to_provision: Resources, num_nodes: int,
                 continue
             zone_str = ','.join(
                 z.name for z in zones) if zones is not None else 'all zones'
-            logger.info(f'\n{style.BRIGHT}Launching on {to_provision.cloud} '
-                        f'{region.name}{style.RESET_ALL} ({zone_str})')
             config_dict = backend_utils.write_cluster_config(
                 to_provision,
                 num_nodes,
@@ -712,10 +709,14 @@ def _retry_region_zones(self, to_provision: Resources, num_nodes: int,
             global_user_state.add_or_update_cluster(cluster_name,
                                                     cluster_handle=handle,
                                                     ready=False)
-
+            logging_info = {
+                'cluster_name': cluster_name,
+                'region_name': region.name,
+                'zone_str': zone_str,
+            }
             gang_failed, rc, stdout, stderr = self._gang_schedule_ray_up(
                 to_provision.cloud, num_nodes, cluster_config_file,
-                log_abs_path, stream_logs)
+                log_abs_path, stream_logs, logging_info)
 
             if gang_failed or rc != 0:
                 if gang_failed:
@@ -761,7 +762,8 @@ def _retry_region_zones(self, to_provision: Resources, num_nodes: int,
 
     def _gang_schedule_ray_up(self, to_provision_cloud: clouds.Cloud,
                               num_nodes: int, cluster_config_file: str,
-                              log_abs_path: str, stream_logs: bool):
+                              log_abs_path: str, stream_logs: bool,
+                              logging_info: dict):
         """Provisions a cluster via 'ray up' with gang scheduling.
 
         Benefits:
@@ -812,11 +814,20 @@ def ray_up(start_streaming_at):
             public_key_path = config['auth']['ssh_public_key']
             file_mounts[public_key_path] = public_key_path
 
-        with console.status('[bold cyan]Starting cluster...'):
+        region_name = logging_info['region_name']
+        zone_str = logging_info['zone_str']
+
+        # Don't show the zone_str if restarting.
+        with console.status(f'[bold cyan]Launching on {to_provision_cloud}'
+                            f' {region_name}[/] '
+                            f'({zone_str})'):
             # ray up.
             returncode, stdout, stderr = ray_up(
                 start_streaming_at='Shared connection to')
 
+        logger.info(f'{colorama.Fore.CYAN}Launching on {to_provision_cloud} '
+                    f'{region_name}{colorama.Style.RESET_ALL} ({zone_str})')
+
         # Only 1 node or head node provisioning failure.
         if num_nodes == 1 or returncode != 0:
             return False, returncode, stdout, stderr
@@ -1189,7 +1200,8 @@ def sync_to_all_nodes(src: str,
                         command,
                         ssh_user=ssh_user,
                         ssh_private_key=ssh_private_key,
-                        log_path=os.path.join(self.log_dir, 'file_mounts.log'),
+                        log_path=os.path.join(os.path.expanduser(self.log_dir),
+                                              'file_mounts.log'),
                         ssh_control_name=self._ssh_control_name(handle))
                     backend_utils.handle_returncode(
                         returncode, command, f'Failed to sync {src} to {dst}.')
@@ -1344,8 +1356,8 @@ def sync_down_logs(self, handle: ResourceHandle, job_id: int) -> None:
                                         'Failed to sync logs.', stderr)
         log_dir = log_dir.strip()
 
-        local_log_dir = log_dir
-        remote_log_dir = os.path.join(SKY_REMOTE_LOGS_ROOT, log_dir)
+        local_log_dir = os.path.expanduser(log_dir)
+        remote_log_dir = log_dir
 
         style = colorama.Style
         fore = colorama.Fore
@@ -1406,9 +1418,9 @@ def _exec_code_on_head(
                            source=fp.name,
                            target=script_path,
                            with_outputs=False)
-
-        job_log_path = os.path.join(self.log_dir, 'job_submit.log')
-        remote_log_dir = os.path.join(SKY_REMOTE_LOGS_ROOT, self.log_dir)
+        remote_log_dir = self.log_dir
+        local_log_dir = os.path.expanduser(remote_log_dir)
+        job_log_path = os.path.join(local_log_dir, 'job_submit.log')
         remote_log_path = os.path.join(remote_log_dir, 'run.log')
 
         assert executable == 'python3', executable
@@ -1525,8 +1537,7 @@ def execute(
     def _execute_task_one_node(self, handle: ResourceHandle, task: Task,
                                job_id: int, detach_run: bool) -> None:
         # Launch the command as a Ray task.
-        log_dir = os.path.join(f'{SKY_REMOTE_LOGS_ROOT}', f'{self.log_dir}',
-                               'tasks')
+        log_dir = os.path.join(self.log_dir, 'tasks')
         log_path = os.path.join(log_dir, 'run.log')
 
         accelerator_dict = _get_task_demands_dict(task)
@@ -1560,8 +1571,7 @@ def _execute_task_n_nodes(self, handle: ResourceHandle, task: Task,
         #   ray.init(...)
         #   for node:
         #     submit _run_cmd(cmd) with resource {node_i: 1}
-        log_dir_base = os.path.join(f'{SKY_REMOTE_LOGS_ROOT}',
-                                    f'{self.log_dir}')
+        log_dir_base = self.log_dir
         log_dir = os.path.join(log_dir_base, 'tasks')
         accelerator_dict = _get_task_demands_dict(task)
 
@@ -1638,7 +1648,8 @@ def teardown_ephemeral_storage(self, task: Task) -> None:
                     storage.delete()
 
     def teardown(self, handle: ResourceHandle, terminate: bool) -> None:
-        log_path = os.path.join(self.log_dir, 'teardown.log')
+        log_path = os.path.join(os.path.expanduser(self.log_dir),
+                                'teardown.log')
         log_abs_path = os.path.abspath(log_path)
         cloud = handle.launched_resources.cloud
         config = backend_utils.read_yaml(handle.cluster_yaml)
diff --git a/sky/cli.py b/sky/cli.py
index 946f6d2dc2a..1a01c5b093c 100644
--- a/sky/cli.py
+++ b/sky/cli.py
@@ -36,7 +36,6 @@
 import yaml
 
 import click
-import colorama
 import pendulum
 from rich import console as rich_console
 
@@ -935,6 +934,10 @@ def start(clusters: Tuple[str]):
     start the cluster.  (In the second case, any failed setup steps are not
     performed and only a request to start the machines is attempted.)
 
+    Note that auto-failover provisioning is not used when restarting stopped
+    clusters. They will be started on the same cloud and region that was chosen
+    before.
+
     If a cluster is already in an UP status, this command has no effect on it.
 
     Examples:
@@ -1015,10 +1018,6 @@ def _filter(name, all_clusters):
         with sky.Dag():
             dummy_task = sky.Task().set_resources(handle.launched_resources)
             dummy_task.num_nodes = handle.launched_nodes
-        click.secho(
-            f'{colorama.Fore.CYAN}Starting cluster '
-            f'{colorama.Fore.GREEN}{name}...',
-            bold=True)
         backend.provision(dummy_task,
                           to_provision=handle.launched_resources,
                           dryrun=False,
diff --git a/sky/skylet/job_lib.py b/sky/skylet/job_lib.py
index 902ba50005d..66af82b1b1c 100644
--- a/sky/skylet/job_lib.py
+++ b/sky/skylet/job_lib.py
@@ -15,8 +15,7 @@
 
 from sky.skylet import util_lib
 
-SKY_LOGS_DIRECTORY = 'sky_logs'
-SKY_REMOTE_LOGS_ROOT = '~'
+SKY_LOGS_DIRECTORY = '~/sky_logs'
 
 
 class JobStatus(enum.Enum):


Code review comments:
diff_hunk: @@ -812,11 +814,20 @@ def ray_up(start_streaming_at):
             public_key_path = config['auth']['ssh_public_key']
             file_mounts[public_key_path] = public_key_path
 
-        with console.status('[bold cyan]Starting cluster...'):
+        region_name = logging_info['region_name']
+        zone_str = logging_info['zone_str']
+
+        # Don't show the zone_str if restarting.
path: sky/backends/cloud_vm_ray_backend.py
text: @user1:
This comment is obsolete.