Add zone support in YAML #1014

infwinston · 2022-07-28T05:40:58Z

This PR adds support for specifying zone in YAML which avoids the need to modify catalog manually. This feature can be useful for TPU users who often get quota only in specific zones (such as us-central1-c but not us-central1-a).
Also, some accelerator validation is added when region/zone is specified.
I'm not sure whether CLI argument overwrite (--zone) is useful so I remove it in the latest commit. But we can bring it back if needed.

Usage example:

resources:
  cloud: gcp
  zone: europe-west4-a
  accelerators: tpu-v3-8
  accelerator_args:
    runtime_version: tpu-vm-base
    tpu_vm: True

Hint example:

(sky) weichiang@mbp sky % sky launch examples/tpu/tpuvm_mnist.yaml
Task from YAML spec: examples/tpu/tpuvm_mnist.yaml
ValueError: Accelerator "tpu-v2-8" is not available in "us-central1-a" region/zone.

TODO:

add test function
smoke test passed
show "did you mean" hint

concretevitamin · 2022-07-29T15:58:24Z

Nice @infwinston. Some high-level UX comments.

resources:
  cloud: gcp
  region: us-west1-a

yields ValueError: Invalid region 'us-west1-a' for cloud GCP.. This is probably a common user "mistake" - can we additionally print, using difflib, something like "Do you mean region X"? Or perhaps difflib is not the right solution here and we should do some pattern matching and print "Do you mean zone 'us-west1-a' rather than region"?

Similar comment for

  zone: us-west1

which yields ValueError: Invalid zone 'us-west1' for cloud GCP..

It'd be great to add some unit test as well. I tried

resources:
  cloud: gcp
  zone: us-west1-a
  region: us-west2

manually and it correctly caught this mismatch: ValueError: Zone 'us-west1-a' is not in region 'us-west2'..

Michaelvll

Thank you for adding the support for zone @infwinston ! It would be very useful for the TPU users. Left several comments.

Another thing that should be fixed is that when user trying to launch or exec with zone specified on an existing cluster, we need to fail the program, similar as we did in the following:

skypilot/sky/backends/cloud_vm_ray_backend.py

Lines 1473 to 1479 in 9724b8f

    
           if (task_resources.region is not None and 
        
                   task_resources.region != launched_resources.region): 
        
               with ux_utils.print_exception_no_traceback(): 
        
                   raise exceptions.ResourcesMismatchError( 
        
                       'Task requested resources in region ' 
        
                       f'{task_resources.region!r}, but the existing cluster ' 
        
                       f'is in region {launched_resources.region!r}.')

We fill the region field of the launched_resources, when creating the handle. zone should be handled carefully:

Single node cluster: we need to fill the zone field of launched_resources.
Multi-node cluster: fill the zone field, only when all the instances are in the same zone.
Check fitness: task.resources.zone should be less demanding than launched_resources.zone, i.e. task.resources.zone is None -> True; task_resources.zone is not None and launched_resources.zone is None -> False; task.resources.zone == launched.resources.zone.

sky/resources.py

Michaelvll · 2022-07-29T17:45:58Z

sky/resources.py

+        if self._cloud is None:
+            with ux_utils.print_exception_no_traceback():
+                raise ValueError('Cloud must be specified together with zone.')
+        elif self._cloud.is_same_cloud(sky.Azure()):


nit: better to use clouds.Azure to be consistent with other places.

Looks like we also use sky.Azure() in a bunch of places. like

skypilot/sky/backends/cloud_vm_ray_backend.py

Line 687 in 9724b8f

elif cloud.is_same_cloud(sky.Azure()):

Should we also replace them?

Yes, we can replace them. : )

sky/clouds/service_catalog/gcp_catalog.py

sky/clouds/service_catalog/common.py

sky/backends/cloud_vm_ray_backend.py

sky/resources.py

Michaelvll

For backward compatibility, we need to update the zone information in the handle for a cluster launched before this PR.

skypilot/sky/backends/cloud_vm_ray_backend.py

Lines 1389 to 1407 in 661264a

    
           def _update_cluster_region(self): 
        
               if self.launched_resources.region is not None: 
        
                   return 
        
               config = common_utils.read_yaml(self.cluster_yaml) 
        
               provider = config['provider'] 
        
               cloud = self.launched_resources.cloud 
        
               if cloud.is_same_cloud(sky.Azure()): 
        
                   region = provider['location'] 
        
               elif cloud.is_same_cloud(sky.GCP()) or cloud.is_same_cloud( 
        
                       sky.AWS()): 
        
                   region = provider['region'] 
        
               elif cloud.is_same_cloud(sky.Local()): 
        
                   # There is only 1 region for Local cluster, 'Local'. 
        
                   local_regions = clouds.Local.regions() 
        
                   region = local_regions[0].name 
        
               self.launched_resources = self.launched_resources.copy( 
        
                   region=region)

Michaelvll · 2022-08-01T17:55:33Z

sky/backends/cloud_vm_ray_backend.py

+                    if zones != prev_resources.zones:
+                        raise ValueError(f'Zones mismatch. The zones in '
+                                         f'{handle.cluster_yaml} '
+                                         'have been changed from '
+                                         f'{prev_resources.zones} to {zones}.')


Please refer to #1014 (review), we need to add the zone information to the handle.launched_resources when creating the handle during provision, i.e. the following line, but the new changes seem do not set the zones in the handle.launched_resources.

skypilot/sky/backends/cloud_vm_ray_backend.py

Line 903 in 661264a

launched_resources=to_provision.copy(region=region.name),

Also, prev_resources.zones seems undefined, is it zone instead?

Also, the comparison is a bit tricky here, as zones can be a list of zone, but prev_resources.zones can be just a single zone. Consider something similar as the following:

if prev_resources.zone in zones: zones = [prev_resources.zone]

Thanks for catching this. It's fixed now. But I wonder the reason behind checking between Ray YAML zone field and the zone in the handle. Is this because we want to avoid user accidentally modifying the Ray YAML file?

sky/resources.py

sky/utils/cli_utils/status_utils.py

infwinston · 2022-08-01T23:50:13Z

@Michaelvll Sorry for the confusion. I should have left comments on why I haven't included zone info to the handle and also why I named the column REQUESTED_ZONE.

This is actually a bit tricky and may need some discussion.
Right now we are lacking a mechanism to identify the final zone of a launched cluster. On AWS, we call ray up with a list of zones set in availability_zone in Ray YAML. But we don't actually check the actual zone launched info afterwards.

Hence if we store such list us-east-1a,us-east-1b,us-east-1c,us-east-1d,us-east-1e,us-east-1f into our handle. It only means the zones requested by users. If for example users change their mind to specify only us-east-1b in YAML. We can't rely on this field to check whether we should raise mismatched error or reuse the cluster.

There are probably two ways:

We add codes to query the actual zone of cluster after or during ray up to reflect the info. We can then handle the above case.
We treat this field as a requested zone by users. If it's None then means no restriction and we raise error if user try to sky launch --zone us-east-1b on a cluster that's previously launched with no restriction.

Pros and cons may need to be further discussed. We can also do a zoom chat on this :)

infwinston · 2022-08-05T02:02:02Z

@concretevitamin I've added the hint support! Now inputing incorrect region or zone will be given a list of candidates if close enough. Let me know if this looks good :)

(sky) weichiang@mbp sky % sky launch examples/minimal.yaml --region us-west1
Task from YAML spec: examples/minimal.yaml
ValueError: Invalid region 'us-west1' for cloud AWS.
Did you mean one of these: us-west-1?

(sky) weichiang@mbp sky % sky launch examples/minimal.yaml --region us-west-1-a
Task from YAML spec: examples/minimal.yaml
ValueError: Invalid region 'us-west-1-a' for cloud AWS.
Did you mean one of these: us-west-1?

(sky) weichiang@mbp sky % sky launch examples/zone_test.yaml
Task from YAML spec: examples/zone_test.yaml
ValueError: Invalid zone 'us-central1' for cloud GCP.
Did you mean one of these: 'us-central1-a, us-central1-b, us-central1-c, us-central1-f'?

Also, smoke test has passed and the PR should be ready for review. Please take a look if you have time :)

Michaelvll

Thank you for the fix @infwinston! The logic looks good to me. Left several comments, mostly about the cleanness of the code. ; )

examples/containerized_app.py

examples/example_app.py

sky/backends/cloud_vm_ray_backend.py

Michaelvll · 2022-08-05T17:54:45Z

sky/backends/cloud_vm_ray_backend.py

+                # if zone is not specified because head and worker nodes
+                # can be launched in different zones.
+                if (task.num_nodes > 1 and
+                        handle.launched_resources.zone is None):


use task.num_nodes == 1 or handle.launched.resources.zone is not None and remove else.

Why do we need the second conditition? If the launched_resources has zone specified, no need to query again, right?

Done. for the second comment, right but the query can be a sanity check for ray up. I think it's still useful?

The sanity check may increase the overhead of the sky launch on an existing cluster, but checking it may be useful for catching problems, up to you.

If the zone mismatch, that probably means an error occurs, right? Should we fail directly instead?
One possible situation the error may occur is:

the user has a cluster stopped in us-west-1a

she tries to sky start the cluster, a resources unavailable error occurs on that zone, and ray up starts a new cluster on us-west-1b.

I think, in that case, we should consider terminating the newly launched instance (not sure if the old stopped instance will be terminated by ray up), but preserve the cluster entry in our database so that the user can retry and launch the instance in the previous zone again.

Oh just realized I missed this comment.. for 2, I thought sky start would just fail due to resources unavailable error? it shouldn't failover to other zone right or I misunderstand it?

Had offline discussion with @Michaelvll , the above case can happen also with the current master and should be considered a bug to fix. Will file an issue.

Filed an issue here. #1054 Will continue tracking this in the future.

sky/clouds/aws.py

tests/test_cli.py

tests/test_optimizer_dryruns.py

tests/test_optimizer_random_dag.py

tests/test_smoke.py

tests/test_spot.py

Michaelvll

Thank you for the fix @infwinston! It looks good to me. Left some nits.

sky/clouds/aws.py

sky/clouds/gcp.py

sky/clouds/aws.py

sky/clouds/cloud.py

sky/clouds/gcp.py

sky/clouds/service_catalog/gcp_catalog.py

sky/resources.py

tests/test_optimizer_random_dag.py

tests/test_spot.py

infwinston · 2022-08-09T17:06:48Z

@Michaelvll Thanks a lot for detailed reviews! I've fixed all the comments and re-run/passed the smoke test. Merging this PR now.

infwinston added 5 commits July 27, 2022 21:35

Add zone support

fd732a8

Zone validation and acc validation

9296f48

Remove CLI overwrite

b246918

yapf

7f34e0a

fix

e074181

Michaelvll reviewed Jul 29, 2022

View reviewed changes

infwinston added 2 commits July 29, 2022 19:29

address comments

6e961d7

yapf

c716745

Michaelvll reviewed Aug 1, 2022

View reviewed changes

infwinston added 17 commits August 3, 2022 20:57

address comments

8ab1a40

yapf

f62a51b

fix

81efd30

fix and remove test

41ef861

fix

fa6c582

fix test

d5a9d60

fix huggingface smoke test

eb30770

handle multi-node

6b084dc

Replace ValueError with assert

38f1542

fix multi-node

d078f3a

replace sky.cloud with clouds.cloud

f9c3b45

Add invalid zone test

dffe538

add smoke test

e75d3c8

merge

ba23115

yapf

1d47d76

Support region/zone close matching

f9aa22c

Don't overwrite ray yaml

80d3189

add comments

b66ea03

infwinston added 2 commits August 4, 2022 19:53

add zone info to usage_lib

c5bd01a

update docstring

d4de404

Michaelvll self-requested a review August 5, 2022 17:05

Michaelvll reviewed Aug 5, 2022

View reviewed changes

infwinston added 8 commits August 5, 2022 14:10

roll back sky.clouds

e6a2143

refactor

b56223a

missed

eb352bf

Fix

aad6068

yapf

b377573

add zone to cli

13818b1

update test

2288374

merge

5a14b8b

Michaelvll approved these changes Aug 8, 2022

View reviewed changes

infwinston added 6 commits August 8, 2022 11:30

fix

f7369ba

Add acc_count constraint

5809202

remove unused func

15c8d93

shorten

a93359b

comments

88df397

fix

9308580

infwinston mentioned this pull request Aug 9, 2022

sky start may unexpectedly launch a new VM #1054

Closed

infwinston merged commit d8a6b3b into master Aug 9, 2022

infwinston deleted the add-zone-support branch August 9, 2022 17:42

Michaelvll mentioned this pull request Aug 15, 2022

Fix spot recovery without cloud specified #1077

Merged

WoosukKwon mentioned this pull request Aug 31, 2022

Optimizing & Provisioning Retries at the granularity of regions/zones #975

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zone support in YAML #1014

Add zone support in YAML #1014

infwinston commented Jul 28, 2022 •

edited

Loading

concretevitamin commented Jul 29, 2022

Michaelvll left a comment •

edited

Loading

Michaelvll Jul 29, 2022

infwinston Jul 30, 2022

Michaelvll Aug 1, 2022

infwinston Aug 4, 2022

Michaelvll left a comment

Michaelvll Aug 1, 2022

infwinston Aug 4, 2022

infwinston commented Aug 1, 2022 •

edited

Loading

infwinston commented Aug 5, 2022

Michaelvll left a comment

Michaelvll Aug 5, 2022

Michaelvll Aug 5, 2022

infwinston Aug 6, 2022

Michaelvll Aug 8, 2022 •

edited

Loading

infwinston Aug 9, 2022

infwinston Aug 9, 2022

infwinston Aug 9, 2022

Michaelvll left a comment

infwinston commented Aug 9, 2022

	if (task_resources.region is not None and
	task_resources.region != launched_resources.region):
	with ux_utils.print_exception_no_traceback():
	raise exceptions.ResourcesMismatchError(
	'Task requested resources in region '
	f'{task_resources.region!r}, but the existing cluster '
	f'is in region {launched_resources.region!r}.')

	def _update_cluster_region(self):
	if self.launched_resources.region is not None:
	return

	config = common_utils.read_yaml(self.cluster_yaml)
	provider = config['provider']
	cloud = self.launched_resources.cloud
	if cloud.is_same_cloud(sky.Azure()):
	region = provider['location']
	elif cloud.is_same_cloud(sky.GCP()) or cloud.is_same_cloud(
	sky.AWS()):
	region = provider['region']
	elif cloud.is_same_cloud(sky.Local()):
	# There is only 1 region for Local cluster, 'Local'.
	local_regions = clouds.Local.regions()
	region = local_regions[0].name

	self.launched_resources = self.launched_resources.copy(
	region=region)

Add zone support in YAML #1014

Add zone support in YAML #1014

Conversation

infwinston commented Jul 28, 2022 • edited Loading

concretevitamin commented Jul 29, 2022

Michaelvll left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infwinston commented Aug 1, 2022 • edited Loading

infwinston commented Aug 5, 2022

Michaelvll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvll left a comment

Choose a reason for hiding this comment

infwinston commented Aug 9, 2022

infwinston commented Jul 28, 2022 •

edited

Loading

Michaelvll left a comment •

edited

Loading

infwinston commented Aug 1, 2022 •

edited

Loading

Michaelvll Aug 8, 2022 •

edited

Loading