-
Notifications
You must be signed in to change notification settings - Fork 510
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
[UI] Now show-gpus shows the combined price for GCP GPU instance #2946
[UI] Now show-gpus shows the combined price for GCP GPU instance #2946
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 @Vaibhav2001 for this amazing feature! 🚀 It will be really helpful to compare the prices across clouds. Left some comments and discussions 🫡
Also currently I am failing https://github.com/skypilot-org/skypilot/actions/runs/7429948668/job/20219105846?pr=2946. I do not understand why it fails. Essentially from my understanding I am assigning an instance type to a TPU VM and that is wrong, but why? Do we not want to attach an instance to the tpu accelerator types? Also @cblmemo if you can advice on what to do for the mypy issue. I can just add that check I think? |
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 prompt fix!! Left some more comments 🫡
Hi @Vaibhav2001 ! Thanks for the discussion 🙂 For the first question, that is because TPU has two types in GCP: TPU VMs and TPU Nodes. You can refer to this doc for more information. A TPU Node will be attached to an instance, whereas a TPU VM is a stand-alone instance so it cannot have an instance type. For the pricing here, a TPU VM's price doesn't need to add the price for an instance. For the second mypy question, from the error message shown in the link, it is because skypilot/sky/clouds/service_catalog/gcp_catalog.py Lines 254 to 267 in a31fdd9
In this case, we can investigate in what cases will the return value be |
@cblmemo Thanks for these comments. I addressed them and pushed the new version. |
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 prompt fix!!! 🚀 Left some discussions ;)
fixing comment Co-authored-by: Tian Xia <cblmemo@gmail.com>
Co-authored-by: Tian Xia <cblmemo@gmail.com>
@cblmemo. Addressed all comments! :) |
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 prompt fix!! Some final comments 🚀 It looks mostly ready to me!
Co-authored-by: Tian Xia <cblmemo@gmail.com>
Addressed! |
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.
Please double-check that all tests still works fine; otherwise LGTM!
@cblmemo All the tests seem to be working fine! Should be good to ship now! |
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.
LGTM! Thanks for adding this exciting feature again!! 🚀🙌🏻
This is in relation to #2039.
Added functionality to show the combined price for GCP GPU instance.
To test: