-
Notifications
You must be signed in to change notification settings - Fork 470
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
Sky init and cloud credentials for storage #102
Changes from 17 commits
3627ba1
6bf331d
5722950
0055be5
9c0d9de
716893f
d32c8e8
cbb1ac7
0feca9c
b0c98e1
f2c28e3
20bd57f
12e7069
0d8c5f5
d40de79
e8035a3
8fe4f82
7e839b7
6d25abd
b43dfd5
d190562
7386753
f13fc34
a4630b1
122625b
6c85f87
81c9c4b
d468056
90744db
0f14428
b10dcbe
ccbad66
f124d3e
096d0c8
80546a9
a0e78f6
b7afe19
737c6f4
7f8de10
5828176
6df4934
c690de1
15dcc9b
9bbfd30
9319dd2
a687e51
5013f5b
285e59b
e46027f
88f92df
33fe806
9a1d8ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,6 @@ tabulate | |
docker | ||
|
||
awscli==1.22.17 | ||
azure-cli | ||
azure-cli==2.22.0 | ||
google-api-python-client | ||
google-cloud-storage |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -981,17 +981,17 @@ def sync_file_mounts( | |
else: | ||
wrapped_dst = backend_utils.FileMountHelper.wrap_file_mount(dst) | ||
storage = cloud_stores.get_storage_from_path(src) | ||
if storage.is_directory(src): | ||
sync = storage.make_sync_dir_command(source=src, | ||
destination=wrapped_dst) | ||
# It is a directory so make sure it exists. | ||
mkdir_for_wrapped_dst = f'mkdir -p {wrapped_dst}' | ||
else: | ||
if storage.is_file(src): | ||
sync = storage.make_sync_file_command(source=src, | ||
destination=wrapped_dst) | ||
# It is a file so make sure *its parent dir* exists. | ||
mkdir_for_wrapped_dst = \ | ||
f'mkdir -p {os.path.dirname(wrapped_dst)}' | ||
else: | ||
sync = storage.make_sync_dir_command(source=src, | ||
destination=wrapped_dst) | ||
# It is a directory so make sure it exists. | ||
mkdir_for_wrapped_dst = f'mkdir -p {wrapped_dst}' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you test a src that doesn't exist, which will hit this branch:
There probably will be some error; is it reasonable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before, this will hit an error while doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @franklsf95 Did you actually run the requested test? I requested it because it does not look trivial. It is not okay / equivalent to before, because with this change, we would have created zombie dirs from L994 |
||
download_target_commands = [ | ||
# Ensure sync can write to wrapped_dst (e.g., '/data/'). | ||
mkdir_for_wrapped_dst, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,18 @@ | ||
"""Azure.""" | ||
import copy | ||
import json | ||
import subprocess | ||
from typing import Dict, Iterator, List, Optional, Tuple | ||
|
||
from sky import clouds | ||
from sky.clouds.service_catalog import azure_catalog | ||
|
||
|
||
def _run_output(cmd): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, move to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any clues on how to fix this circular import? @franklsf95 How did Ray solve this type of issue? |
||
proc = subprocess.run(cmd, shell=True, check=True, stdout=subprocess.PIPE) | ||
return proc.stdout.decode('ascii') | ||
|
||
|
||
class Azure(clouds.Cloud): | ||
"""Azure.""" | ||
|
||
|
@@ -128,3 +134,13 @@ def _make(instance_type): | |
if instance_type is None: | ||
return [] | ||
return _make(instance_type) | ||
|
||
def check_credentials(self) -> Tuple[bool, Optional[str]]: | ||
"""Checks if the user has access credentials to this cloud.""" | ||
try: | ||
output = _run_output('az account show --output=json') | ||
except subprocess.CalledProcessError: | ||
return False, 'Azure CLI returned error.' | ||
if output.startswith('{'): | ||
return True, None | ||
return False, 'Azure credentials not set. Run `az login`.' | ||
franklsf95 marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
"""Google Cloud Platform.""" | ||
import copy | ||
import json | ||
import os | ||
from typing import Dict, Iterator, List, Optional, Tuple | ||
|
||
import google.auth | ||
franklsf95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
from sky import clouds | ||
|
||
|
||
|
@@ -276,3 +279,17 @@ def get_accelerators_from_instance_type( | |
instance_type: str, | ||
) -> Optional[Dict[str, int]]: | ||
return None | ||
|
||
def check_credentials(self) -> Tuple[bool, Optional[str]]: | ||
"""Checks if the user has access credentials to this cloud.""" | ||
try: | ||
for file in [ | ||
'~/.config/gcloud/access_tokens.db', | ||
'~/.config/gcloud/credentials.db' | ||
]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check the |
||
assert os.path.isfile(os.path.expanduser(file)) | ||
google.auth.default() | ||
franklsf95 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
except (AssertionError, google.auth.exceptions.DefaultCredentialsError): | ||
return False, ('GCP credentials not set.' | ||
' Run `gcloud auth application-default login`.') | ||
return True, None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of this sentence?