Skip to content
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

Jump to bottom

Added region name to the sagemaker deploy cli. #24

Merged
merged 1 commit into from Jun 16, 2018
Merged

Added region name to the sagemaker deploy cli. #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions mlflow/sagemaker/cli.py
View file
Open in desktop
Expand Up @@ -28,7 +28,8 @@ def commands():
@click.option("--bucket", "-b", help="S3 bucket to store model artifacts", required=True)
@click.option("--run_id", "-r", default=None, help="Run id")
@click.option("--container", "-c", default="mlflow_sage", help="container name")
def deploy(app_name, model_path, execution_role_arn, bucket, run_id=None, container="mlflow_sage"): # noqa
@click.option("--region-name", default="us-west-2", help="region name")
def deploy(app_name, model_path, execution_role_arn, bucket, run_id=None, container="mlflow_sage", region_name="us-west-2"): # noqa
""" Deploy model on sagemaker.

:param app_name: Name of the deployed app.
Expand All @@ -51,7 +52,8 @@ def deploy(app_name, model_path, execution_role_arn, bucket, run_id=None, contai
container_name=container,
app_name=app_name,
model_s3_path=model_s3_path,
run_id=run_id)
run_id=run_id,
region_name=region_name)


@commands.command("run-local")
Expand Down Expand Up @@ -130,4 +132,4 @@ def _check_compatible(path):
path = os.path.abspath(path)
servable = Model.load(os.path.join(path, "MLmodel"))
if pyfunc.FLAVOR_NAME not in servable.flavors:
raise Exception("Currenlty only supports pyfunc format.")
raise Exception("Currenlty only supports pyfunc format.")
4 changes: 2 additions & 2 deletions mlflow/sagemaker/deploy.py
View file
Open in desktop
Expand Up @@ -47,7 +47,7 @@ def _upload_s3(local_model_path, bucket, prefix):
shutil.rmtree(tmp_dir)


def _deploy(role, container_name, app_name, model_s3_path, run_id):
def _deploy(role, container_name, app_name, model_s3_path, run_id, region_name):
"""
Deploy model on sagemaker.
:param role:
Expand All @@ -57,7 +57,7 @@ def _deploy(role, container_name, app_name, model_s3_path, run_id):
:param run_id:
:return:
"""
sage_client = boto3.client('sagemaker', region_name="us-west-2")
sage_client = boto3.client('sagemaker', region_name)
Copy link
Contributor

@aarondav aarondav Jun 11, 2018 •
edited

Choose a reason for hiding this comment

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

Are you sure the second argument is region_name? This documentation would seem to indicate that everything but the name is a **kwargs, so it would need to be named -- i.e., region_name= region_name.

Copy link
Contributor

@aarondav aarondav Jun 11, 2018 •
edited

Choose a reason for hiding this comment

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

Ah -- I see it now, it is actually the second argument from here. Since it has a default value of None, would you mind adding the explicit region_name= region_name anyway, just to be 100% obvious?

ecr_client = boto3.client("ecr")
repository_conf = ecr_client.describe_repositories(
repositoryNames=[container_name])['repositories'][0]
Expand Down