Model linkage #638

chuyang-deng · 2019-02-11T23:38:57Z

Issue #, if available:
#447

Description of changes:
Continuation of #630
Let Predictor delete SageMaker model directly in align with Transformer.

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.

ChoiByungWook

Awesome!

Let's modify one integ test with a deep learning framework to delete the model.

Let's add corresponding unit tests for the 1ps and frameworks :( sorry. AHAHA

src/sagemaker/predictor.py

tests/integ/test_kmeans.py

ChoiByungWook · 2019-02-12T02:56:59Z

src/sagemaker/predictor.py

+        """Access to the SageMaker Python SDK Model object
+
+        Returns:
+            object: A SageMaker Python SDK Model object


I think a little bit more description would be better here.

ChoiByungWook · 2019-02-12T20:41:06Z

src/sagemaker/predictor.py

@@ -49,13 +49,15 @@ def __init__(self, endpoint, sagemaker_session=None, serializer=None, deserializ
            content_type (str): The invocation's "ContentType", overriding any ``content_type`` from
                the serializer (default: None).
            accept (str): The invocation's "Accept", overriding any accept from the deserializer (default: None).
+            model (sagemaker.model.Model): A SageMaker Python SDK Model object.


Default? What happens when it is None?

… transformer to delete model.

…on and transformer.

… transformer to delete model.

…on and transformer.

ChoiByungWook · 2019-02-13T23:09:07Z

CHANGELOG.rst

@@ -8,8 +8,8 @@ CHANGELOG
 * 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()``


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

ohh. So the next version should be 1.18.4.dev?

I would say 'add support for..." to be consistent

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

ChoiByungWook · 2019-02-13T23:19:14Z

src/sagemaker/predictor.py

-            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): Flag to indicate whether to delete endpoint configuration together with
+                endpoint. If False, only endpoint will be deleted. Default: True.


Let's follow this for our docstrings.

https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

ChoiByungWook · 2019-02-13T23:24:05Z

src/sagemaker/predictor.py

+        endpoint_config = self.sagemaker_session.sagemaker_client.describe_endpoint_config(
+            EndpointConfigName=endpoint_config_name)
+        production_variants = endpoint_config['ProductionVariants']
+        return map(lambda d: d['ModelName'], production_variants)


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.
https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/pipeline.py

Here is an example on how it is done:
https://github.com/awslabs/amazon-sagemaker-examples/blob/master/sagemaker-python-sdk/scikit_learn_inference_pipeline/Inference%20Pipeline%20with%20Scikit-learn%20and%20Linear%20Learner.ipynb

We should make sure the deletion of multiple models in the same endpoint configuration is tested in an integ test.

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.

Just to confirm, for Pipeline, delete_model() should delete all the SageMaker models from the list[sagemaker.Model] and delete the model with name (if name is not None)?

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"?

Let's initialize the model name in the constructor per our in person discussion.

We're going to call describe_endpoint() and describe_endpoint_config() once the Predictor gets endpoint_name in the constructor, so that we don't restrict the order of deletions to model -> endpoint/endpoint config.

Change the existing delete_endpoint() functionality to depend on the endpoint_configuration_name instead of describing it.

sagemaker-python-sdk/src/sagemaker/predictor.py

Line 112 in d7a2bf6

endpoint_description = self.sagemaker_session.sagemaker_client.describe_endpoint(EndpointName=self.endpoint)

tests/integ/test_kmeans.py

ChoiByungWook · 2019-02-13T23:31:12Z

tests/unit/test_predictor.py

+    endpoint_desc = {
+        'EndpointConfigName': 'my-endpoint-config'
+    }
+    endpoint_config_desc = {


can we unit test an endpoint configuration with multiple models?

Yes, will do this in next revision.

icywang86rui · 2019-02-14T23:13:46Z

CHANGELOG.rst

@@ -8,8 +8,8 @@ CHANGELOG
 * 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()``


I would say 'add support for..." to be consistent

icywang86rui · 2019-02-14T23:14:39Z

CHANGELOG.rst

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


I guess we are already not consistent -_- . So it's up to you

icywang86rui · 2019-02-14T23:32:39Z

src/sagemaker/predictor.py

+        endpoint_config = self.sagemaker_session.sagemaker_client.describe_endpoint_config(
+            EndpointConfigName=endpoint_config_name)
+        production_variants = endpoint_config['ProductionVariants']
+        return map(lambda d: d['ModelName'], production_variants)


Let's initialize the model name in the constructor per our in person discussion.

ChoiByungWook · 2019-02-15T19:28:07Z

src/sagemaker/predictor.py

+        for model_name in self._model_names:
+            self.sagemaker_session.delete_model(model_name)
+
+    def _get_model_names(self):


this method does more than get the model names, it also get's the endpoint config name and sets it.

ChoiByungWook · 2019-02-15T19:28:34Z

setup.py

@@ -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',


your branch might be out of date?

ChoiByungWook · 2019-02-16T00:37:12Z

Closing in favor of #647

chuyang-deng requested a review from ChoiByungWook February 11, 2019 23:38

ChoiByungWook suggested changes Feb 12, 2019

View reviewed changes

ChoiByungWook reviewed Feb 12, 2019

View reviewed changes

Chuyang Deng and others added 11 commits February 13, 2019 11:00

Add new APIs to predictor to delete endpoint and endpoint config, and…

f6f0bec

… transformer to delete model.

Modify some functions, tests and update docs.

a6278bc

Modify timeout class, README, CHANGELOG and model none check in sessi…

c9245f8

…on and transformer.

Add new APIs to predictor to delete endpoint and endpoint config, and…

b380f17

… transformer to delete model.

Modify some functions, tests and update docs.

30160ed

Address comments.

56011f6

Modify timeout class, README, CHANGELOG and model none check in sessi…

0f77d10

…on and transformer.

Resolve conflicts.

05223e2

Resolve flake8 error.

b95710d

Support for Predictor to delete SageMaker model.

1ae3b34

Fix unit test.

d7a2bf6

ChoiByungWook mentioned this pull request Feb 13, 2019

Estimator.delete_endpoint does not delete the Model or Endpoint Configuration #447

Closed

ChoiByungWook suggested changes Feb 13, 2019

View reviewed changes

ChoiByungWook requested review from icywang86rui and laurenyu February 13, 2019 23:31

icywang86rui reviewed Feb 14, 2019

View reviewed changes

laurenyu and others added 3 commits February 15, 2019 10:14

Bump version to v1.18.3 (#641)

390029c

Fix README for PyPI (#643)

14ca674

Add delete_model to Predictor and Pipeline.

d37a504

ChoiByungWook reviewed Feb 15, 2019

View reviewed changes

ChoiByungWook and others added 6 commits February 15, 2019 13:33

fix EI TFS docs (#644)

a0ab2bb

Add delete_model to Predictor and Pipeline.

6a38832

Bump version to v1.18.3 (#641)

9e29d6d

Add delete_model to Predictor and Pipeline.

318bddf

Rebase with aws master and rephrase private methods.

dfc3866

Add a missed README example change.

181c08e

chuyang-deng mentioned this pull request Feb 16, 2019

Add support to delete model within Predictor and Pipeline class. #647

Merged

4 tasks

ChoiByungWook closed this Feb 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model linkage #638

Model linkage #638

chuyang-deng commented Feb 11, 2019 •

edited

Loading

ChoiByungWook left a comment

ChoiByungWook Feb 12, 2019

ChoiByungWook Feb 12, 2019

ChoiByungWook Feb 13, 2019

chuyang-deng Feb 14, 2019

icywang86rui Feb 14, 2019

laurenyu Feb 14, 2019

ChoiByungWook Feb 13, 2019

ChoiByungWook Feb 13, 2019

ChoiByungWook Feb 13, 2019

chuyang-deng Feb 14, 2019 •

edited

Loading

icywang86rui Feb 14, 2019

chuyang-deng Feb 14, 2019

ChoiByungWook Feb 14, 2019

ChoiByungWook Feb 13, 2019

chuyang-deng Feb 14, 2019

icywang86rui Feb 14, 2019

icywang86rui Feb 14, 2019

icywang86rui Feb 14, 2019

ChoiByungWook Feb 15, 2019

ChoiByungWook Feb 15, 2019

ChoiByungWook commented Feb 16, 2019

Model linkage #638

Model linkage #638

Conversation

chuyang-deng commented Feb 11, 2019 • edited Loading

Merge Checklist

ChoiByungWook 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

chuyang-deng Feb 14, 2019 • edited Loading

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

Choose a reason for hiding this comment

ChoiByungWook commented Feb 16, 2019

chuyang-deng commented Feb 11, 2019 •

edited

Loading

chuyang-deng Feb 14, 2019 •

edited

Loading