[Cost Report] Add status for cost report #1621
Conversation
There was a problem hiding this comment.
Thanks for submitting the PR @sumanthgenz! Left several comments. The most concerning part might be status table in the cluster_history table, as it is redundant with the status column in clusters table.
sky/global_user_state.py
Outdated
|
|
||
| db_utils.add_column_to_table(cursor, conn, 'cluster_history', 'status', | ||
| 'TEXT DEFAULT null') |
There was a problem hiding this comment.
It seems to me this column is not necessary for the cluster_history table as we can always do a join for the cluster_history and the clusters table to get the status. Do you think we should do the something like the following whenever we need the status instead?
SELECT *
FROM cluster_history
LEFT OUTER JOIN clusters ON cluster_history.cluster_hash=clusters.cluster_hash
There was a problem hiding this comment.
Should we only show status for clusters that are not terminated, so clusters that are in both the clusters and cluster_history table?
There was a problem hiding this comment.
Actually never mind, lets show terminated as well, which will be a result of when the sql join query returns nothing.
sky/global_user_state.py
Outdated
| @@ -134,6 +140,7 @@ def colored_str(self): | |||
| ClusterStatus.INIT: colorama.Fore.BLUE, | |||
| ClusterStatus.UP: colorama.Fore.GREEN, | |||
| ClusterStatus.STOPPED: colorama.Fore.YELLOW, | |||
| ClusterStatus.TERMINATED: colorama.Fore.RED, | |||
There was a problem hiding this comment.
It would be a bit alerting to use RED. It could be better to use some less noticeable color, e.g. GREY instead?
sky/global_user_state.py
Outdated
| # Stopped. This means a `sky down` call has previously succeeded. | ||
| TERMINATED = 'TERMINATED' | ||
|
|
There was a problem hiding this comment.
We may not want to have the TERMINATED status in the enum as it will not actually appear in the clusters table.
How about we move the colored_str function to cli_utils.status_utils.py instead and change the _STATUS_TO_COLOR to:
_STATUS_TO_COLOR = {
ClusterStatus.INIT.value: colorama.Fore.BLUE,
ClusterStatus.UP.value: colorama.Fore.GREEN,
ClusterStatus.STOPPED.vaue: colorama.Fore.YELLOW,
'TERMINATED': <color for the terminated status>,
There was a problem hiding this comment.
Thanks for the quick fix @sumanthgenz! Left several comments. : )
sky/global_user_state.py
Outdated
| def get_cluster_history_status(cluster_hash: str): | ||
|
|
||
| rows = _DB.cursor.execute( | ||
| 'SELECT clusters.status FROM cluster_history ' | ||
| 'INNER JOIN clusters ' | ||
| 'ON cluster_history.cluster_hash=clusters.cluster_hash ' | ||
| 'WHERE cluster_history.cluster_hash=(?)', (cluster_hash,)) | ||
| for (status,) in rows: | ||
| return status | ||
| return 'TERMINATED' |
There was a problem hiding this comment.
For efficiency, why don't we join the two whole tables and retrieve the results, i.e. at the beginning of get_clusters_from_history do something like the following?
rows = _DB.cursor.execute(
'SELECT (ch.hash, ch.name, ch.num_nodes, ch.launched_resources, ch.usage_intervals, clusters.status) FROM cluster_history ch'
'INNER JOIN clusters '
'ON ch.cluster_hash=clusters.cluster_hash '
)
sky/global_user_state.py
Outdated
| usage_intervals, | ||
| ) = row[:6] | ||
|
|
||
| # backwards compatibility for null status in cluster_history |
sky/global_user_state.py
Outdated
| ClusterStatus.INIT.value: colorama.Fore.BLUE, | ||
| ClusterStatus.UP.value: colorama.Fore.GREEN, | ||
| ClusterStatus.STOPPED.value: colorama.Fore.YELLOW, | ||
| 'TERMINATED': colorama.Fore.BLACK, # sky down |
There was a problem hiding this comment.
I am sorry, but after looking at the new changes, I found it might be better to revert this by adding the TERMINATED status to the ClusterStatus. Can we revert to your original changes for the ClusterStatus and move that colored_str back in the ClusterStatus class? We may also want to add a comment in the ClusterStatus saying that the TERMINATED status will not appear in the clusters table.
sky/global_user_state.py
Outdated
| @@ -544,15 +550,33 @@ def get_clusters() -> List[Dict[str, Any]]: | |||
|
|
|||
|
|
|||
| def get_clusters_from_history() -> List[Dict[str, Any]]: | |||
| rows = _DB.cursor.execute('SELECT * from cluster_history').fetchall() | |||
| rows = _DB.cursor.execute('SELECT * from cluster_history') | |||
There was a problem hiding this comment.
Does this line need to be removed?
sky/global_user_state.py
Outdated
| @@ -134,6 +139,7 @@ def colored_str(self): | |||
| ClusterStatus.INIT: colorama.Fore.BLUE, | |||
| ClusterStatus.UP: colorama.Fore.GREEN, | |||
| ClusterStatus.STOPPED: colorama.Fore.YELLOW, | |||
| ClusterStatus.TERMINATED: colorama.Fore.BLACK, | |||
There was a problem hiding this comment.
How about using colorama.Style.DIM?
| # '(cluster_hash, name, num_nodes, requested_resources, ' | ||
| # 'launched_resources, usage_intervals) ' |
* status for cost report * address PR changes * remove status column in add update clusters for history * address PR comments * address PR changes * remove terminated from cluster status --------- Co-authored-by: Sumanth <[email protected]>
Added a status column, which includes a new ClusterStatus type TERMINATED, to the cost report.
Tested (each the same cluster, just when calling
sky cost-reportat different times:'''
cluster 1 min ago 1m 8s 1x GCP(n1-highmem-8, {'V100': 1}) INIT $ 2.953 $0.056
cluster 4 mins ago 4m 31s 1x GCP(n1-highmem-8, {'V100': 1}) UP $ 2.953 $0.222
cluster 21 mins ago 15m 9s 1x GCP(n1-highmem-8, {'V100': 1}) STOPPED $ 2.953 $0.746
cluster 34 mins ago 22m 31s 1x GCP(n1-highmem-8, {'V100': 1}) TERMINATED $ 2.953 $1.108
'''