Improve UX for logs to include SSH name and rank

[iojw]

As discussed in #1369, this updates the debug logs to make it easier to figure out which node is the head node, which host name to use to ssh into a specific node, and also the rank of the node. With this PR, the debug output becomes the following:

(node-2, head, rank=0, pid=23862) 0
(node-2, head, rank=0, pid=23862) 172.31.95.105 172.31.81.91 172.31.85.131 172.31.89.126
(node-0, worker1, rank=1, pid=22755, ip=172.31.81.91) 1
(node-0, worker1, rank=1, pid=22755, ip=172.31.81.91) 172.31.95.105 172.31.81.91 172.31.85.131 172.31.89.126
(node-1, worker3, rank=3, pid=22760, ip=172.31.89.126) 3
(node-1, worker3, rank=3, pid=22760, ip=172.31.89.126) 172.31.95.105 172.31.81.91 172.31.85.131 172.31.89.126
(node-3, worker2, rank=2, pid=22757, ip=172.31.85.131) 2
(node-3, worker2, rank=2, pid=22757, ip=172.31.85.131) 172.31.95.105 172.31.81.91 172.31.85.131 172.31.89.126

with the following config:

resources:
  cloud: aws
  disk_size: 1024
  instance_type: m6i.large

num_nodes: 4

run: |
  echo $SKY_NODE_RANK
  echo $SKY_NODE_IPS

Thus, if the cluster is named mycluster, the user can easily figure out which host name to use to ssh into a node using the logs e.g. ssh mycluster-worker1 for the log corresponding to worker1.

To ensure stableness for the naming of workers, we first sort them by their external IP, then do a mapping from internal to external IP in the code generation step to determine the node name. This means that the node name is always the same for a cluster regardless of what value is passed via the num_nodes argument.

This also updates the logic for determining the rank of node to use the external IP of a node rather than the internal IP. However, as discussed in #1291, the rank is not always constant for a cluster, and is assigned on a per-task basis, as can be seen from the following output with num_nodes set to 2:

(node-0, worker3, rank=1, pid=24355, ip=172.31.89.126) 1
(node-0, worker3, rank=1, pid=24355, ip=172.31.89.126) 172.31.95.105 172.31.89.126
(node-1, head, rank=0, pid=26036) 0
(node-1, head, rank=0, pid=26036) 172.31.95.105 172.31.89.126

Questions

• Do we still want to respect the task_name variable that is passed in and display it? The above output includes the task name.
• Open to suggestions on how to further improve the UX!

Tested

• Multi-node cluster
• Single-node cluster

[concretevitamin]

This is awesome @iojw! Quick comments:

1. Maybe it's okay to get ride of node-<i> in the prefix now? It doesn't seem to help.
2. RE: "Do we still want to respect the task_name variable that is passed in and display it?" I feel it's okay to not display it in the logging prefix at all, for both multi- and single-node tasks. For the latter, it doesn't seem helpful to display head as well.

[iojw]

@concretevitamin I think one argument for keeping it in is that we currently use it to name the log path for multi-node clusters, and it is possible that users would want to figure out which log file contains the full logs for a node given the debug output. There might not be any better naming scheme for the logs since we do not have access to the rank of a node and its ip from outside the codegen class.

skypilot/sky/backends/cloud_vm_ray_backend.py

Line 3072 in a613b43

log_path = os.path.join(f'{log_dir}', f'{name}.log')

[concretevitamin]

Since log_path is eventually used inside the codegen

skypilot/sky/backends/cloud_vm_ray_backend.py

Line 402 in 3d3da86

log_path,

can we name the path using IP there?

[iojw]

@concretevitamin Ah yup! In that case I don't see any need for the notion of task name at all for multi-node tasks. I've updated the code to remove the notion of task name from the logs. The log files are now named according to the node name e.g. head.log, worker1.log etc.

The logs for multi-node tasks now look like so:

(worker1, rank=1, pid=23368, ip=172.31.87.209) 1
(worker1, rank=1, pid=23368, ip=172.31.87.209) 172.31.81.189 172.31.87.209 172.31.86.128 172.31.87.196
(worker3, rank=3, pid=23414, ip=172.31.87.196) 3
(worker3, rank=3, pid=23414, ip=172.31.87.196) 172.31.81.189 172.31.87.209 172.31.86.128 172.31.87.196
(head, rank=0, pid=26060) 0
(head, rank=0, pid=26060) 172.31.81.189 172.31.87.209 172.31.86.128 172.31.87.196
(worker2, rank=2, pid=23348, ip=172.31.86.128) 2
(worker2, rank=2, pid=23348, ip=172.31.86.128) 172.31.81.189 172.31.87.209 172.31.86.128 172.31.87.196

[iojw]

For single node tasks, the logs display the name of the task, or just task if no name is set.

(task, pid=32503) 0
(task, pid=32503) 172.31.95.145

[concretevitamin]

Thanks @iojw - one thought: how about we don't show the task name even for single-node tasks? This is to keep it consistent. Users can see their custom task names in sky queue.

[iojw]

@concretevitamin Ray does not play well with the name option being empty - if it is, the name in the logs defaults to the name of the remote function run_bash_command_with_logs instead. We can work around with passing in a space " " as the name, but this will lead to 2 spaces at the start of each log which is not ideal. Additionally, something to consider - it is not always the case that the task is executed on the head node for single-node tasks on multi-node clusters, so it may still make sense to include the node name. e.g. A single-node task may still be executed on worker1 or worker2.

What do you think?

[concretevitamin]

These are great points @iojw. How about for single-node tasks we just show head/worker<i> depending on which node is scheduled (no task.name)? This way we're consistent in both cases.

EDIT: thinking more, I think it may be better if for 1-node clusters, we just do what you suggested:

For single node tasks, the logs display the name of the task, or just task if no name is set.

(task, pid=32503) 0
(task, pid=32503) 172.31.95.145

For n-node clusters, for both 1-node tasks and m-node tasks we can go with the head/worker<i> prefix.

The rationale is if most users use 1-node clusters, there's one less concept ("head") to see and learn.

[concretevitamin]

Thanks @iojw! Did a pass.

[iojw]

Thanks @concretevitamin! To summarize the changes, there are no longer any differences between single-node and multi-node tasks on multi-node clusters in the logging output and log path. The difference lies between single-node clusters and multi-node clusters.

For single-node clusters:

• For logging, we display the task name, or just task if the name is not set. (no rank)
• We store logs at run.log
• This is so that users who primarily use single-node clusters need not know the concept of head nodes at all

For multi-node clusters:

• For logging, we always display node name along with rank, regardless of num_nodes
• We store logs at node_name-ip.log, where IP is the private ip of the node

I've also updated ResourceHandle so that we able to cache the value of the stable_cluster_internal_ips variable after it is retrieved the first time. Unsure if there might be an issue when workers get restarted since we do not invalidate the cache.

SIde-note: I think this also makes the code is a lot clearer!

[Michaelvll]

Thanks for the quick fix @iojw! These changes can potentially reduce the overhead of our system for launching tasks on multi-node clusters. Left several comments.

[iojw]

@Michaelvll I can run the smoke tests for AWS and Azure but not GCP since I don't have GCP credentials - could you help me run them for GCP?

[Michaelvll]

We got several smoke tests issues for incorrect cluster status refresh and some internal/external IP length mismatch problem:
https://gist.github.com/Michaelvll/c7f3891653ded20f0aa3cd41562bc285
https://gist.github.com/Michaelvll/1bfeeb379c355f7fde3baeab1a04f90d
Let's hold this PR from merging until we fix the test errors. : )

[Michaelvll]

Thanks for the quick fix @iojw! Just tried the sky status that has an existing cluster, and it worked. I am running the smoke test for this and the tests/backward_comaptibility_tests.sh.

Tested (c5ade55):

• tests/run_smoke_tests.sh
• tests/backward_comaptibility_tests.sh

[Michaelvll]

Thank you for the awesome work @iojw! It passed all the tests on my side. I think it is good to go.

[iojw]

Thank you @Michaelvll @concretevitamin for the reviews!