-
Notifications
You must be signed in to change notification settings - Fork 549
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
[sky/feat/show-gpus] adding region based filtering for show-gpus command. #1187
[sky/feat/show-gpus] adding region based filtering for show-gpus command. #1187
Conversation
@romilbhardwaj can you help me approve the workflows blocked for the first-time contributors? |
Done, should be running now! |
Should be ready for review contingent on the fact that the workflows pass! @romilbhardwaj |
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.
Great work @vivekkhimani! Left some comments and notes on unintended behavior I observed.
It would be great to have hints on regions supported by a cloud if the user enters incorrect regions. For instance, for small typos, I noticed we have automatic suggestions which is great:
However, for incorrectly specified regions (severe typos? :) ), the error message isn't very helpful:
Can we have a line like |
ProblemI noticed that we added For instance, we know that V100 is available in aws us-east-1, aws us-east-2 and aws us-west-2. This is also recorded in our catalog (L4884-L4896 in aws.csv). However, when I run
Of course, we don't want to show all regions in a new line in Proposed solution
Only problem is this will likely be a long output with lots of similar lines, but it does solve the pain point in #1170. @Michaelvll please feel free to suggest any other way you'd like to see per region pricing information. |
@romilbhardwaj thanks for the review! All of these comments make sense and I have a pretty good idea to fix them. I have been caught up w some school work but I will get to this by end of this week. |
Hi @vivekkhimani - any updates on this? |
…skypilot-1170-gpu-pricing-by-region
…skypilot-1170-gpu-pricing-by-region
…skypilot-1170-gpu-pricing-by-region
…skypilot-1170-gpu-pricing-by-region
Added a fix for this @romilbhardwaj
@romilbhardwaj very true! fixed this by disabling the region column on |
@romilbhardwaj extremely sorry for the huge delay! I got busy with school and my own project because of a conference deadline and then was traveling during the winter break. I fixed/resolved all the comments and most of the pylint/yapf issues. Can you give me a maintainer approval to run the pipelines again? |
@romilbhardwaj supporting screenshots for the fix.
|
Thanks @vivekkhimani! I'll get to this soon. |
…skypilot-1170-gpu-pricing-by-region
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 @vivekkhimani - I tested this out and it works nicely (including for gcp TPUs)! Left some minor nit comments - should be ready to ship once resolved!
…skypilot-1170-gpu-pricing-by-region
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.
Awesome work @vivekkhimani! This looks good to go!
Thanks for reviewing it all along, @romilbhardwaj ! Don't think I can merge this as I don't have the write access to repository so do you mind doing it for me? |
Acceptance Criteria
This PR aims to add the features proposed in #1170. Based on the feedback received from the discussion in the issue, the following changes are introduced:
--region
flag on theshow-gpus
command.cloud
flag is provided.--region
argument will be invalidated if it can't be found in the target provider.show-gpus
documentation to include the--region
flag and the instructions to use it.