Add Local Mode Batch Inference support. #414

iquintero · 2018-10-01T18:15:46Z

Add support for Local Mode Batch Inference (Local Transform Jobs).

Creating local transform jobs will perform all the steps locally. The serving container will be created the same way as it is for serving but then the data will be fed to it and written to the corresponding output location. Data splits and batch strategies are implemented as closely as possible to the behavior in SageMaker so that the end results are consistent.

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 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.

basic stuff works

It is magic! No, it actually checks how to assemble the already split data to feed into the container.

Fixed all broken tests and code complete. Need to add a few more tests

codecov-io · 2018-10-01T18:18:19Z

Codecov Report

Merging #414 into master will increase coverage by 0.27%.
The diff coverage is 92.64%.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   93.44%   93.71%   +0.27%     
==========================================
  Files          53       55       +2     
  Lines        3737     4010     +273     
==========================================
+ Hits         3492     3758     +266     
- Misses        245      252       +7

Impacted Files	Coverage Δ
src/sagemaker/amazon/common.py	`97.01% <100%> (ø)`	⬆️
src/sagemaker/transformer.py	`100% <100%> (ø)`	⬆️
src/sagemaker/local/local_session.py	`88.88% <78.57%> (-2.13%)`	⬇️
src/sagemaker/local/image.py	`90.76% <83.33%> (+2.22%)`	⬆️
src/sagemaker/utils.py	`90.29% <83.87%> (-2.86%)`	⬇️
src/sagemaker/local/entities.py	`95.17% <93.7%> (+2.94%)`	⬆️
src/sagemaker/local/data.py	`94.64% <94.64%> (ø)`
src/sagemaker/local/utils.py	`96.66% <96.66%> (ø)`
... and 1 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 80ba921...94c7ead. Read the comment docs.

laurenyu · 2018-10-02T16:22:56Z

CHANGELOG.rst

@@ -6,6 +6,7 @@ CHANGELOG
 =========

 * enhancement: Local Mode: add training environment variables for AWS region and job name
+* feature: Add support for Local Mode Batch Inference


feature: Local Mode: add support for Batch Transform

README.rst

src/sagemaker/local/data.py

src/sagemaker/local/entities.py

src/sagemaker/local/utils.py

tests/unit/test_local_data.py

tests/unit/test_local_entities.py

tests/unit/test_local_session.py

tests/unit/test_local_utils.py

laurenyu · 2018-10-04T00:29:00Z

tests/unit/test_local_utils.py

+
+BUCKET_NAME = 'some-nice-bucket'
+
+


no tests for copy_directory_structure or recursive_copy?

mvsusp

Some docstrings are missing.

src/sagemaker/local/data.py

src/sagemaker/local/entities.py

tests/integ/test_local_mode.py

One unit test broke when merging from master. Fixing it.

laurenyu

might want to just do a pass through all of the docstrings

README.rst

CHANGELOG.rst

src/sagemaker/local/data.py

laurenyu · 2018-10-10T22:53:16Z

src/sagemaker/local/data.py

+        else:
+            files = [self.root_path]
+
+        return files


bump (especially because you do it in get_root_dir)

laurenyu · 2018-10-10T22:54:43Z

src/sagemaker/local/entities.py

+            environment['SAGEMAKER_BATCH_STRATEGY'] = strategy_env_value
+
+        # we only do 1 max concurrent transform in Local Mode
+        environment['SAGEMAKER_MAX_CONCURRENT_TRANSFORMS'] = '1'


laurenyu · 2018-10-10T23:01:13Z

src/sagemaker/local/utils.py

+        path = parsed_uri.path.strip('/')
+        sagemaker_session.upload_data(source, bucket, path)
+    else:
+        raise ValueError('Invalid destination URI, must be s3:// or file://')


include the URI in the error message

src/sagemaker/local/utils.py

src/sagemaker/local/data.py

laurenyu · 2018-10-10T23:04:52Z

src/sagemaker/local/local_session.py

+
+    def describe_transform_job(self, TransformJobName):
+        if TransformJobName not in LocalSagemakerClient._transform_jobs:
+            error_response = {'Error': {'Code': 'ValidationException', 'Message': 'Could not find local transform job'}}


I'd include TransformJobName in the error message

laurenyu · 2018-10-10T23:06:28Z

tests/integ/test_local_mode.py

+
+
+@pytest.mark.continuous_testing
+def test_local_transform_mxnet(sagemaker_local_session, tmpdir):


nitpick: s/transform/batch_transform (just in case anyone thinks of transform as real-time inference since the input-predict-output cycle is "transform")

laurenyu · 2018-10-10T23:07:33Z

tests/integ/test_local_mode.py

+@pytest.mark.continuous_testing
+def test_local_transform_mxnet(sagemaker_local_session, tmpdir):
+    data_path = os.path.join(DATA_DIR, 'mxnet_mnist')
+    script_path = os.path.join(data_path, 'mnist.py')


a lot of these tests seem to do the same boilerplate MXNet/MNIST setup - perhaps something can be refactored?

laurenyu · 2018-10-10T23:08:17Z

tests/unit/test_local_data.py

+
+
+def test_recordio_splitter(tmpdir):
+


nitpick: remove the extra newline

laurenyu · 2018-10-10T23:08:56Z

tests/unit/test_local_data.py

+def test_get_batch_strategy_instance_with_invalid_type():
+    with pytest.raises(ValueError):
+        # something invalid
+        sagemaker.local.data.get_batch_strategy_instance('NiceRecord', None)


personal preference - I'd just make the string 'SomethingInvalid' instead of writing a comment to explain it

laurenyu · 2018-10-10T23:36:14Z

src/sagemaker/local/data.py

+        else:
+            files = [self.root_path]
+
+        return files


you can remove the return files line now :)

src/sagemaker/local/utils.py

Creating local batch transform jobs will perform all the steps locally. The serving container will be created the same way as it is for serving but then the data will be fed to it and written to the corresponding output location. Data splits and batch strategies are implemented as closely as possible to the behavior in SageMaker so that the end results are consistent.

Ignacio Quintero added 5 commits September 28, 2018 10:52

Add Transformer local mode

7d59fa3

basic stuff works

Implemented BatchStrategy

3a61975

It is magic! No, it actually checks how to assemble the already split data to feed into the container.

Added remaining data source and recordio split

2ae0aab

Code complete + fix unit tests

abd70af

Fixed all broken tests and code complete. Need to add a few more tests

Add more unit tests and fix flake8

df3a240

Ignacio Quintero added 3 commits October 1, 2018 16:33

Completed test coverage

4dd8990

changelog + readme example

99e9ee1

Removed stray print() added 2 more unit tests.

1270160

iquintero changed the title ~~Local batch inference~~ Add Local Mode Batch Inference support. Oct 1, 2018

iquintero requested a review from laurenyu October 2, 2018 00:01

laurenyu reviewed Oct 2, 2018

View reviewed changes

Merge branch 'master' into local_batch_inference_2

2ef84ab

laurenyu reviewed Oct 4, 2018

View reviewed changes

mvsusp suggested changes Oct 6, 2018

View reviewed changes

iquintero and others added 4 commits October 9, 2018 11:09

Merge branch 'master' into local_batch_inference_2

19d5a8f

address feedback

4f4d1e8

Merge branch 'master' into local_batch_inference_2

f5d7aa1

Fix broken unit test by merge from master.

079e85b

One unit test broke when merging from master. Fixing it.

laurenyu reviewed Oct 10, 2018

View reviewed changes

Address comments once again

2bf5745

laurenyu reviewed Oct 10, 2018

View reviewed changes

Fix a couple more comments

a634efb

laurenyu reviewed Oct 10, 2018

View reviewed changes

Minor minor fixes

94c7ead

laurenyu previously approved these changes Oct 10, 2018

View reviewed changes

mvsusp previously approved these changes Oct 10, 2018

View reviewed changes

Fix integ test + address some comments

dd9851f

iquintero dismissed stale reviews from mvsusp and laurenyu via dd9851f October 11, 2018 18:53

laurenyu previously approved these changes Oct 11, 2018

View reviewed changes

Fix Unit test :(

807b0ec

iquintero dismissed laurenyu’s stale review via 807b0ec October 11, 2018 19:54

laurenyu approved these changes Oct 11, 2018

View reviewed changes

iquintero merged commit c8006fa into aws:master Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Local Mode Batch Inference support. #414

Add Local Mode Batch Inference support. #414

iquintero commented Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading

laurenyu Oct 2, 2018

laurenyu Oct 4, 2018

mvsusp left a comment

laurenyu left a comment

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018

laurenyu Oct 10, 2018



		@pytest.mark.continuous_testing
		def test_local_transform_mxnet(sagemaker_local_session, tmpdir):


		BUCKET_NAME = 'some-nice-bucket'

Add Local Mode Batch Inference support. #414

Add Local Mode Batch Inference support. #414

Conversation

iquintero commented Oct 1, 2018 • edited Loading

Merge Checklist

codecov-io commented Oct 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvsusp left a comment

Choose a reason for hiding this comment

laurenyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iquintero commented Oct 1, 2018 •

edited

Loading

codecov-io commented Oct 1, 2018 •

edited

Loading