Allow Local Serving of Models in S3 #217
Conversation
src/sagemaker/local/image.py
Outdated
| @@ -453,8 +488,8 @@ def _build_optml_volumes(self, host, subdirs): | |||
| volumes = [] | |||
|
|
|||
| # Ensure that model is in the subdirs | |||
| if 'model' not in subdirs: | |||
| subdirs.add('model') | |||
| #if 'model' not in subdirs: | |||
There was a problem hiding this comment.
will get rid of this
tests/integ/test_local_mode.py
Outdated
| data = numpy.zeros(shape=(1, 1, 28, 28)) | ||
| predictor.predict(data) | ||
| finally: | ||
| mxnet_model.sagemaker_session.delete_endpoint('') |
There was a problem hiding this comment.
Is the empty string passed here because that's the name we explicitly chose for the endpoint?
Can we add delete_endpoint to the predictor class to make this more intuitive? That's where it should be anyway, IMO.
| key_prefix='integ-test-data/mxnet_mnist/test') | ||
|
|
||
| mx.fit({'train': train_input, 'test': test_input}) | ||
| model = mx.create_model(1) |
There was a problem hiding this comment.
Is the argument necessary here?
| model_data = mxnet_model.model_data | ||
| boto_session = sagemaker_local_session.boto_session | ||
| default_bucket = sagemaker_local_session.default_bucket() | ||
| uploaded_data = tar_and_upload_dir(boto_session, default_bucket, |
There was a problem hiding this comment.
This is only necessary to create the model data, right? In the normal use cases, the user would either do both training and deploy locally, or create the MXNetModel pointing to local model data, or already have an S3 url with model data, right?
There was a problem hiding this comment.
yeah, this is because I wanted to make this faster by training using local mode instead of training on sagemaker like the chainer tests do for instance.
|
|
||
| s3_model = MXNetModel(model_data=uploaded_data.s3_prefix, role='SageMakerRole', | ||
| entry_point=mxnet_model.entry_point, image=mxnet_model.image, | ||
| sagemaker_session=sagemaker_local_session) |
There was a problem hiding this comment.
Is it necessary to explicitly pass a LocalSession here, or is that only for our tests?
There was a problem hiding this comment.
It used to be required, now its only for our tests.
| uploaded_data = tar_and_upload_dir(boto_session, default_bucket, | ||
| 'test_mxnet_local_mode', '', model_data) | ||
|
|
||
| s3_model = MXNetModel(model_data=uploaded_data.s3_prefix, role='SageMakerRole', |
There was a problem hiding this comment.
Is role still necessary here?
There was a problem hiding this comment.
its not necessary, but the estimators/model expects one. For SageMaker this is a must have, on local mode its not even used but I dont know if we should make this optional.
| volumes = [] | ||
| if parsed_uri.scheme == 'file': | ||
| volumes.append(_Volume(parsed_uri.path, '/opt/ml/code')) | ||
| if sagemaker.estimator.DIR_PARAM_NAME.upper() in primary_container['Environment']: |
There was a problem hiding this comment.
What's the case where this is false?
There was a problem hiding this comment.
BYO :) we only have sagemaker_submit_dir on our Frameworks.
if a model is located in S3, download and extract it and then start serving as usual. By overriding the SageMaker Session on any Model Object, local serving is possible on pre-trained models.
iquintero
force-pushed
the
lm_model_serving
branch
from
18f1020 to
ed659ae
Compare
Codecov Report
@@ Coverage Diff @@
## master #217 +/- ##
=========================================
+ Coverage 91.53% 91.7% +0.16%
=========================================
Files 45 45
Lines 3166 3194 +28
=========================================
+ Hits 2898 2929 +31
+ Misses 268 265 -3
Continue to review full report at Codecov.
|
src/sagemaker/predictor.py
Outdated
| @@ -93,6 +93,12 @@ def predict(self, data): | |||
| response_body.close() | |||
| return data | |||
|
|
|||
| def delete_endpoint(self): | |||
| def delete_endpoint(self): | |||
There was a problem hiding this comment.
Why is there an inner function here? 😅
There was a problem hiding this comment.
its unclear XD. Fixed
src/sagemaker/predictor.py
Outdated
| @@ -93,6 +93,12 @@ def predict(self, data): | |||
| response_body.close() | |||
| return data | |||
|
|
|||
| def delete_endpoint(self): | |||
| def delete_endpoint(self): | |||
| """Delete an Amazon SageMaker ``Endpoint``. | |||
There was a problem hiding this comment.
How about "Delete the Amazon SageMaker endpoint backing this predictor" or similar?
if a model is located in S3, download and extract it and then start
serving as usual. By overriding the SageMaker Session on any Model
Object, local serving is possible on pre-trained models.
Issue #, if available:
Description of changes:
Merge Checklist
Put an
xin 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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.