Add EI support to TFS container. #10

chuyang-deng · 2019-02-25T19:13:42Z

Issue #, if available:
aws/amazon-sagemaker-examples#613
aws/sagemaker-python-sdk@4732545#r32336726

Description of changes:
Introduce a Dockerfile.ei to build TFS container with EI support.
Modify scripts to build and run Dockerfile.ei:

build.sh: login to account to pull cpu image since Dockerfile.ei depends on Dockerfile.cpu; also add a build argument to pass in the AmazonEI model server binary
shared.sh: add commands to parse the model variable

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

jesterhazy · 2019-02-25T19:23:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

ChoiByungWook

Can we add a EI specific integ test for this?

In addition, we should update the docs: https://github.com/aws/sagemaker-tensorflow-serving-container#building-your-image.

ChoiByungWook · 2019-02-25T20:22:55Z

docker/Dockerfile.ei

+# Validate the argument is specified
+RUN test $TENSORFLOW_MODEL_SERVER || exit 1
+
+COPY ./$TENSORFLOW_MODEL_SERVER /usr/bin/tensorflow_model_server


Is the ./ needed?

ChoiByungWook · 2019-02-25T20:25:36Z

scripts/shared.sh

@@ -32,6 +32,7 @@ function parse_std_args() {
    # defaults
    arch='cpu'
    version='1.12.0'
+    model='AmazonEI_TensorFlow_Serving_v1.12_v1'


The file hosted in S3 is a zip, where does that get handled?

also 'model' isn't a good name

+1 the s3 fetch is missing, and there should be updates to the build.sh as well (or nothing is getting built).

also, if the S3 fetch normalized the file name when it downloaded, then there would likely be no need to pass the filename into the dockerfile as an ARG.

jesterhazy · 2019-02-25T20:28:43Z

scripts/build.sh

+# logout 520713654638 account after building
+if [ "$arch" = "ei" ]; then
+    docker logout https://520713654638.dkr.ecr.$aws_region.amazonaws.com &>/dev/null
+fi


add nl @ eof

jesterhazy · 2019-02-25T20:29:19Z

scripts/build.sh

+# Dockerfile.ei needs to login 520713654638 to pull the cpu image
+if [ "$arch" = "ei" ]; then
+    echo "pulling cpu image..."
+    $(aws ecr get-login --no-include-email --registry-id 520713654638) &>/dev/null || echo 'warning: ecr login failed'


no do not log in to prod repo. this breaks pr and release build flow.

jesterhazy · 2019-02-25T20:31:00Z

docker/Dockerfile.ei

@@ -0,0 +1,10 @@
+ARG TFS_VERSION
+
+FROM 520713654638.dkr.ecr.us-west-2.amazonaws.com/sagemaker-tensorflow-serving:${TFS_VERSION}-cpu


no do not use the prod repo! this breaks pr and release build flow. the image will be built from the previously released production image instead of the current image and we will ship a stale container!

jesterhazy · 2019-02-25T20:43:43Z

scripts/shared.sh

@@ -55,6 +56,11 @@ function parse_std_args() {
        shift
        shift
        ;;
+        -m|--model)


model isn't a good name for something that points at a tfs executable

jesterhazy · 2019-02-25T20:47:21Z

docker/Dockerfile.ei

+ARG TENSORFLOW_MODEL_SERVER
+
+# Validate the argument is specified
+RUN test $TENSORFLOW_MODEL_SERVER || exit 1


i don't think this check is needed. docker build should fail by itself it you don't specify, because the arg declaration has no default

jesterhazy · 2019-02-25T20:49:19Z

scripts/build.sh


 echo "building image... "
 docker build \
    --cache-from $aws_account.dkr.ecr.$aws_region.amazonaws.com/sagemaker-tensorflow-serving:$full_version-$arch \
    --build-arg TFS_VERSION=$full_version \
    --build-arg TFS_SHORT_VERSION=$short_version \
+    --build-arg TENSORFLOW_MODEL_SERVER=$model \


consider EI_EXECUTABLE or TFS_EI_EXECUTABLE instead

jesterhazy · 2019-02-28T07:28:31Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

chuyang-deng · 2019-02-28T07:34:43Z

I've updated the scripts to handle zip files from s3 bucket, changed the EI dockerfile to avoid using prod account, dropped unnecessary arguments. I also updated README and add a functional test.

jesterhazy · 2019-02-28T07:52: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

jesterhazy · 2019-02-28T13:46:43Z

test/functional/test_elastic_inference.py

+
+@pytest.mark.skip_if_non_supported_ei_region
+@pytest.mark.skip_if_no_accelerator
+def test_deploy_elastic_inference_with_pretrained_model(pretrained_model_data, docker_image_uri, sagemaker_session, instance_type, accelerator_type):


endpoint needs to be removed at end of test

not excited about taking a sagemaker sdk dependency for this. can we change this to use boto3 only? it shouldn't be too complicated since the model is already in s3

ohhh, gotcha!

@ChuyangDeng

https://github.com/aws/sagemaker-tensorflow-container/blob/66d62178248e112bd2cbe593aa3dcceec318a652/test/resources/python_sdk/timeout.py

ChoiByungWook

Good job!

ChoiByungWook · 2019-02-28T19:06:03Z

scripts/shared.sh

@@ -28,6 +28,21 @@ function get_aws_account() {
    aws sts get-caller-identity --query 'Account' --output text
 }

+function get_tfs_executable() {


Should this function utilize the parsed args below for the TF version?

It also looks like the naming scheme isn't consistent for the zip file for 1.11 and 1.12 already, which might require updating this file often.

It only utilizes $version here, coz the naming for v1.11 and v1.12 are:
v1.11 -> Ubuntu -> Ubuntu.zip: Unbuntu/executable
v1.12 -> Ubuntu -> tfs_ei_v1_12_ubuntu.zip: v1_12_Ubuntu_2/executable

Even the v1.12's zip name and unzipped directory name are different. So I believe we will need to up date this file in the future.

It seems like in S3 it is always going to follow the pattern of: s3://amazonei-tensorflow/Tensorflow\ Serving/{version}/Ubuntu.

We can do a discovery on the item for example with:
aws s3 ls s3://amazonei-tensorflow/Tensorflow\ Serving/v1.11/Ubuntu/ | awk '{print $4}'
which should return Ubuntu.zip

Afterwards, I think we can probably also discover the unzipped name as well. For example:
find . -name AmazonEI_TensorFlow_Serving_v${version}_v1* -exec mv {} container/ \;

There are probably better commands than the ones I used above in my examples.

ChoiByungWook · 2019-02-28T19:08:32Z

test/functional/conftest.py

@@ -0,0 +1,100 @@
+# Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


@jesterhazy, do you care about this being under functional or integration/sagemaker?

I think most of our framework containers are under integration/sagemaker, except for the sagemaker-tensorflow-container repository.

yes, it seems a bit funky to have both. the stuff in test/integration are local docker container tests, so renaming test/functional to test/sagemaker or renaming everything to test/integration/{local, sagemaker} would fit the convention better

I'll rename it to test/sagemaker

ChoiByungWook · 2019-02-28T19:15:12Z

test/functional/test_elastic_inference.py

+
+@pytest.mark.skip_if_non_supported_ei_region
+@pytest.mark.skip_if_no_accelerator
+def test_deploy_elastic_inference_with_pretrained_model(pretrained_model_data, docker_image_uri, sagemaker_session, instance_type, accelerator_type):


@ChuyangDeng

https://github.com/aws/sagemaker-tensorflow-container/blob/66d62178248e112bd2cbe593aa3dcceec318a652/test/resources/python_sdk/timeout.py

ChoiByungWook · 2019-03-01T20:04:19Z

test/sagemaker/conftest.py

+
+def pytest_addoption(parser):
+    parser.addoption('--aws-id')
+    parser.addoption('--docker-base-name', default='functional-tensorflow-serving')


is functional meant to correspond to test/functional?

It's meant to be sagemaker-tensorflow-serving ... I just accidentally changed it to 'functional' for some reason I dont remember.

ChoiByungWook · 2019-03-01T20:04:52Z

test/sagemaker/conftest.py

+@pytest.fixture(scope='session')
+def tag(request, framework_version, processor):
+    provided_tag = request.config.getoption('--tag')
+    default_tag = '{}-{}-py2'.format(framework_version, processor)


do we ever specify python within the tag for this specific container?

ChoiByungWook · 2019-03-01T20:06:05Z

test/sagemaker/test_elastic_inference.py

+                           EndpointConfigName=endpoint_config_name)
+
+    try:
+        client.get_waiter('endpoint_in_service').wait(EndpointName=endpoint_name)


You don't have to do this, however I can foresee us reusing this logic, maybe it would be better to refactor to another file?

ChoiByungWook · 2019-03-01T20:07:45Z

README.md

@@ -106,6 +108,15 @@ checkers using `tox`:
 tox
 ```

+To test Elastic Inference with Accelerator, you will need an AWS account, publish your built image to ECR repository and run the following command:


can you provide an example of the command needed to run the test?

"To test against Elastic Inference, you will..."

I'll update these as well as the above comments.

jesterhazy · 2019-03-01T20:09:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

jesterhazy · 2019-03-01T20:19:13Z

test/sagemaker/test_elastic_inference.py

+    result = json.loads(response['Body'].read().decode())
+    assert result['predictions'] is not None
+
+    client.delete_endpoint(EndpointName=endpoint_name)


delete_endpoint should clear endpoint_config too
also needs to be in a finally block so delete happens even if test fails

jesterhazy · 2019-03-01T21:57: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

ChoiByungWook

LGTM!

Let's wait on approval from @jesterhazy

ChoiByungWook · 2019-03-01T23:23:03Z

test/sagemaker/test_elastic_inference.py

+
+
+def _execution_role(session):
+    return session.resource('iam').Role('SageMakerRole').arn


should we consider adding an argparser for the role?

laurenyu · 2019-03-02T00:35:29Z

test/sagemaker/conftest.py

@@ -0,0 +1,96 @@
+# Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.


should this be under test/integration/sagemaker to match the other repos?

renaming test/functional to test/sagemaker or renaming everything to test/integration/{local, sagemaker} would fit the convention better

I went with the option that involves fewer changes. but I'm also okay to put everything under test/integration/... 😺

if you do move it, do it in a different PR haha

jesterhazy

this looks great!

Chuyang Deng added 2 commits February 25, 2019 11:04

Introduce Dockerfile.ei to support EI and modify scripts.

78b9479

Incorporate build script.

4257a56

chuyang-deng requested review from jesterhazy and ChoiByungWook February 25, 2019 19:13

ChoiByungWook suggested changes Feb 25, 2019

View reviewed changes

ChoiByungWook reviewed Feb 25, 2019

View reviewed changes

jesterhazy suggested changes Feb 25, 2019

View reviewed changes

jesterhazy reviewed Feb 25, 2019

View reviewed changes

Add functional test and update README.

0a11fd3

Only get executable when --arch is ei.

66bfc3e

jesterhazy reviewed Feb 28, 2019

View reviewed changes

ChoiByungWook reviewed Feb 28, 2019

View reviewed changes

Update integ test using boto3, update shared.sh to discover file names.

cdf51e5

ChoiByungWook suggested changes Mar 1, 2019

View reviewed changes

jesterhazy reviewed Mar 1, 2019

View reviewed changes

Update README example, refactor test.

9b7ff35

ChoiByungWook approved these changes Mar 1, 2019

View reviewed changes

laurenyu reviewed Mar 2, 2019

View reviewed changes

jesterhazy approved these changes Mar 2, 2019

View reviewed changes

chuyang-deng merged commit db12d18 into aws:master Mar 4, 2019

This was referenced Mar 6, 2019

Update TFS EI example. aws/amazon-sagemaker-examples#660

Merged

change: add EI support for TFS framework aws/sagemaker-python-sdk#682

Merged

ChoiByungWook mentioned this pull request Oct 16, 2019

TensorFlowModel with Python 3, no official docker image exists to pull aws/amazon-sagemaker-examples#609

Closed

		@@ -0,0 +1,10 @@
		ARG TFS_VERSION

		FROM 520713654638.dkr.ecr.us-west-2.amazonaws.com/sagemaker-tensorflow-serving:${TFS_VERSION}-cpu

		@@ -0,0 +1,100 @@
		# Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.



		def _execution_role(session):
		return session.resource('iam').Role('SageMakerRole').arn

		@@ -0,0 +1,96 @@
		# Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved.

Add EI support to TFS container. #10

Add EI support to TFS container. #10

Uh oh!

Conversation

chuyang-deng commented Feb 25, 2019 • edited by ChoiByungWook Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jesterhazy commented Feb 25, 2019

AWS CodeBuild CI Report

Uh oh!

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

jesterhazy commented Feb 28, 2019

AWS CodeBuild CI Report

Uh oh!

chuyang-deng commented Feb 28, 2019

Uh oh!

jesterhazy commented Feb 28, 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!

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

ChoiByungWook Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chuyang-deng commented Feb 25, 2019 •

edited by ChoiByungWook

Loading

ChoiByungWook Feb 28, 2019 •

edited

Loading