fix: enable kms support for repack_model #1061

icywang86rui · 2019-09-23T23:44:26Z

Currently repack_model doesn't accept a kms key. This change
added a kms_key argument to the fucntion. In addition repack_model
will always use the output_kms_key inside the Estimator if it's set.

Issue #, if available:

Description of changes:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated any necessary documentation (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Currently repack_model doesn't accept a kms key. This change added a kms_key argument to the fucntion. In addition repack_model will always use the output_kms_key inside the Estimator if it's set.

sagemaker-bot · 2019-09-23T23:47:08Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu · 2019-09-23T23:50:09Z

src/sagemaker/chainer/estimator.py

@@ -162,6 +162,7 @@ def create_model(
        entry_point=None,
        source_dir=None,
        dependencies=None,
+        **kwargs


is there something here that should actually get used?

I am only fixing TensorFlow and SKLearn for now. I think all other frameworks needs more fixes than this one(similar to the fixes I added to sklearn). That work should be properly scheduled and we should add integ tests as well.

laurenyu · 2019-09-23T23:52:38Z

src/sagemaker/estimator.py

@@ -547,6 +547,7 @@ def deploy(
                )
            model = self._compiled_models[family]
        else:
+            kwargs["output_kms_key"] = self.output_kms_key


I realize that I'm probably the source of this problem (with one of my recent code changes), but should we distinguish more between output_kms_key and kms_key? also, is there a case where someone might want to specify output_kms_key in deploy?

When we repack the model the new model tar ball is uploaded to the same bucket. we need to use the same output_kms_key in the estimator, otherwise uploading the repacked tar ball will fail.

I can't really think of any use case where you would want a different kms key since we don't allow user to specify a new bucket when deploy is called.

maybe let's rename the Model's output_kms_key to something like model_data_kms_key? (model_kms_key?)

i will do model_kms_key

laurenyu · 2019-09-23T23:53:03Z

src/sagemaker/model.py

@@ -79,6 +79,7 @@ def __init__(
        vpc_config=None,
        sagemaker_session=None,
        enable_network_isolation=False,
+        output_kms_key=None,


docstring? (same for the rest)

**kwargs also need docstring entries - if possible, include a link to where the **kwargs are eventually sent to

sagemaker-bot · 2019-09-24T00:03:04Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T00:09:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T00:54:15Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T03:53:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T04:10:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T04:23:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T04:30:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T04:46:43Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu · 2019-09-24T16:08:39Z

src/sagemaker/estimator.py

@@ -547,6 +547,7 @@ def deploy(
                )
            model = self._compiled_models[family]
        else:
+            kwargs["output_kms_key"] = self.output_kms_key


maybe let's rename the Model's output_kms_key to something like model_data_kms_key? (model_kms_key?)

laurenyu · 2019-09-24T16:09:35Z

src/sagemaker/model.py

@@ -114,6 +115,8 @@ def __init__(
                network isolation in the endpoint, isolating the model
                container. No inbound or outbound network calls can be made to
                or from the model container.
+            output_kms_key (str): KMS key ARN, it's only used to encrypt the
+                repacked model archive file.


"KMS key ARN used to encrypt the repacked model archive file if the model is repacked"

laurenyu · 2019-09-24T16:10:37Z

src/sagemaker/model.py

@@ -79,6 +79,7 @@ def __init__(
        vpc_config=None,
        sagemaker_session=None,
        enable_network_isolation=False,
+        output_kms_key=None,


**kwargs also need docstring entries - if possible, include a link to where the **kwargs are eventually sent to

sagemaker-bot · 2019-09-24T17:33:37Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T17:50:49Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T17:53:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T17:56:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T18:06:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T18:13:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T18:52:37Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T21:54:55Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T22:15:55Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T22:17:49Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T22:59:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T23:06:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-24T23:51:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T03:42:52Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T03:42:53Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T03:43:11Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T03:59:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:06:59Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:20:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:29:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:32:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:40:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T17:50:37Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T18:04:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T18:10:27Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T18:49:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T19:32:56Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T19:43:44Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T19:52:14Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T20:04:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T20:04:21Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T20:04:40Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-25T20:09:24Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

fix: enable kms support for repack_model

dc07547

Currently repack_model doesn't accept a kms key. This change added a kms_key argument to the fucntion. In addition repack_model will always use the output_kms_key inside the Estimator if it's set.

icywang86rui requested review from ajaykarpur, laurenyu and knakad September 23, 2019 23:44

laurenyu reviewed Sep 23, 2019

View reviewed changes

fix docstring

92814f7

laurenyu reviewed Sep 24, 2019

View reviewed changes

changes based on cr feedbacks

60cdfa1

fix pylint error

392216a

Merge branch 'master' into fix-repack-model

1a19e9f

fix transformer

5c7b4e8

fix transformer

45ec7d5

black format

0cb4d9a

Merge branch 'master' into fix-repack-model

e77701b

laurenyu approved these changes Sep 25, 2019

View reviewed changes

Merge branch 'master' into fix-repack-model

4bcb042

Merge branch 'master' into fix-repack-model

f992f2c

icywang86rui merged commit 76d46d0 into aws:master Sep 25, 2019

fix: enable kms support for repack_model #1061

fix: enable kms support for repack_model #1061

Uh oh!

Conversation

icywang86rui commented Sep 23, 2019

Merge Checklist

Uh oh!

sagemaker-bot commented Sep 23, 2019

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Sep 24, 2019

AWS CodeBuild CI Report

Uh oh!