Repository: skypilot-org/skypilot
Pull request: Fix gcp t4 attach instance
<!-- Describe the changes in this PR -->

Fixes #2038.

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [x] Code formatting: `bash format.sh`
- [x] Any manual or new tests for this PR (please specify below)
  - [x] `sky launch --gpus T4:4 --cloud gcp`
- [ ] 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`


Problem statement:
GCP logic paired `T4:4` with a wrong instance type
```
» sky launch --gpus T4:4 --cloud gcp              
I 06-05 11:00:37 optimizer.py:636] == Optimizer ==
I 06-05 11:00:37 optimizer.py:648] Target: minimizing cost
I 06-05 11:00:37 optimizer.py:659] Estimated cost: $4.4 / hour
I 06-05 11:00:37 optimizer.py:659]
I 06-05 11:00:37 optimizer.py:733] Considered resources (1 node):
I 06-05 11:00:37 optimizer.py:781] --------------------------------------------------------------------------------------------
I 06-05 11:00:37 optimizer.py:781]  CLOUD   INSTANCE        vCPUs   Mem(GB)   ACCELERATORS   REGION/ZONE   COST ($)   CHOSEN
I 06-05 11:00:37 optimizer.py:781] --------------------------------------------------------------------------------------------
I 06-05 11:00:37 optimizer.py:781]  GCP     n1-340-48-tpu   48      340       T4:4           us-east1      4.36          ✔
I 06-05 11:00:37 optimizer.py:781] --------------------------------------------------------------------------------------------
```
The instance type n1-340-48-tpu is not expected and is a TPU instance type. We may be trying to find any n1-* instance with 48 CPUs.

cc @WoosukKwon @infwinston

Hints from issue discussion:

Commit message: add comment

Patch to review:
diff --git a/sky/clouds/service_catalog/gcp_catalog.py b/sky/clouds/service_catalog/gcp_catalog.py
index e3f32a8ceef..5d5f3586787 100644
--- a/sky/clouds/service_catalog/gcp_catalog.py
+++ b/sky/clouds/service_catalog/gcp_catalog.py
@@ -48,12 +48,16 @@
     # Memory: 1 GiB RAM per 1 vCPU;
     'n2-highcpu',
 ]
+_DEFAULT_HOST_VM_FAMILY = (
+    'n1-standard',
+    'n1-highmem',
+    'n1-highcpu',
+)
 _DEFAULT_NUM_VCPUS = 8
 _DEFAULT_MEMORY_CPU_RATIO = 4
 
 # This can be switched between n1 and n2.
 # n2 is not allowed for launching GPUs.
-_DEFAULT_HOST_VM_FAMILY = 'n1'
 _DEFAULT_GPU_MEMORY_CPU_RATIO = 4
 
 # TODO(zongheng): fix A100 info directly in catalog.
@@ -249,14 +253,17 @@ def get_instance_type_for_accelerator(
     if acc_name not in _NUM_ACC_TO_NUM_CPU:
         acc_name = 'DEFAULT'
 
-    assert _DEFAULT_HOST_VM_FAMILY == 'n1'
-
+    default_host_cpus = _NUM_ACC_TO_NUM_CPU[acc_name].get(acc_count, None)
     if cpus is None and memory is None:
-        cpus = f'{_NUM_ACC_TO_NUM_CPU[acc_name].get(acc_count, None)}+'
+        assert default_host_cpus is not None, (acc_name, acc_count)
+        cpus = f'{default_host_cpus}+'
     if memory is None:
-        memory = f'{_DEFAULT_GPU_MEMORY_CPU_RATIO}x'
+        assert cpus is not None, (acc_name, acc_count)
+        cpu_val = int(cpus.strip('+').strip('x'))
+        # The memory size should be at least 4x the requested number of vCPUs.
+        memory = f'{cpu_val * _DEFAULT_MEMORY_CPU_RATIO}+'
     df = _df[_df['InstanceType'].notna()]
-    df = df[df['InstanceType'].str.startswith(f'{_DEFAULT_HOST_VM_FAMILY}-')]
+    df = df[df['InstanceType'].str.startswith(_DEFAULT_HOST_VM_FAMILY)]
 
     instance_type = common.get_instance_type_for_cpus_mem_impl(df, cpus, memory)
     # The fuzzy candidate should have already been fetched in the caller.


Code review comments:
diff_hunk: @@ -249,14 +253,17 @@ def get_instance_type_for_accelerator(
     if acc_name not in _NUM_ACC_TO_NUM_CPU:
         acc_name = 'DEFAULT'
 
-    assert _DEFAULT_HOST_VM_FAMILY == 'n1'
-
+    default_host_cpus = _NUM_ACC_TO_NUM_CPU[acc_name].get(acc_count, None)
     if cpus is None and memory is None:
-        cpus = f'{_NUM_ACC_TO_NUM_CPU[acc_name].get(acc_count, None)}+'
+        assert default_host_cpus is not None, (acc_name, acc_count)
+        cpus = f'{default_host_cpus}+'
     if memory is None:
-        memory = f'{_DEFAULT_GPU_MEMORY_CPU_RATIO}x'
+        assert cpus is not None, (acc_name, acc_count)
+        cpu_val = int(cpus.strip('+').strip('x'))
+        # The memory size should be at least 4x the requested number of vCPUs.
path: sky/clouds/service_catalog/gcp_catalog.py
text: @user1:
add: ...to be consistent with all other clouds?

@author:
Done. Thanks!

diff_hunk: @@ -48,12 +48,16 @@
     # Memory: 1 GiB RAM per 1 vCPU;
     'n2-highcpu',
 ]
+_DEFAULT_HOST_VM_FAMILY = (
+    'n1-standard',
+    'n1-highmem',
+    'n1-highcpu',
+)
 _DEFAULT_NUM_VCPUS = 8
 _DEFAULT_MEMORY_CPU_RATIO = 4
 
 # This can be switched between n1 and n2.
 # n2 is not allowed for launching GPUs.
path: sky/clouds/service_catalog/gcp_catalog.py
text: @user1:
Probably should update / move to L51?

@author:
Good point! Fixed. Thanks!