Repository: skypilot-org/skypilot
Pull request: Fix optimizer messages
This PR closes #700

The fixed optimizer will 
* skip the head message (`Optimizer - plan minimizing cost`) if the search space is trivial, and
* print hourly cost when the DAG consists of a task that does not have a time estimator and does not involve any data egress.

NOTE: `sky gpunode` still prints the header message because there are three candidates (each for three cloud vendors).
```
$ sky gpunode
I 04-04 19:09:08 optimizer.py:601] Optimizer - plan minimizing cost
I 04-04 19:09:08 optimizer.py:613] Estimated hourly cost: ~$0.2/hr
I 04-04 19:09:08 optimizer.py:628] 
I 04-04 19:09:08 optimizer.py:628] TASK     BEST_RESOURCE
I 04-04 19:09:08 optimizer.py:628] gpunode  Azure(Standard_NC6_Promo, {'K80': 1})
I 04-04 19:09:08 optimizer.py:628] 
I 04-04 19:09:08 optimizer.py:648] Considered resources -> cost ($)
I 04-04 19:09:08 optimizer.py:649] {GCP(n1-highmem-8, {'K80': 1}): 0.92,
I 04-04 19:09:08 optimizer.py:649]  AWS(p2.xlarge, {'K80': 1}): 0.9,
I 04-04 19:09:08 optimizer.py:649]  Azure(Standard_NC6_Promo, {'K80': 1}): 0.22}
I 04-04 19:09:08 optimizer.py:649] 
I 04-04 19:09:08 optimizer.py:665] Multiple Azure instances satisfy K80:1. The cheapest Azure(Standard_NC6_Promo, {'K80': 1}) is considered among:
I 04-04 19:09:08 optimizer.py:665] ['Standard_NC6_Promo', 'Standard_NC6'].
I 04-04 19:09:08 optimizer.py:665]
```

On the other hand, `sky gpunode --cloud aws` skips the message (`Optimizer - plan minimizing cost`) since the candidate instance type is unique.
```
$ sky gpunode --cloud aws
I 04-04 19:04:58 optimizer.py:613] Estimated hourly cost: ~$0.9/hr
I 04-04 19:04:58 optimizer.py:628] 
I 04-04 19:04:58 optimizer.py:628] TASK     BEST_RESOURCE
I 04-04 19:04:58 optimizer.py:628] gpunode  AWS(p2.xlarge, {'K80': 1})
I 04-04 19:04:58 optimizer.py:628]
```


Problem statement:
User is confused by optimizer messages for single-task/single-resource
```
$ sky gpunode --spot
I 04-04 12:31:57 optimizer.py:594] Optimizer - plan minimizing cost
I 04-04 12:31:57 optimizer.py:597] Estimated total run time: ~1.0 hr, total cost: ~$0.3
I 04-04 12:31:57 optimizer.py:609]
I 04-04 12:31:57 optimizer.py:609] TASK     BEST_RESOURCE
I 04-04 12:31:57 optimizer.py:609] gpunode  AWS(p2.xlarge[Spot], {'K80': 1})
I 04-04 12:31:57 optimizer.py:609]
Launching a new cluster. Proceed? [Y/n]:
```
User is confused by two things:

- what does the “estimated total run time” mean here?
- Also curious what’s the default instance type selection logic; e.g. why wasn’t g4dn.xlarge chosen as default if minimizing cost?

----

Proposal: Remove the following two lines
```
I 04-04 12:31:57 optimizer.py:594] Optimizer - plan minimizing cost
I 04-04 12:31:57 optimizer.py:597] Estimated total run time: ~1.0 hr, total cost: ~$0.3
```
when it makes sense (perhaps when there's a single-task + single-resource)?

The first line is definitely confusing.

For the second line, perhaps replace it with "Estimated hourly cost: ~$0.3".

@WoosukKwon @infwinston could you look into this?

Hints from issue discussion:
> For the second line, perhaps replace it with "Estimated hourly cost: ~$0.3".

Sounds good. Will fix this.

> The first line is definitely confusing.

Agreed. Will fix to skip the line whenever appropriate.

> Also curious what’s the default instance type selection logic; e.g. why wasn’t g4dn.xlarge chosen as default if minimizing cost?

It's hardcoded, right? Do you think we need to print out some messages about this as well?
> > Also curious what’s the default instance type selection logic; e.g. why wasn’t g4dn.xlarge chosen as default if minimizing cost?
> 
> It's hardcoded, right? Do you think we need to print out some messages about this as well?

I believe the confusion is the following. The user reads "plan minimizing cost". So he assumes this is within some search space. He does not know what the search space is. So he assumes the search space is all possible GPU instances on AWS, and so g4dn.xlarge should be chosen as it should be cheaper than our hard-coded K80 VM.   

The problem is search space here == resource choices for each task. For the most common 1-task/1-resource case, we may want to hide this concept of "minimizing X (in a trivial search space)".
Speaking of the default gpu node type, should we consider changing it to `g4dn.xlarge`? It can be cheaper (0.900 vs 0.526, tho host memory size is smaller) and T4 is a newer generation than K80. (remember that we had to choose images with older nvidia driver in order to support K80). It supports mixed precision and seems to be more cost efficient than K80 ([details here](https://cloud.google.com/blog/products/ai-machine-learning/your-ml-workloads-cheaper-and-faster-with-the-latest-gpus)).

<img width="400" alt="Screen Shot 2022-04-04 at 4 59 02 PM" src="https://user-images.githubusercontent.com/1206058/161652807-d82d6ad6-9c8d-4b74-8f7b-8e8c249c7a6a.png">


Good idea!

Commit message: Fix optimizer msg

Patch to review:
diff --git a/sky/optimizer.py b/sky/optimizer.py
index a882eb9ac28..5d3f37e5980 100644
--- a/sky/optimizer.py
+++ b/sky/optimizer.py
@@ -590,12 +590,31 @@ def print_optimized_plan(
         node_to_cost_map: _TaskToCostMap,
         minimize_cost: bool,
     ):
-        if minimize_cost:
-            logger.info('Optimizer - plan minimizing cost')
+        node_to_cost_map = {
+            k: v
+            for k, v in node_to_cost_map.items()
+            if k.name not in (_DUMMY_SOURCE_NAME, _DUMMY_SINK_NAME)
+        }
+        is_trivial = any(len(v) == 1 for v in node_to_cost_map.values())
+        if not is_trivial:
+            if minimize_cost:
+                logger.info('Optimizer - plan minimizing cost')
+            else:
+                logger.info('Optimizer - plan minimizing run time')
+
+        print_hourly_cost = False
+        if len(node_to_cost_map) == 1:
+            node = list(node_to_cost_map.keys())[0]
+            if node.time_estimator_func is None and \
+                node.get_inputs() is None and node.get_outputs() is None:
+                print_hourly_cost = True
+
+        if print_hourly_cost:
+            logger.info(f'Estimated hourly cost: ~${total_cost:.1f}/hr')
         else:
-            logger.info('Optimizer - plan minimizing run time')
-        logger.info(f'Estimated total run time: ~{total_time / 3600:.1f} hr, '
-                    f'total cost: ~${total_cost:.1f}')
+            logger.info(
+                f'Estimated total run time: ~{total_time / 3600:.1f} hr, '
+                f'total cost: ~${total_cost:.1f}')
 
         # Do not print Source or Sink.
         message_data = [
@@ -608,16 +627,11 @@ def print_optimized_plan(
                                     tablefmt='plain')
         logger.info(f'\n{message}\n')
 
+        # Print the egress plan if any data egress is scheduled.
         Optimizer._print_egress_plan(graph, best_plan, minimize_cost)
 
         # Print the list of resouces that the optimizer considered.
-        should_print = any(len(v) > 1 for v in node_to_cost_map.values())
-        if should_print:
-            node_to_cost_map = {
-                k: v
-                for k, v in node_to_cost_map.items()
-                if k.name not in (_DUMMY_SOURCE_NAME, _DUMMY_SINK_NAME)
-            }
+        if not is_trivial:
             metric = 'cost ($)' if minimize_cost else 'time (hr)'
             for k, v in node_to_cost_map.items():
                 node_to_cost_map[k] = {


Code review comments:
diff_hunk: @@ -590,12 +590,31 @@ def print_optimized_plan(
         node_to_cost_map: _TaskToCostMap,
         minimize_cost: bool,
     ):
-        if minimize_cost:
-            logger.info('Optimizer - plan minimizing cost')
+        node_to_cost_map = {
+            k: v
+            for k, v in node_to_cost_map.items()
+            if k.name not in (_DUMMY_SOURCE_NAME, _DUMMY_SINK_NAME)
+        }
+        is_trivial = any(len(v) == 1 for v in node_to_cost_map.values())
+        if not is_trivial:
+            if minimize_cost:
+                logger.info('Optimizer - plan minimizing cost')
+            else:
+                logger.info('Optimizer - plan minimizing run time')
+
+        print_hourly_cost = False
+        if len(node_to_cost_map) == 1:
+            node = list(node_to_cost_map.keys())[0]
+            if node.time_estimator_func is None and \
path: sky/optimizer.py
text: @user1:
Consider dropping the \: using parens to wrap + yapf will automatically format it.

if (node.time_.... is None):

@author:
Good point. I didn't try the parentheses. Will fix that.

diff_hunk: @@ -590,12 +590,31 @@ def print_optimized_plan(
         node_to_cost_map: _TaskToCostMap,
         minimize_cost: bool,
     ):
-        if minimize_cost:
-            logger.info('Optimizer - plan minimizing cost')
+        node_to_cost_map = {
+            k: v
+            for k, v in node_to_cost_map.items()
+            if k.name not in (_DUMMY_SOURCE_NAME, _DUMMY_SINK_NAME)
+        }
+        is_trivial = any(len(v) == 1 for v in node_to_cost_map.values())
+        if not is_trivial:
+            if minimize_cost:
+                logger.info('Optimizer - plan minimizing cost')
+            else:
+                logger.info('Optimizer - plan minimizing run time')
+
+        print_hourly_cost = False
+        if len(node_to_cost_map) == 1:
+            node = list(node_to_cost_map.keys())[0]
+            if node.time_estimator_func is None and \
+                node.get_inputs() is None and node.get_outputs() is None:
+                print_hourly_cost = True
+
+        if print_hourly_cost:
+            logger.info(f'Estimated hourly cost: ~${total_cost:.1f}/hr')
path: sky/optimizer.py
text: @user1:
Surprised to see "/hr" when we already said "hourly cost". Maybe remove the former?

@author:
Sounds good.

diff_hunk: @@ -590,12 +590,31 @@ def print_optimized_plan(
         node_to_cost_map: _TaskToCostMap,
         minimize_cost: bool,
     ):
-        if minimize_cost:
-            logger.info('Optimizer - plan minimizing cost')
+        node_to_cost_map = {
+            k: v
+            for k, v in node_to_cost_map.items()
+            if k.name not in (_DUMMY_SOURCE_NAME, _DUMMY_SINK_NAME)
+        }
+        is_trivial = any(len(v) == 1 for v in node_to_cost_map.values())
path: sky/optimizer.py
text: @user1:
should any() be all() instead?

task1: 1 resource choice
task2: 2 resource choices

in this case the search space is not trivial?

@author:
You're right. Thanks for letting me know. I made a mistake when I cleaned the code. Will fix that right away.