Uh oh!
There was an error while loading. Please reload this page.
add tfs container support - #460
Conversation
Codecov Report
@@ Coverage Diff @@## master #460 +/- ##
==========================================
+ Coverage 93.77% 93.86% +0.08%
==========================================
Files 55 56 +1 Lines 4114 4190 +76 ==========================================
+ Hits 3858 3933 +75 - Misses 256 257 +1
Continue to review full report at Codecov.
|
owen-t
left a comment
There was a problem hiding this comment.
Please consider including Readme updates as well.
Uh oh!
There was an error while loading. Please reload this page.
| If a serializer was specified when creating the RealTimePredictor, the result of the | ||
| serializer is sent as input data. Otherwise the data must be sequence of bytes, and | ||
| the predict method then sends the bytes in the request body as is. | ||
| initial_args (dict[str,str]): Optional. Initial request arguments. Default is None. |
There was a problem hiding this comment.
Please clarify more this docstring argument.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| from sagemaker.tensorflow.estimator import TensorFlow # noqa: E402, F401 | ||
| from sagemaker.tensorflow.model import TensorFlowModel, TensorFlowPredictor # noqa: E402, F401 | ||
| from sagemaker.tensorflow.tfs import TFSModel, TFSPredictor # noqa: E402, F401 |
There was a problem hiding this comment.
Let's avoid acronyms to keep the same pattern of the SDK
There was a problem hiding this comment.
'TensorFlowServingModel' is unwieldy, and ambiguous since TensorFlowModel also refers to a kind of TensorFlow Serving model. TFS is well aligned with the module name, and the container side package name.
Another option... just calling them Model and Predictor (sagemaker.tensorflow.tfs.Model etc), but this invites collision with sagemaker.Model etc.
There was a problem hiding this comment.
what about sagemaker.tesorflow.serving.Model or sagemaker.tensorflow_serving.Model?
Collision should not happen if we are importing modules not classes like specified in the style guide.
| 1.14.0-dev | ||
| ========== | ||
| * add support for sagemaker-tfs container |
There was a problem hiding this comment.
Please follow correct changelog format.
| name=self._current_job_name, | ||
| framework_version=self.framework_version, | ||
| sagemaker_session=self.sagemaker_session, | ||
| vpc_config=self.get_vpc_config(vpc_config_override)) |
There was a problem hiding this comment.
Are you missing container_log_level?
| from sagemaker import Model, RealTimePredictor | ||
| from sagemaker.content_types import CONTENT_TYPE_JSON | ||
| from sagemaker.fw_utils import create_image_uri | ||
| from sagemaker.predictor import json_serializer, json_deserializer |
There was a problem hiding this comment.
nit: from google python style guide -> import modules not functions and classes.
| attributes.append('tfs-model-name={}'.format(model_name)) | ||
| if model_version: | ||
| attributes.append('tfs-model-version={}'.format(model_version)) | ||
| self._model_attributes = ','.join(attributes) if attributes else None |
There was a problem hiding this comment.
Is it possible to use an empty array to represent an empty list of model attributes instead of None?
| class TFSModel(Model): | ||
| FRAMEWORK_NAME = 'tfs' |
There was a problem hiding this comment.
I do believe that other classes use dunders instead of a constant, although I Iike your style better and matches the conventions https://github.com/google/styleguide/blob/gh-pages/pyguide.md#3164-guidelines-derived-from-guidos-recommendations
nit: consider leaving it at module level
| logging.WARNING: 'warn', | ||
| logging.ERROR: 'error', | ||
| logging.CRITICAL: 'crit', | ||
| } |
| logging.WARNING: 'warn', | ||
| logging.ERROR: 'error', | ||
| logging.CRITICAL: 'crit', | ||
| } |
There was a problem hiding this comment.
Dictionaries are not constants in Python.
| return self.env | ||
| env = dict(self.env) | ||
| env['SAGEMAKER_TFS_NGINX_LOGLEVEL'] = TFSModel.LOG_LEVEL_MAP[self._container_log_level] |
There was a problem hiding this comment.
Let's use a constant for this key.
| # reuse standard image uri function, then strip unwanted python component | ||
| region_name = self.sagemaker_session.boto_region_name | ||
| image = create_image_uri(region_name, TFSModel.FRAMEWORK_NAME, instance_type, | ||
| self._framework_version, 'py3') |
There was a problem hiding this comment.
How hard is to change create image uri instead of reusing and striping it.
There was a problem hiding this comment.
lots of fussy behavior in create_image_uri. it would be easy to fall out of sync
| with timeout_and_delete_endpoint_by_name(endpoint_name, sagemaker_session): | ||
| model = TFSModel(model_data, 'SageMakerRole', framework_version=tf_full_version) | ||
| predictor = model.deploy(1, 'ml.c5.xlarge', endpoint_name=endpoint_name) | ||
| yield predictor |
| serializer=sagemaker.predictor.csv_serializer) | ||
| result = predictor.predict(input_data) | ||
| assert expected_result == result |
There was a problem hiding this comment.
We are missing test cases for errors and empty requests.
There was a problem hiding this comment.
- handled in unit tests and in container package tests
- also this is generic predictor / serializer behavior that is well covered by other tests
* add tensorflow serving container support
Description of changes:
add support for sagemaker-tfs inference container
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.