Skip to content

DB deployment support for cluster IAM role #1

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.

Sign up for GitHub

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

Open
dbczumar wants to merge 8 commits into
base: master
Choose a base branch
from
Open

DB deployment support for cluster IAM role #1

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2801d26
DB deployment support for cluster IAM role
dbczumar Jun 26, 2018
435adc0
fix
dbczumar Jun 26, 2018
463e251
Fix region name
dbczumar Jun 26, 2018
8da00a6
use assumed role ARN if none specified
dbczumar Jun 29, 2018
9af3669
Container env fix
dbczumar Jun 29, 2018
888c066
Add region to default bucket name
dbczumar Jun 29, 2018
c2343b8
print >> eprint
dbczumar Jun 29, 2018
7ead7a4
Address comments
dbczumar Jun 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions mlflow/sagemaker/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
PYFUNC_IMAGE_URL = "707343435239.dkr.ecr.us-west-2.amazonaws.com/mlflow-pyfunc-test:latest"

@sueann sueann Jun 27, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: we want to change this to the databricks prod account in submitting the PR to mlflow master

@tomasatdatabricks tomasatdatabricks Jun 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should tag the image with the current mlflow version instead of latest.

@sueann sueann Jun 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to clarify, so for master, we'd make it the version of the next release, right? is that 0.2.2 now? @tomasatdatabricks

@tomasatdatabricks tomasatdatabricks Jul 5, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make most sense for this to point to the current version of MLflow - i.e. mlflow.version.VERSION. That way we do not have to be managing updating versions on multiple places.

DEFAULT_IMAGE_URL = PYFUNC_IMAGE_URL

DEFAULT_BUCKET_NAME_PREFIX = "mlflow-sagemaker"
DEFAULT_BUCKET_NAME_PREFIX = "mlflow-sagemaker"

_DOCKERFILE_TEMPLATE = """
# Build an image that can serve pyfunc model in SageMaker
Expand Down Expand Up @@ -158,13 +158,16 @@ def deploy(app_name, model_path, bucket=None, image_url=DEFAULT_IMAGE_URL, run_i
if run_id:
model_path = _get_model_log_dir(model_path, run_id)
prefix = os.path.join(run_id, prefix)
run_id = _check_compatible(model_path)
run_id = _check_compatible(model_path)

@sueann sueann Jun 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changes the behavior ? was it incorrect before?

@sueann sueann Jun 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomasatdatabricks

@sueann sueann Jun 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, was viewing a subset of changes - this was changing it back to the original logic


if bucket is None:
# Attempt to create a default bucket
eprint("No model data bucket specified, using the default bucket")
bucket = _get_default_s3_bucket(region_name)

if execution_role_arn is None:
execution_role_arn = _get_assumed_role_arn()

model_s3_path = _upload_s3(local_model_path=model_path, bucket=bucket, prefix=prefix)
_deploy(role=execution_role_arn,
image_url=image_url,
Expand Down Expand Up @@ -244,24 +247,25 @@ def _get_assumed_role_arn():

def _get_default_s3_bucket(region_name):
# create bucket if it does not exist
account_id = _get_account_id()
bucket_name = "{pfx}-{aid}".format(pfx=DEFAULT_BUCKET_NAME_PREFIX, aid=account_id)
sess = boto3.Session()
account_id = _get_account_id()
region_name = sess.region_name or "us-west-2"
bucket_name = "{pfx}-{rn}-{aid}".format(pfx=DEFAULT_BUCKET_NAME_PREFIX, rn=region_name, aid=account_id)
s3 = sess.client('s3')
response = s3.list_buckets()
buckets = [b['Name'] for b in response["Buckets"]]
if not bucket_name in buckets:
print("Default bucket `%s` not found. Creating..." % bucket_name)
eprint("Default bucket `%s` not found. Creating..." % bucket_name)
response = s3.create_bucket(
ACL='bucket-owner-full-control',
Bucket=bucket_name,
CreateBucketConfiguration={
'LocationConstraint': region_name,
},
)
print(response)
eprint(response)
else:
print("Default bucket `%s` already exists. Skipping creation." % bucket_name)
eprint("Default bucket `%s` already exists. Skipping creation." % bucket_name)
return bucket_name

def _upload_s3(local_model_path, bucket, prefix):
Expand All @@ -287,7 +291,7 @@ def _upload_s3(local_model_path, bucket, prefix):
Tagging={'TagSet': [{'Key': 'SageMaker', 'Value': 'true'}, ]}
)
eprint('tag response', response)
return os.path.join(s3.meta.endpoint_url, bucket, key)
return "/".join(map(lambda x: str(x).rstrip('/'), [s3.meta.endpoint_url, bucket, key]))

def _deploy(role, image_url, app_name, model_s3_path, run_id, region_name):
"""
Expand Down
3 changes: 3 additions & 0 deletions mlflow/sagemaker/container/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ def _serve():
print("activating custom environment")
env = conf[pyfunc.ENV]
env_path_dst = os.path.join("/opt/mlflow/", env)
env_path_dst_dir = os.path.dirname(env_path_dst)

@sueann sueann Jul 4, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tomasatdatabricks do you know why this might have been added?

@dbczumar dbczumar Jul 4, 2018

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we were attempting to copy files to a destination directory that may not exist; ran into an error during copying due to this. There may be an easier way to force directory creation by adding a flag to the copy command.

@tomasatdatabricks tomasatdatabricks Jul 5, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it looks reasonable to me. Btw, I fix this in my pending PR too so we might need to merge it at some point.

if not os.path.exists(env_path_dst_dir):
os.makedirs(env_path_dst_dir)
# /opt/ml/ is read-only, we need to copy the env elsewhere before importing it
shutil.copy(src=os.path.join("/opt/ml/model/", env), dst=env_path_dst)
os.system("conda env create -n custom_env -f {}".format(env_path_dst))
Expand Down