-
Notifications
You must be signed in to change notification settings - Fork 460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Cost Report] Add status for cost report #1621
[Cost Report] Add status for cost report #1621
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remnant?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using colorama.Style.DIM
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix @sumanthgenz! LGTM.
# '(cluster_hash, name, num_nodes, requested_resources, ' | ||
# 'launched_resources, usage_intervals) ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
* 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-report
at 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
'''