Add APIs to export transform and deploy config #497

yangaws · 2018-11-16T23:42:23Z

Issue #, if available:

Description of changes:

Add APIs to export transform config from a transformer or an estimator.
Add APIs to export deploy config from a model or an estimator.
Add related unit tests.

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 have added tests that prove my fix is effective or that my feature works (if appropriate)
I have updated the changelog with a description of my changes (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.

codecov-io · 2018-11-16T23:45:54Z

Codecov Report

Merging #497 into master will decrease coverage by <.01%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master     #497      +/-   ##
==========================================
- Coverage   94.28%   94.28%   -0.01%     
==========================================
  Files          59       59              
  Lines        4551     4603      +52     
==========================================
+ Hits         4291     4340      +49     
- Misses        260      263       +3

Impacted Files	Coverage Δ
src/sagemaker/transformer.py	`100% <ø> (ø)`	⬆️
src/sagemaker/estimator.py	`90.47% <100%> (+0.16%)`	⬆️
src/sagemaker/workflow/airflow.py	`92.09% <93.61%> (+0.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e37ac12...d1a005e. Read the comment docs.

laurenyu · 2018-11-17T00:12:35Z

src/sagemaker/workflow/airflow.py

    Args:
        model (sagemaker.model.FrameworkModel): The framework model
        instance_type (str): The EC2 instance type to deploy this Model to. For example, 'ml.p2.xlarge'.
        s3_operations (dict): The dict to specify S3 operations (upload `source_dir`).
-


these newlines should stay

laurenyu · 2018-11-17T00:13:05Z

src/sagemaker/workflow/airflow.py

-                If None, server will use one worker per vCPU. Only effective when estimator is
-                SageMaker framework.
+            If None, server will use one worker per vCPU. Only effective when estimator is
+            SageMaker framework.


laurenyu · 2018-11-17T00:13:40Z

src/sagemaker/workflow/airflow.py

@@ -394,5 +392,223 @@ def model_config_from_estimator(instance_type, estimator, role=None, image=None,
    elif isinstance(estimator, sagemaker.estimator.Framework):
        model = estimator.create_model(model_server_workers=model_server_workers, role=role,
                                       vpc_config_override=vpc_config_override)
+    else:
+        raise TypeError('Estimator must be one of BYO estimator, framework estimator or amazon algorithm'
+                        'estimator.')


it might be more helpful here to put the paths to the classes themselves, i.e. sagemaker.estimator.Estimator, etc.

laurenyu · 2018-11-17T00:14:57Z

src/sagemaker/workflow/airflow.py

+
+    if transformer.output_path is None:
+        transformer.output_path = 's3://{}/{}'.format(
+            transformer.sagemaker_session.default_bucket(), transformer._current_job_name)


is this logic that's also in Transformer.transform? if so, I wonder if it'd be worth refactoring it into a private method that you can call on transformer here (like EstimatorBase._prepare_for_training)

Yep it's similar except the naming method is different. One is airflow_name_from_base and the other is name_from_base. So if I wrap them in a method, that method needs to take a method as input arg which I don't want to do for just these small amounts of codes for now. If we apply the different methods to get name first, then there's just one line left which I guess no need to wrap. I am still targeting at what I mentioned in other comments that we could reformat the codes in session/estimator/transformer/etc in a way that we could wrap a lot codes in methods and couple airflow with them.

laurenyu · 2018-11-17T00:20:44Z

src/sagemaker/workflow/airflow.py


    return model_config(instance_type, model, role, image)
+
+
+def transform_config(transformer, data, data_type='S3Prefix', content_type=None, compression_type=None,


are these kinds of methods potentially reusable for Session? (this is sort of out of scope of this PR)

Yep I do want it to be reused in session. Now I cannot because I need to bypass all the validations/describe calls/etc to use Jinja templating.

If in session, we can construct the config first, and then apply all manipulations afterwards. That would be awesome and these two parts can be coupled. (which is really good since for now I am worried if session part got updated, for example, new entries in boto config introduced, airflow part cannot catch it)

iquintero

Just some minor comments.

iquintero · 2018-11-17T00:11:14Z

src/sagemaker/estimator.py

-        transform_env = model.env.copy()
-        if env is not None:
-            transform_env.update(env)
+        if self.latest_training_job is not None:


you can just do

if self.latest_training_job:

Yep that looks nice. But the problem is we won't get into
if var
if var is things like [], or {}. Not just None. Hence I kind of want to explicitly say the only thing I don't want is None. Especially sometimes I do have some var = {} and need to do var.update() after. If there's an if in between, I do need to deal with the {} case.
Yep we probably should do
if var
most of times and
if var is not None
only when needed. But using the second one for now is kind of consistent to what we have in the existed classes (like estimator, model, etc). I prefer not to change for now. If later when we introduce style guide and can force it everywhere, we probably could do the change for all.

iquintero · 2018-11-17T00:11:39Z

src/sagemaker/estimator.py

+            logging.warning('No finished training job found associated with this estimator. Please make sure'
+                            'this estimator is only used for building workflow config')
+            model_name = self._current_job_name
+            transform_env = env if env is not None else {}


similar here,

transform_env = env or {}

iquintero · 2018-11-17T00:12:49Z

src/sagemaker/estimator.py

+            vpc_config = model.vpc_config
+            self.sagemaker_session.create_model(model_name, role, container_def, vpc_config)
+            transform_env = model.env.copy()
+            if env is not None:


Answered above.

iquintero · 2018-11-17T00:19:27Z

src/sagemaker/workflow/airflow.py

+        transformer._current_job_name = job_name
+    else:
+        base_name = transformer.base_transform_job_name
+        transformer._current_job_name = utils.airflow_name_from_base(base_name) \


transformer._current_job_name = utils.airflow_name_from_base(base_name) if base_name else transformer.model_name

Answered above.

iquintero · 2018-11-17T00:20:27Z

src/sagemaker/workflow/airflow.py

+        'TransformResources': job_config['resource_config'],
+    }
+
+    if transformer.strategy is not None:


similar to my other comments, do all these as

if transformer.strategy:

Answered above.

iquintero · 2018-11-17T00:22:22Z

src/sagemaker/workflow/airflow.py

+    production_variant = sagemaker.production_variant(model.name, instance_type, initial_instance_count)
+    name = model.name
+    config_options = {'EndpointConfigName': name, 'ProductionVariants': [production_variant]}
+    if tags is not None:


Answered above.

iquintero · 2018-11-17T00:22:40Z

src/sagemaker/workflow/airflow.py

+
+    # if there is s3 operations needed for model, move it to root level of config
+    s3_operations = model_base_config.pop('S3Operations', None)
+    if s3_operations is not None:


Answered above.

iquintero · 2018-11-17T00:25:00Z

tests/unit/test_airflow.py

+def test_transformer_config(sagemaker_session):
+    tf_transformer = transformer.Transformer(
+        model_name="tensorflow-model",
+        instance_count="{{ instance_count }}",


Im not sure I understand what this syntax is doing?

"{{ instance_count }}"

This is Jinja templates. It will be evaluated during Airflow runtime. For example, I can put something in database (using xcom in ariflow) and do "{{ task_instance.xcom_pull(task_id='task', key='key') }}" to get the record in the table 'task' with key 'key'. Here in unit tests I just make some random Jinja templating strings for testing purpose.

laurenyu · 2018-11-17T01:20:33Z

tests/unit/test_airflow.py

+    assert config == expected_config
+
+
+def test_transform_config_from_amazon_alg_estimator(sagemaker_session):


can you include in the test name that this also tests the code paths where you don't supply the optional args?

Talked offline. No need to change.

laurenyu · 2018-11-17T01:21:56Z

src/sagemaker/estimator.py

+            if env is not None:
+                transform_env.update(env)
+        else:
+            logging.warning('No finished training job found associated with this estimator. Please make sure'


are there unit tests for this change?

Yep the transformer from estimator tests will cover this.

yangaws added 10 commits November 16, 2018 13:48

draft for model config export

43a2e53

Add APIs for Airflow model config export

7daf88d

Address comments from Lauren

18eec18

Create fake uploaded_code in framework model

f022323

Update model config from estimator with framework logic

ab57396

draft for transformer

ca0357c

Add APIs to export Airflow transform config

901f0bf

draft for deploy

6f90472

Finish rebasing

30f3030

Add docstring and more tests

0e4e314

yangaws requested review from laurenyu and icywang86rui November 16, 2018 23:42

yangaws requested a review from iquintero November 17, 2018 00:09

laurenyu reviewed Nov 17, 2018

View reviewed changes

iquintero reviewed Nov 17, 2018

View reviewed changes

Address PR comments

d1a005e

laurenyu reviewed Nov 17, 2018

View reviewed changes

laurenyu approved these changes Nov 17, 2018

View reviewed changes

yangaws merged commit 53a43f6 into aws:master Nov 18, 2018

ChoiByungWook pushed a commit that referenced this pull request Dec 8, 2020

fix: Re-enable model monitor integration tests. (#497)

37af818


		return model_config(instance_type, model, role, image)


		def transform_config(transformer, data, data_type='S3Prefix', content_type=None, compression_type=None,

		assert config == expected_config


		def test_transform_config_from_amazon_alg_estimator(sagemaker_session):

Add APIs to export transform and deploy config #497

Add APIs to export transform and deploy config #497

Uh oh!

Conversation

yangaws commented Nov 16, 2018

Merge Checklist

Uh oh!

codecov-io commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov 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!

iquintero left a comment

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!

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!

Uh oh!

codecov-io commented Nov 16, 2018 •

edited

Loading