Skip to content

Add Local Mode Batch Inference support. #414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Oct 11, 2018
Merged

Add Local Mode Batch Inference support. #414

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7d59fa3
Add Transformer local mode
Sep 25, 2018
3a61975
Implemented BatchStrategy
Sep 26, 2018
2ae0aab
Added remaining data source and recordio split
Sep 28, 2018
abd70af
Code complete + fix unit tests
Sep 28, 2018
df3a240
Add more unit tests and fix flake8
Oct 1, 2018
4dd8990
Completed test coverage
Oct 1, 2018
99e9ee1
changelog + readme example
Oct 1, 2018
1270160
Removed stray print() added 2 more unit tests.
Oct 1, 2018
2ef84ab
Merge branch 'master' into local_batch_inference_2
iquintero Oct 2, 2018
19d5a8f
Merge branch 'master' into local_batch_inference_2
iquintero Oct 9, 2018
4f4d1e8
address feedback
Oct 10, 2018
f5d7aa1
Merge branch 'master' into local_batch_inference_2
iquintero Oct 10, 2018
079e85b
Fix broken unit test by merge from master.
Oct 10, 2018
2bf5745
Address comments once again
Oct 10, 2018
a634efb
Fix a couple more comments
Oct 10, 2018
94c7ead
Minor minor fixes
Oct 10, 2018
dd9851f
Fix integ test + address some comments
Oct 11, 2018
807b0ec
Fix Unit test :(
Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/sagemaker/local/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ def get_splitter_instance(split_type):

Returns
:class:`sagemaker.local.data.Splitter`: an Instance of a Splitter

"""
if split_type is None:
return NoneSplitter()
Expand All @@ -79,7 +78,6 @@ def get_batch_strategy_instance(strategy, splitter):

Returns
:class:`sagemaker.local.data.BatchStrategy`: an Instance of a BatchStrategy

"""
if strategy == 'SingleRecord':
return SingleRecordStrategy(splitter)
Expand Down Expand Up @@ -126,10 +124,10 @@ def get_file_list(self):
List[str] List of absolute paths.
"""
if os.path.isdir(self.root_path):
files = [os.path.join(self.root_path, f) for f in os.listdir(self.root_path)
if os.path.isfile(os.path.join(self.root_path, f))]
return [os.path.join(self.root_path, f) for f in os.listdir(self.root_path)
if os.path.isfile(os.path.join(self.root_path, f))]
else:
files = [self.root_path]
return [self.root_path]

return files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could just put the return in each branch of the if statement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump (especially because you do it in get_root_dir)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the return files line now :)


Expand Down
3 changes: 2 additions & 1 deletion src/sagemaker/local/entities.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ def _get_container_environment(self, **kwargs):
environment['SAGEMAKER_MAX_PAYLOAD_IN_MB'] = str(kwargs['MaxPayloadInMB'])

if 'BatchStrategy' in kwargs:
strategy_env_value = ''
if kwargs['BatchStrategy'] == 'SingleRecord':
strategy_env_value = 'SINGLE_RECORD'
elif kwargs['BatchStrategy'] == 'MultiRecord':
Expand All @@ -216,6 +215,8 @@ def _get_container_environment(self, **kwargs):
environment['SAGEMAKER_BATCH_STRATEGY'] = strategy_env_value

# we only do 1 max concurrent transform in Local Mode
if 'MaxConcurrentTransforms' in kwargs and int(kwargs['MaxConcurrentTransforms']) > 1:
logger.warning('Local Mode only supports 1 ConcurrentTransform. Setting MaxConcurrentTransforms to 1')
environment['SAGEMAKER_MAX_CONCURRENT_TRANSFORMS'] = '1'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible for the user to mistakenly set this value? if so, should we have some kind of warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bump


# if there were environment variables passed to the Transformer we will pass them to the
Expand Down
3 changes: 2 additions & 1 deletion src/sagemaker/local/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ def copy_directory_structure(destination_directory, relative_path):

def move_to_destination(source, destination, sagemaker_session):
"""move source to destination. Can handle uploading to S3

Args:
source (str): root directory to move
destination (str): file:// or s3:// URI that source will be moved to.
Expand All @@ -54,7 +55,7 @@ def move_to_destination(source, destination, sagemaker_session):
path = parsed_uri.path.strip('/')
sagemaker_session.upload_data(source, bucket, path)
else:
raise ValueError('Invalid destination URI, must be s3:// or file://')
raise ValueError('Invalid destination URI, must be s3:// or file:// got: %s' % destination)

shutil.rmtree(source)

Expand Down