Skip to content
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

[UX] Fix GCP show-gpus order after combining VM types #3083

Merged
merged 6 commits into from
Feb 6, 2024

Conversation

Michaelvll
Copy link
Collaborator

The current price order for GCP in sky show-gpus is incorrect, because:

  1. The VM price used for the total price is the cheapest one across all regions instead of the current region.
  2. We only sort the price before combining the price of VM and GPUs. This PR re-sort the price/spot price after combining GPU and host VM price.

It also fixes #2953.

Tested (run the relevant ones):

  • Code formatting: bash format.sh
  • Any manual or new tests for this PR (please specify below)
    • sky show-gpus a100
    • sky show-gpus a100 --all-regions
    • sky show-gpus
    • sky show-gpus v100:1 --all-regions
      Before this PR
    sky show-gpus v100:1 --cloud gcp --all-regions
    GPU   QTY  CLOUD  INSTANCE_TYPE  DEVICE_MEM  vCPUs  HOST_MEM  HOURLY_PRICE  HOURLY_SPOT_PRICE  REGION        
    V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 0.811            asia-east1    
    V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.013            us-central1   
    V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.013            us-east1      
    V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.013            us-west1      
    V100  1    GCP    n1-highmem-8   -           8      52GB      $ 3.023       $ 0.938            europe-west4  
    
  After this PR
  ```console
  $ sky show-gpus v100 --cloud gcp --all-regions
  GPU   QTY  CLOUD  INSTANCE_TYPE  DEVICE_MEM  vCPUs  HOST_MEM  HOURLY_PRICE  HOURLY_SPOT_PRICE  REGION        
  V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.037            us-central1   
  V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.037            us-east1      
  V100  1    GCP    n1-highmem-8   -           8      52GB      $ 2.953       $ 1.037            us-west1      
  V100  1    GCP    n1-highmem-8   -           8      52GB      $ 3.028       $ 0.845            asia-east1    
  V100  1    GCP    n1-highmem-8   -           8      52GB      $ 3.071       $ 0.973            europe-west4  

Note: for example, asia-east1 has different hourly price, because before this PR we use the same VM price for all different regions which is not correct.

  • All smoke tests: pytest tests/test_smoke.py
  • Relevant individual smoke tests: pytest tests/test_smoke.py::test_fill_in_the_name
  • Backward compatibility tests: bash tests/backward_comaptibility_tests.sh

sky/clouds/service_catalog/gcp_catalog.py Outdated Show resolved Hide resolved
sky/clouds/service_catalog/gcp_catalog.py Outdated Show resolved Hide resolved
Comment on lines +468 to +472
for info in acc_infos:
info_key = _get_key(info)
if cur_key != info_key:
cur_key = info_key
new_acc_infos.append(info)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we are keeping the first entry in the list. Why is that the cheapest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we have sorted it by the price in L454. : )

Michaelvll and others added 3 commits February 5, 2024 10:53
Co-authored-by: Tian Xia <cblmemo@gmail.com>
Co-authored-by: Tian Xia <cblmemo@gmail.com>
Copy link
Collaborator

@cblmemo cblmemo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM. Left a nit on readability

sky/clouds/service_catalog/gcp_catalog.py Outdated Show resolved Hide resolved
@Michaelvll Michaelvll merged commit 98e92fa into master Feb 6, 2024
19 checks passed
@Michaelvll Michaelvll deleted the fix-show-gpus-gcp-regions branch February 6, 2024 17:29
@concretevitamin
Copy link
Collaborator

This seems to slow down show-gpus significantly. Any quick way to fix?

» time sky show-gpus --cloud gcp
  • This PR: 6.19s user 0.47s system 98% cpu 6.771 total
  • 872d969: 1.22s user 0.43s system 114% cpu 1.437 total

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Catalog] Inconsistency for GCP, A100:16 instance
3 participants