-
Notifications
You must be signed in to change notification settings - Fork 547
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
Minor update to sort by price in show-gpus #2933
Minor update to sort by price in show-gpus #2933
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. GCP requires some care; the following doesn't sort properly:
sky show-gpus --all-regions A100 --cloud gcp
sky show-gpus T4 --all-regions --cloud gcp
Examples mentioned so far seem correct, but adding @Michaelvll to take a look at the dataframe wrangling. I'd have thought the groupby() is easier to understand than drop_duplicates(); also, seems like by removing groupby() we now have some interleaving
whereas on master
|
Fixed now! I am adding the sorting by intsance_type as well, because the sorting results in a reshuffling. Also I simplified the logic so we can sort regardless of |
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 @Vaibhav2001! The sort_values
and drop_duplicates
implementation looks good to me. Just a minor nit.
@Michaelvll Hi made those changes, should be good to merge. |
This pertains to #2863, and updates previous PR #2892
Minor update to fix the sorting issue mentioned by @concretevitamin #2892 (comment)