Skip to content

Commit

Permalink
[UX] Optimizing error message when disabled cloud storage attempted (#…
Browse files Browse the repository at this point in the history
…1858)

* ux_optimization_disabled_clouds

* support get_enabled_clouds_str()

* fix return type

* adding new line eof

* fix error messages

* update for non-cloud-storage

* update check.py, cloudflare.py

* ux_optimization_disabled_clouds

* support get_enabled_clouds_str()

* fix return type

* adding new line eof

* fix error messages

* update for non-cloud-storage

* update with enabled_storage_clouds

* update STORE_ENABLED_CLOUDS to List[str]
  • Loading branch information
landscapepainter authored May 25, 2023
1 parent 3995bae commit 4849c86
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 9 deletions.
1 change: 1 addition & 0 deletions sky/adaptors/cloudflare.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
AWS_R2_PROFILE_PATH = '~/.aws/credentials'
R2_PROFILE_NAME = 'r2'
_INDENT_PREFIX = ' '
NAME = 'Cloudflare'


def import_package(func):
Expand Down
36 changes: 35 additions & 1 deletion sky/data/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
# Storage isn't supported yet (even though Azure is).
# TODO(Doyoung): need to add clouds.CLOUDFLARE() to support
# R2 to be an option as preferred store type
STORE_ENABLED_CLOUDS = [clouds.AWS(), clouds.GCP()]
STORE_ENABLED_CLOUDS: List[str] = [
str(clouds.AWS()), str(clouds.GCP()), cloudflare.NAME
]

# Maximum number of concurrent rsync upload processes
_MAX_CONCURRENT_UPLOADS = 32
Expand Down Expand Up @@ -912,6 +914,17 @@ def _validate(self):
# Validate name
self.name = self.validate_name(self.name)

# Check if the storage is enabled
enabled_storage_clouds = global_user_state.get_enabled_storage_clouds()
if str(clouds.AWS()) not in enabled_storage_clouds:
with ux_utils.print_exception_no_traceback():
raise exceptions.ResourcesUnavailableError(
'Storage \'store: s3\' specified, but ' \
'AWS access is disabled. To fix, enable '\
'AWS by running `sky check`. More info: '\
'https://skypilot.readthedocs.io/en/latest/getting-started/installation.html.' # pylint: disable=line-too-long
)

@classmethod
def validate_name(cls, name) -> str:
"""Validates the name of the S3 store.
Expand Down Expand Up @@ -1287,6 +1300,16 @@ def _validate(self):
'R2 Bucket should exist.')
# Validate name
self.name = self.validate_name(self.name)
# Check if the storage is enabled
enabled_storage_clouds = global_user_state.get_enabled_storage_clouds()
if str(clouds.GCP()) not in enabled_storage_clouds:
with ux_utils.print_exception_no_traceback():
raise exceptions.ResourcesUnavailableError(
'Storage \'store: gcs\' specified, but ' \
'GCP access is disabled. To fix, enable '\
'GCP by running `sky check`. '\
'More info: https://skypilot.readthedocs.io/en/latest/getting-started/installation.html.' # pylint: disable=line-too-long
)

@classmethod
def validate_name(cls, name) -> str:
Expand Down Expand Up @@ -1689,7 +1712,18 @@ def _validate(self):
assert self.name == data_utils.split_r2_path(self.source)[0], (
'R2 Bucket is specified as path, the name should be '
'the same as R2 bucket.')
# Validate name
self.name = S3Store.validate_name(self.name)
# Check if the storage is enabled
enabled_storage_clouds = global_user_state.get_enabled_storage_clouds()
if cloudflare.NAME not in enabled_storage_clouds:
with ux_utils.print_exception_no_traceback():
raise exceptions.ResourcesUnavailableError(
'Storage \'store: r2\' specified, but ' \
'Cloudflare R2 access is disabled. To fix, '\
'enable Cloudflare R2 by running `sky check`. '\
'More info: https://skypilot.readthedocs.io/en/latest/getting-started/installation.html.' # pylint: disable=line-too-long
)

def initialize(self):
"""Initializes the R2 store object on the cloud.
Expand Down
20 changes: 19 additions & 1 deletion sky/global_user_state.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
import colorama

from sky import clouds
from sky.utils import db_utils
from sky.adaptors import cloudflare
from sky.data import storage as storage_lib
from sky.utils import common_utils
from sky.utils import db_utils

if typing.TYPE_CHECKING:
from sky import backends
Expand Down Expand Up @@ -663,6 +665,22 @@ def get_enabled_clouds() -> List[clouds.Cloud]:
return enabled_clouds


def get_enabled_storage_clouds() -> List[str]:
# This is a temporary solution until https://github.com/skypilot-org/skypilot/issues/1943 # pylint: disable=line-too-long
# is resolved by implementing separate 'enabled_storage_clouds'
enabled_clouds = get_enabled_clouds()
enabled_clouds = [str(cloud) for cloud in enabled_clouds]

enabled_storage_clouds = [
cloud for cloud in enabled_clouds
if cloud in storage_lib.STORE_ENABLED_CLOUDS
]
r2_is_enabled, _ = cloudflare.check_credentials()
if r2_is_enabled:
enabled_storage_clouds.append(cloudflare.NAME)
return enabled_storage_clouds


def set_enabled_clouds(enabled_clouds: List[str]) -> None:
_DB.cursor.execute('INSERT OR REPLACE INTO config VALUES (?, ?)',
(_ENABLED_CLOUDS_KEY, json.dumps(enabled_clouds)))
Expand Down
11 changes: 4 additions & 7 deletions sky/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -686,13 +686,10 @@ def get_preferred_store_type(self) -> storage_lib.StoreType:
if storage_cloud is None:
# Get the first enabled cloud.
backend_utils.check_public_cloud_enabled()
enabled_clouds = global_user_state.get_enabled_clouds()

for cloud in storage_lib.STORE_ENABLED_CLOUDS:
for enabled_cloud in enabled_clouds:
if cloud.is_same_cloud(enabled_cloud):
storage_cloud = cloud
break
enabled_storage_clouds = \
global_user_state.get_enabled_storage_clouds()
if enabled_storage_clouds:
storage_cloud = enabled_storage_clouds[0]
if storage_cloud is None:
raise ValueError('No available cloud to mount storage.')
store_type = storage_lib.get_storetype_from_cloud(storage_cloud)
Expand Down

0 comments on commit 4849c86

Please sign in to comment.