-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Model linkage #638
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.
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
Model linkage #638
Changes from all commits
f6f0bec
a6278bc
c9245f8
b380f17
30160ed
56011f6
0f77d10
05223e2
b95710d
1ae3b34
d7a2bf6
390029c
14ca674
d37a504
a0ab2bb
6a38832
9e29d6d
318bddf
dfc3866
181c08e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,26 @@ | |
CHANGELOG | ||
========= | ||
|
||
1.18.3.dev | ||
1.18.4.dev | ||
========== | ||
|
||
* doc-fix: Remove incorrect parameter for EI TFS Python README | ||
* feature: ``Predictor``: delete SageMaker model | ||
* feature: ``Pipeline``: delete SageMaker model | ||
|
||
1.18.3.post1 | ||
============ | ||
|
||
* doc-fix: fix README for PyPI | ||
|
||
1.18.3 | ||
====== | ||
|
||
* doc-fix: update information about saving models in the MXNet README | ||
* doc-fix: change ReadTheDocs links from latest to stable | ||
* doc-fix: add ``transform_fn`` information and fix ``input_fn`` signature in the MXNet README | ||
* feature: Support for ``Predictor`` to delete endpoint configuration by default when calling ``delete_endpoint()`` | ||
* feature: Support for ``model`` to delete SageMaker model | ||
* feature: Support for ``Predictor`` to delete model, and delete endpoint configuration by default when calling ``delete_endpoint()`` | ||
* feature: Support for ``Model`` to delete SageMaker model | ||
* feature: Support for ``Transformer`` to delete SageMaker model | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we are already not consistent -_- . So it's up to you |
||
* bug-fix: Default account for SKLearnModel fixed | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ def read(fname): | |
required_packages.append('enum34>=1.1.6') | ||
|
||
setup(name="sagemaker", | ||
version='1.18.2', | ||
version='1.18.3.post1', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. your branch might be out of date? |
||
description="Open source library for training and deploying models on Amazon SageMaker.", | ||
packages=find_packages('src'), | ||
package_dir={'': 'src'}, | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -56,6 +56,7 @@ def __init__(self, endpoint, sagemaker_session=None, serializer=None, deserializ | |||
self.deserializer = deserializer | ||||
self.content_type = content_type or getattr(serializer, 'content_type', None) | ||||
self.accept = accept or getattr(deserializer, 'accept', None) | ||||
self._model_names = self._get_model_names() | ||||
|
||||
def predict(self, data, initial_args=None): | ||||
"""Return the inference from the specified endpoint. | ||||
|
@@ -109,23 +110,42 @@ def _delete_endpoint_config(self): | |||
"""Delete the Amazon SageMaker endpoint configuration | ||||
|
||||
""" | ||||
endpoint_description = self.sagemaker_session.sagemaker_client.describe_endpoint(EndpointName=self.endpoint) | ||||
endpoint_config_name = endpoint_description['EndpointConfigName'] | ||||
self.sagemaker_session.delete_endpoint_config(endpoint_config_name) | ||||
self.sagemaker_session.delete_endpoint_config(self._endpoint_config_name) | ||||
|
||||
def delete_endpoint(self, delete_endpoint_config=True): | ||||
"""Delete the Amazon SageMaker endpoint and endpoint configuration backing this predictor. | ||||
"""Delete the Amazon SageMaker endpoint backing this predictor. Also delete the endpoint configuration attached | ||||
to it if delete_endpoint_config is True. | ||||
|
||||
Args: | ||||
delete_endpoint_config (bool): Flag to indicate whether to delete the corresponding SageMaker endpoint | ||||
configuration tied to the endpoint. If False, only the endpoint will be deleted. (default: True) | ||||
delete_endpoint_config (bool, optional): Flag to indicate whether to delete endpoint configuration together | ||||
with endpoint. Defaults to True. If True, both endpoint and endpoint configuration will be deleted. If | ||||
False, only endpoint will be deleted. | ||||
|
||||
""" | ||||
if delete_endpoint_config: | ||||
self._delete_endpoint_config() | ||||
|
||||
self.sagemaker_session.delete_endpoint(self.endpoint) | ||||
|
||||
def delete_model(self): | ||||
"""Deletes the Amazon SageMaker models backing this predictor. | ||||
|
||||
""" | ||||
for model_name in self._model_names: | ||||
self.sagemaker_session.delete_model(model_name) | ||||
|
||||
def _get_endpoint_config_desc(self): | ||||
endpoint_desc = self.sagemaker_session.sagemaker_client.describe_endpoint(EndpointName=self.endpoint) | ||||
self._endpoint_config_name = endpoint_desc['EndpointConfigName'] | ||||
endpoint_config = self.sagemaker_session.sagemaker_client.describe_endpoint_config( | ||||
EndpointConfigName=self._endpoint_config_name) | ||||
return endpoint_config | ||||
|
||||
def _get_model_names(self): | ||||
endpoint_config = self._get_endpoint_config_desc() | ||||
production_variants = endpoint_config['ProductionVariants'] | ||||
return map(lambda d: d['ModelName'], production_variants) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like it is possible for multiple models to exist behind an endpoint configuration and endpoint. Let's make sure this class also has the delete_model functionality as well. Here is an example on how it is done: We should make sure the deletion of multiple models in the same endpoint configuration is tested in an integ test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something to consider is that this solution assumes the endpoint configuration exists. It is possible for a user to call delete_endpoint() and that might error out when they call delete_model(). Technically, the model object does still exist within SageMaker. It arguably makes sense to actually delete the endpoint first, then endpoint configuration and then model since they all have dependency on each other. I believe SageMaker allows you to delete them regardless of order, since they snapshot everything, however this problem can still persist if the user deletes the endpoint first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm, for And for the order of deletions, if endpoint/endpoint config is deleted before attempting to delete model, should we say something like "model cannot be deleted because the attached endpoint/endpoint config is already gone"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's initialize the model name in the constructor per our in person discussion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're going to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change the existing delete_endpoint() functionality to depend on the endpoint_configuration_name instead of describing it.
|
||||
|
||||
|
||||
class _CsvSerializer(object): | ||||
def __init__(self): | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your feature addition needs to be separate and in a new version, as your last PR go merged and released to PyPI already.
https://github.com/aws/sagemaker-python-sdk/releases/tag/v1.18.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ohh. So the next version should be 1.18.4.dev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say 'add support for..." to be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even better would be something like "feature: Predictor: delete model and endpoint configuration by default when calling
delete_endpoint()
", though we've been extremely inconsistent in keeping that format