[Core] Support choosing cloud for Spot controller #3363

cblmemo · 2024-03-25T07:20:40Z

Addresses the new comment under #3231.

Tested (run the relevant ones):

Code formatting: bash format.sh
Any manual or new tests for this PR (please specify below)
- pytest tests/unit_tests/test_controller_utils.py with [Core][BugFix] Remove extra field _require_fuse in Resources.to_yaml_config() #3457
- The following YAML and check cloud resources is in {AWS, GCP}/{AWS, GCP, Azure}

service:
  readiness_probe: /
  replicas: 1

resources:
  ports: 8000
  any_of:
    - cloud: aws
    - cloud: gcp
    - cloud: kubernetes

run: python -m http.server 8000

resources:
  any_of:
    - cloud: aws
    - cloud: gcp
run: echo "Hello, World!"
---
resources:
  any_of:
    - cloud: aws
    - cloud: runpod
run: echo "Hello, World!"
---
resources:
  any_of:
    - cloud: azure
    - cloud: gcp
    - cloud: kubernetes
run: echo "Hello, World!"

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

Michaelvll

Thanks for updating the behavior for the controller resources selection @cblmemo! Left several comments.

sky/serve/core.py

Michaelvll · 2024-03-25T16:16:47Z

sky/utils/controller_utils.py

+    # If the controller and replicas are from the same cloud, it should
+    # provide better connectivity. We will let the controller choose from
+    # the clouds of the resources if the controller does not exist.
+    # TODO(tian): Consider respecting the regions/zones specified for the
+    # resources as well.
+    requested_clouds: Set['clouds.Cloud'] = set()
+    for res in task_resources:
+        # cloud is an object and will not be able to be distinguished by set.
+        # Here we manually check if the cloud is in the set.
+        if res.cloud is not None and not clouds.cloud_in_iterable(
+                res.cloud, requested_clouds):
+            requested_clouds.add(res.cloud)
+    if not requested_clouds:
+        return {controller_resources_to_use}
+    return {
+        controller_resources_to_use.copy(cloud=controller_cloud)
+        for controller_cloud in requested_clouds
+    }


If the service is on Fluidstack, what will happen? Does that mean we will have a controller with GPU? This is not expected. Maybe we should blacklist some clouds that only has GPU instances.

Sounds good! Could you help double-check if the list is comprehensive? Currently I only know RunPod, FluidStack and Lambda

Michaelvll · 2024-03-25T16:19:38Z

sky/utils/controller_utils.py

+    controller_exist = (global_user_state.get_cluster_from_name(controller_name)
+                        is not None)
+    if controller_exist or controller_resources_to_use.cloud is not None:
+        return {controller_resources_to_use}


Why we don't have such check in this previous function? Does that mean a custom resource will cause an existing controller fail?

We do have such a check. Here I did a refactor...

skypilot/sky/serve/core.py

Lines 177 to 181 in 99d0aff

if (not controller_exist and

controller_resources_in_config.cloud is None):

controller_clouds = requested_clouds

else:

controller_clouds = {controller_resources_in_config.cloud}

Michaelvll · 2024-03-31T23:34:06Z

sky/utils/controller_utils.py

+        if res.cloud is not None and not clouds.cloud_in_iterable(
+                res.cloud, requested_clouds):
+            if not clouds.cloud_in_iterable(res.cloud,
+                                            _CONTROLLER_CLOUD_BLACKLIST):


In #3377, we adds the HOST_CONTROLLER implementation feature in the Cloud to filter out the clouds that do not suitable for hosting controllers. We might want to do similar thing, instead of having another list for the black list:

skypilot/sky/clouds/cloud.py

Line 44 in 87330bd

HOST_CONTROLLERS = 'host_controllers'

skypilot/sky/clouds/kubernetes.py

Lines 105 to 106 in 87330bd

unsupported_features[

clouds.CloudImplementationFeatures.HOST_CONTROLLERS] = message

Sounds good! Seems like #3377 is still under reviewing. Should we extract those part into a separate PR and merge it first? cc @romilbhardwaj for a look here

sounds good @cblmemo - I'll create a separate PR for CloudImplementationFeatures.HOST_CONTROLLERS

cblmemo · 2024-04-17T06:46:42Z

@Michaelvll bump for review when you got time 👀

Michaelvll

Thanks for updating this @cblmemo! Left several comments : )

sky/serve/core.py

sky/utils/controller_utils.py

tests/unit_tests/test_controller_utils.py

Michaelvll

Thanks for the update @cblmemo! It seems the PR is ready to go. Could you test the case where both spot and serve controller do not exist?

Michaelvll · 2024-04-23T02:35:00Z

sky/utils/controller_utils.py

+    controller_resources_to_use: resources.Resources = list(
+        controller_resources)[0]
+
+    controller_exist = (global_user_state.get_cluster_from_name(
+        controller.value.name) is not None)
+    if controller_exist or controller_resources_to_use.cloud is not None:
+        return {controller_resources_to_use}


If we are returning a set of resources anyway, why do we only return the first one but not the whole controller_resources, i.e. allowing a user to specify multiple resources for the controller? (If needed, we can add a TODO here)

Also, just wondering, do we have some places depending on the results of the resources returned by this function to decide how many services we can run on the controller? In that case, will changing this to a set cause failure?

why not allowing a user to specify multiple resources for the controller?

That is a good point! I think it is at least feasible to support set of resources (i.e. any_of). One thing we need to discuss is what is the behaviour if the controller specified ordered resources w/o cloud specified and we want to override it with task resources cloud? Consider the following:

# ~/.sky/config.yaml serve: controller: resources: ordered: - accelerators: L4 - accelerators: T4

# service.yaml resources: any_of: - cloud: aws - cloud: gcp

Should the controller resoruces be a list?

If it is a list, what is the order of aws and gcp?

If it is a set, does it break the semantic of ordered in controller resources?

do we have some places depending on the results of the resources returned by this function to decide how many services we can run on the controller?

No, this is automatically calculated from system memory. Reference here:

skypilot/sky/serve/serve_utils.py

Lines 43 to 45 in 48a5c63

_SYSTEM_MEMORY_GB = psutil.virtual_memory().total // (1024**3)

NUM_SERVICE_THRESHOLD = (_SYSTEM_MEMORY_GB //

constants.CONTROLLER_MEMORY_USAGE_GB)

Added a TODO first. Thanks!

Michaelvll · 2024-04-23T02:36:11Z

tests/unit_tests/test_controller_utils.py

+def test_get_controller_resources(controller_type,
+                                  custom_controller_resources_config, expected,
+                                  enable_all_clouds, monkeypatch):


Could we add type hints?

Added hint except enable_all_clouds and monkeypatch. Do we need to add type annotations for those fixture as well?

Michaelvll · 2024-04-23T02:43:51Z

tests/unit_tests/test_controller_utils.py

+    ('spot', spot_constants.CONTROLLER_RESOURCES),
+    ('serve', serve_constants.CONTROLLER_RESOURCES),
+])
+def test_get_controller_resources_with_task_resources(


cblmemo · 2024-04-23T03:10:09Z

Thanks for the update @cblmemo! It seems the PR is ready to go. Could you test the case where both spot and serve controller do not exist?

Re-tested the YAMLs in the PR description with no controller exists and it works well 🫡

init

245848e

cblmemo mentioned this pull request Mar 25, 2024

[Serve] Make controller cloud choose from replica resources if it is not up #3231

Merged

5 tasks

cblmemo added 2 commits March 25, 2024 00:27

fix unittest

25ea7b3

fix unittest

7ab4b7c

Michaelvll reviewed Mar 25, 2024

View reviewed changes

cblmemo added 2 commits March 26, 2024 01:22

infer args from type

c5069a7

blacklist for gpu cloud

55d7287

cblmemo requested a review from Michaelvll March 31, 2024 04:33

Michaelvll reviewed Mar 31, 2024

View reviewed changes

romilbhardwaj mentioned this pull request Apr 2, 2024

[Core] Add HOST_CONTROLLERS check for clouds #3407

Merged

2 tasks

cblmemo added 2 commits April 2, 2024 21:16

Merge remote-tracking branch 'origin/master' into fix-controller-cloud

755abab

use host controllers

64f7c09

cblmemo requested a review from Michaelvll April 7, 2024 02:51

Merge remote-tracking branch 'origin/master' into fix-controller-cloud

c8319b6

Michaelvll reviewed Apr 21, 2024

View reviewed changes

sky/serve/core.py Outdated Show resolved Hide resolved

sky/utils/controller_utils.py Outdated Show resolved Hide resolved

sky/utils/controller_utils.py Outdated Show resolved Hide resolved

tests/unit_tests/test_controller_utils.py Outdated Show resolved Hide resolved

cblmemo added 5 commits April 22, 2024 00:49

apply suggestions from code review

a017063

Merge remote-tracking branch 'origin/master' into fix-controller-cloud

42475ef

format

53d11ca

fix

b677509

Merge branch 'fix-extra-required-fuse' into fix-controller-cloud

c22beef

cblmemo mentioned this pull request Apr 23, 2024

[Dependency] Fix the lazy import failure for OCI #3463

Merged

6 tasks

Michaelvll approved these changes Apr 23, 2024

View reviewed changes

Michaelvll linked an issue Apr 23, 2024 that may be closed by this pull request

[Serve] Fail to launch controller when no resources is specified for a service when firstly used #3462

Closed

cblmemo added 2 commits April 22, 2024 20:05

type annotation

05226a1

todo

b9ccaa2

Merge remote-tracking branch 'origin/master' into fix-controller-cloud

9cddf93

cblmemo merged commit cc1c58b into master Apr 23, 2024
20 checks passed

cblmemo deleted the fix-controller-cloud branch April 23, 2024 03:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Support choosing cloud for Spot controller #3363

[Core] Support choosing cloud for Spot controller #3363

cblmemo commented Mar 25, 2024 •

edited

Loading

Michaelvll left a comment

Michaelvll Mar 25, 2024

cblmemo Mar 26, 2024

Michaelvll Mar 25, 2024

cblmemo Mar 26, 2024

Michaelvll Mar 31, 2024

cblmemo Apr 1, 2024

romilbhardwaj Apr 2, 2024

cblmemo commented Apr 17, 2024

Michaelvll left a comment

Michaelvll left a comment

Michaelvll Apr 23, 2024

cblmemo Apr 23, 2024

cblmemo Apr 23, 2024

Michaelvll Apr 23, 2024

cblmemo Apr 23, 2024

Michaelvll Apr 23, 2024

cblmemo commented Apr 23, 2024

	if (not controller_exist and
	controller_resources_in_config.cloud is None):
	controller_clouds = requested_clouds
	else:
	controller_clouds = {controller_resources_in_config.cloud}

	unsupported_features[
	clouds.CloudImplementationFeatures.HOST_CONTROLLERS] = message

	_SYSTEM_MEMORY_GB = psutil.virtual_memory().total // (1024**3)
	NUM_SERVICE_THRESHOLD = (_SYSTEM_MEMORY_GB //
	constants.CONTROLLER_MEMORY_USAGE_GB)

[Core] Support choosing cloud for Spot controller #3363

[Core] Support choosing cloud for Spot controller #3363

Conversation

cblmemo commented Mar 25, 2024 • edited Loading

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

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

cblmemo commented Apr 17, 2024

Michaelvll left a 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

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

cblmemo commented Apr 23, 2024

cblmemo commented Mar 25, 2024 •

edited

Loading