Enable setting VPC config when creating/deploying models #412

mmeidl · 2018-09-29T01:56:22Z

Issue #, if available: N/A

Description of changes: Previous commits (#353 , #370 ) enabled users to set VpcConfig on Estimators/Training Jobs. The same VpcConfig can be set on Models in order to protect network traffic in inference use cases (see docs).

In this PR I've added the optional parameter vpc_config to Model constructors and Session methods, and modified most create_model and deploy implementations to pass forward the same VPC config used in training, or take an override from the optional parameter vpc_config.

To ensure the functionality works for both training and inference, I've improved the integ tests to cover VPC features for a variety of models. Note that name and resource configurations belonging to the test VPC have changed. Before running the integ tests in your account, you should delete the existing test VPC.

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.

Models created from Estimator or Training Job use the same VpcConfig

…mator, Model, Session

Configure subnets and security groups to support multi-node VPC jobs and endpoints Implement more VPC integ tests covering several Estimators and Predictors

icywang86rui · 2018-10-01T20:44:15Z

tests/integ/vpc_utils.py

@@ -47,25 +49,43 @@ def _get_route_table_id(ec2_client, vpc_id):
    return desc['RouteTables'][0]['RouteTableId']


-def create_vpc_with_name(ec2_client, name):
+def create_vpc_with_name(ec2_client, region, name):


Is this called outside of the module? We should make it private as well. I think it was my fault.

icywang86rui · 2018-10-01T20:44:48Z

tests/integ/vpc_utils.py


-    subnet_id = ec2_client.create_subnet(CidrBlock='10.0.0.0/24', VpcId=vpc_id)['Subnet']['SubnetId']
+    # sagemaker endpoints require subnets in at least 2 different AZs for vpc mode
+    subnet_id_a = ec2_client.create_subnet(CidrBlock='10.0.0.0/24', VpcId=vpc_id,


Optional - I would create another method for creating subnets. YMMV

I think I'll keep this under one create_vpc_with_name method. Most of the create_subnet parameters are coupled to the vpc's ID and CIDR range anyway.

icywang86rui · 2018-10-01T20:57:34Z

tests/integ/test_vpc.py

+                           subnets=subnet_ids,
+                           security_group_ids=[security_group_id])
+
+    with timeout(minutes=TRAINING_DEFAULT_TIMEOUT_MINUTES):


The code from this line to the end of the function is duplicated multiple times in this test module. I would have a function called something like this:

def run_training_job_and_verify(estimator) with timeout(...): estimator.fit()

I thought about this, though one difficulty is that each Estimator/Predictor has some slightly different input shapes and serializers that would need to be set by the generic function. I'll give it a try.

icywang86rui · 2018-10-01T21:02:03Z

src/sagemaker/session.py

@@ -489,6 +496,11 @@ def create_model_from_job(self, training_job_name, name=None, role=None, primary
            model_data_url (str): S3 location of the model data (default: None). If None, defaults to
                the ``ModelS3Artifacts`` of ``training_job_name``.
            env (dict[string,string]): Model environment variables (default: {}).
+            vpc_config (dict[str, list[str]]):


icywang86rui · 2018-10-01T21:07:02Z

src/sagemaker/session.py

@@ -671,6 +684,10 @@ def endpoint_from_job(self, job_name, initial_instance_count, instance_type,
            wait (bool): Whether to wait for the endpoint deployment to complete before returning (default: True).
            model_environment_vars (dict[str, str]): Environment variables to set on the model container
                (default: None).
+            model_vpc_config (dict[str, list[str]]): Overrides VpcConfig set on the model.


Tom mentioned he wants us to use a marker like USE_MODEL_VPC. Are we still doing that?

Tom clarified that he was asking for a global constant like this which would be used as the default value. It makes it more clear for a user to reuse training's vpc_config, or else override to some different value, or even None. I will add this.

icywang86rui · 2018-10-01T21:08:02Z

src/sagemaker/session.py

@@ -721,7 +743,8 @@ def endpoint_from_model_data(self, model_s3_location, deployment_image, initial_
                              role=role,
                              primary_container=container_def(image=deployment_image,
                                                              model_data_url=model_s3_location,
-                                                              env=model_environment_vars))
+                                                              env=model_environment_vars),
+                              vpc_config=model_vpc_config)


indentation

mvsusp · 2018-10-01T21:56:15Z

src/sagemaker/amazon/factorization_machines.py

@@ -163,11 +164,19 @@ def __init__(self, role, train_instance_count, train_instance_type,
        self.factors_init_sigma = factors_init_sigma
        self.factors_init_value = factors_init_value

-    def create_model(self):
+    def create_model(self, vpc_config=None):


Nit: add a link to the documentation about vpc config in the docstring.l

I may add a doc link in the Model/Session methods, but I don't see the need to add it to every Estimator.

That might be fine. This project follows the Python Google Style standards https://github.com/google/styleguide/blob/gh-pages/pyguide.md#383-functions-and-methods. Please take them in consideration.

mvsusp · 2018-10-02T17:24:21Z

src/sagemaker/utils.py

@@ -88,6 +88,13 @@ def wrapper(*args, **kwargs):
    return wrapper


+def vpc_config_dict(subnets, security_group_ids):


This function should not be in utils. Maybe in session.py where container_def is defined? https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/session.py#L917

Please, add docstrings.

The function is imported and used in all Estimators too. It's how they translate the subnets and security_group_ids attributes into a vpc_config passed to the Model or Session method. For this reason I put it in utils so it can be imported from a central module.

Would you rather see

from sagemaker.session import Session, vpc_config_dict

or

from sagemaker.session import Session from sagemaker.utils import vpc_config_dict

either way, yes I will add docstring

I also considered moving to a totally new module vpc_utils which would include the following:

VPC_CONFIG_DEFAULT = {} def vpc_config_dict(subnets, security_group_ids): ... def vpc_config_unpack(vpc_config): ...

Any guiding principles for when to split off a new utility module?

Based on https://github.com/google/styleguide/blob/gh-pages/pyguide.md#224-decision. I would prefer

from sagemaker import session session.Session ... session.vpc_config_dict

I am ok if you decide to move the vpc to a separate file thought.

mvsusp · 2018-10-02T17:25:38Z

tests/integ/test_vpc.py

+DATA_PATH = os.path.join(DATA_DIR, 'iris', 'data')
+
+
+@pytest.mark.skipif(PYTHON_VERSION != 'py2', reason="TensorFlow image supports only python 2.")


nit: use not instead of !=

tests/integ/test_vpc.py

… estimators

…idate to sanitize

codecov-io · 2018-10-04T01:19:00Z

Codecov Report

Merging #412 into master will increase coverage by 0.07%.
The diff coverage is 98.23%.

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   93.29%   93.36%   +0.07%     
==========================================
  Files          52       53       +1     
  Lines        3655     3724      +69     
==========================================
+ Hits         3410     3477      +67     
- Misses        245      247       +2

Impacted Files	Coverage Δ
src/sagemaker/model.py	`95.58% <100%> (+0.06%)`	⬆️
src/sagemaker/amazon/randomcutforest.py	`100% <100%> (ø)`	⬆️
src/sagemaker/amazon/ntm.py	`100% <100%> (ø)`	⬆️
src/sagemaker/tensorflow/estimator.py	`93.43% <100%> (+0.04%)`	⬆️
src/sagemaker/estimator.py	`89.51% <100%> (+0.45%)`	⬆️
src/sagemaker/amazon/lda.py	`97.56% <100%> (+0.06%)`	⬆️
src/sagemaker/session.py	`89.43% <100%> (+0.32%)`	⬆️
src/sagemaker/amazon/knn.py	`100% <100%> (ø)`	⬆️
src/sagemaker/pytorch/estimator.py	`100% <100%> (ø)`	⬆️
src/sagemaker/amazon/factorization_machines.py	`100% <100%> (ø)`	⬆️
... and 8 more

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 4d6a4a4...aedd5cd. Read the comment docs.

mvsusp

Thanks for working on this.

* Create SageMaker Models including optional VpcConfig Models created from Estimator or Training Job use the same VpcConfig * Adjust naming, preparing, and passing vpc_config in base classes Estimator, Model, Session * Add optional vpc_config parameter for Amazon/Frameworks models * Improve integ test coverage of VPC features Configure subnets and security groups to support multi-node VPC jobs and endpoints Implement more VPC integ tests covering several Estimators and Predictors * Update changelog * Add vpc_utils with constants and utilities for handling VpcConfig * Rename/refactor parameter vpc_config_override for models created from estimators * Consolidate VPC integ tests to a single TensorFlow multi-instance case * Tweaks to vpc_utils: make VPC_CONFIG_DEFAULT immutable and rename validate to sanitize * Add integ test for transform with vpc config * Update README with SageMaker VPC documentation and examples * Remove tests/integ/test_vpc.py

Updated: notebook name from HPO to tune

Michael Meidl added 5 commits September 25, 2018 01:58

Create SageMaker Models including optional VpcConfig

36290ee

Models created from Estimator or Training Job use the same VpcConfig

Adjust naming, preparing, and passing vpc_config in base classes Esti…

6a4e50f

…mator, Model, Session

Add optional vpc_config parameter for Amazon/Frameworks models

c94d7fb

Improve integ test coverage of VPC features

c47a94d

Configure subnets and security groups to support multi-node VPC jobs and endpoints Implement more VPC integ tests covering several Estimators and Predictors

Update changelog

b9701cd

icywang86rui reviewed Oct 1, 2018

View reviewed changes

mvsusp suggested changes Oct 2, 2018

View reviewed changes

Michael Meidl and others added 5 commits October 3, 2018 23:28

Add vpc_utils with constants and utilities for handling VpcConfig

5a0aba8

Rename/refactor parameter vpc_config_override for models created from…

65d688e

… estimators

Consolidate VPC integ tests to a single TensorFlow multi-instance case

fcd3db3

Tweaks to vpc_utils: make VPC_CONFIG_DEFAULT immutable and rename val…

a853f58

…idate to sanitize

Merge branch 'master' into model-vpc

49d48fb

Michael Meidl added 2 commits October 4, 2018 06:21

Add integ test for transform with vpc config

f5b7144

Update README with SageMaker VPC documentation and examples

c6e7b7c

mvsusp previously approved these changes Oct 4, 2018

View reviewed changes

Remove tests/integ/test_vpc.py

6d8e06c

mmeidl dismissed mvsusp’s stale review via 6d8e06c October 4, 2018 21:14

mvsusp previously approved these changes Oct 4, 2018

View reviewed changes

Merge branch 'master' into model-vpc

aedd5cd

mmeidl dismissed mvsusp’s stale review via aedd5cd October 4, 2018 22:09

mvsusp approved these changes Oct 4, 2018

View reviewed changes

mvsusp merged commit dd44d25 into aws:master Oct 5, 2018

apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018

Merge pull request aws#412 from awslabs/arpin_change_hpo_name

e8aa973

Updated: notebook name from HPO to tune

		@@ -88,6 +88,13 @@ def wrapper(args, *kwargs):
		return wrapper


		def vpc_config_dict(subnets, security_group_ids):

		DATA_PATH = os.path.join(DATA_DIR, 'iris', 'data')


		@pytest.mark.skipif(PYTHON_VERSION != 'py2', reason="TensorFlow image supports only python 2.")

Enable setting VPC config when creating/deploying models #412

Enable setting VPC config when creating/deploying models #412

Uh oh!

Conversation

mmeidl commented Sep 29, 2018

Merge Checklist

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 Oct 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvsusp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 4, 2018 •

edited

Loading