-
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
print cloud info in cluster status #1209
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
852c2b8
to
07bac3e
Compare
if cloud_properties: | ||
properties_to_check = ["cloud", "instance_type", "region", "cost_per_hour"] | ||
for p in properties_to_check: | ||
property_value = ( |
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.
what about the cloud
property necessitates this?
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 could add a validation for the launched_properties
in the status output, but i think it is more of a den test.
The point of test is to make sure that the cloud info is printed as expected to stdout
.
@jlewitt1 is this what you were referencing to? Or something else?
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 meant what makes cloud differnt from the other properties, but seems like it's just bc of how Sky formats it?
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.
yes, i think so. We could also double check it with Caroline.
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.
you added this line cloud = cloud_properties.get("cloud").upper()
in _print_cloud_properties
. I think this might be in response to me adding the cloud = str(launched_resource.cloud).lower()
in setting the launched properties cloud attribute?
In the sky status resource, they have the cloud capitalized (Kubernetes, AWS, GCP, Lambda, etc) but I thought it simpler to just use the lowercase form (since that's what we prefer/use in our examples anyways). I think we can leave it that way for printing out in status too? But if we want the original form then we should update the launched_properties attribute to that too. Since calling upper()
doesn't work in every case
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.
@carolineechen sounds good, I don't have strong opinion about that. If lower case is consistent with out examples, let's print the cloud in the status as lower case as well. :)
07bac3e
to
a6f81bb
Compare
010f8de
to
0987928
Compare
5284e6d
to
edda744
Compare
edda744
to
649d5ee
Compare
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
Co-authored-by: Alexandra Belousov <sashabelousovrh@Alexandras-MacBook-Pro.local>
Output will looks as follows:
Single-node GPU
AWS g5.xlarge cluster | 🌍 us-east-1 | 💰 $1.006/hr
Multi-node GPU
2x AWS g5.xlarge cluster | 🌍 us-east-1 | 💰 $1.006/hr
Single-node CPU
AWS m6i.xlarge cluster | 🌍 us-east-1 | :💸: $0.096/hr
Multi-node CPU
2x AWS m6i.xlarge cluster | 🌍 us-east-1 | :💸: $0.096/hr