-
Notifications
You must be signed in to change notification settings - Fork 37
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
cluster list filters #1233
base: sb/cluster-list-last-active-info
Are you sure you want to change the base?
cluster list filters #1233
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
81ef086
to
e41609e
Compare
4b2517b
to
e150b64
Compare
e41609e
to
1633300
Compare
582a6d7
to
1c535c0
Compare
1633300
to
efd4fa9
Compare
0dcf85b
to
81c7cac
Compare
efd4fa9
to
b2d1f10
Compare
b10541c
to
4e7363b
Compare
4e7363b
to
15b29f8
Compare
den_clusters = den_clusters_resp.json().get("data") | ||
|
||
# get sky live clusters | ||
sky_live_clusters = sky.status(refresh=True) |
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.
just confirming - does this by default only show live ones? (what about ones in "INIT" state for example?)
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.
According to skypilot documentation, sky.status()
will return clusters with the following statuses: UP
, INIT
or STOPPED
.
If the cluster is saved in den, we won't treat it as sky cluster, even if its status is INIT
or STOPPED
.
It’s more of a product question, but we’ll probably need to decide how to handle INIT
clusters that are saved to the den. I think it might be worth consulting with Donny about this because I remember him mentioning that we want to minimize the use of sky commands and create a wrapper around them, so users will interact with the Runhouse API instead of the Skypilot API.
b2d1f10
to
c31c146
Compare
15b29f8
to
23218be
Compare
c31c146
to
447a12c
Compare
23218be
to
3a3f0a4
Compare
447a12c
to
1481a0d
Compare
5481af9
to
a5edfdd
Compare
def _create_output_table( | ||
total_clusters: int, running_clusters: int, displayed_clusters: int | ||
): | ||
displayed_running_clusters = ( |
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.
wondering if we should have some sort of pagination feature here, meaning if for example there are 100 clusters and we show 10 when the command is first run, we could make it easy for the user to toggle through the complete list. wdyt?
cc @mkandler - we're doing something similar for the resources page in Den right now also
if not sky_live_clusters and not den_clusters: | ||
return [], [], [] | ||
|
||
sky_live_clusters = Cluster._get_unsaved_live_clusters( |
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.
a little confused by the naming - just to clarify:
sky_live_clusters
: sky clusters found in the local sky DB but not saved in den?
running_clusters
: running clusters which are saved in Den?
not running clusters
: clusters that are terminated / unknown / down which are also saved in Den?
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.
exactly, will add this explanation as a comment
e197306
to
2f32e58
Compare
a5edfdd
to
88948f8
Compare
2f32e58
to
d633ff7
Compare
88948f8
to
a388bb8
Compare
a388bb8
to
001f605
Compare
d633ff7
to
b98bad0
Compare
001f605
to
edf7c85
Compare
b98bad0
to
324d6cf
Compare
436a61a
to
2959b98
Compare
2959b98
to
c79fc7d
Compare
runhouse cluster list --a
output: