Add support for async fit() #59

iquintero · 2018-01-25T18:57:14Z

when calling fit(wait=False) it will return immediately. The training
job will carry on even if the process exits. by using attach() the
estimator can be retrieved by providing the training job name.

This fixes: #4

when calling fit(wait=False) it will return immediately. The training job will carry on even if the process exits. by using attach() the estimator can be retrieved by providing the training job name.

iquintero · 2018-01-25T18:58:00Z

Note:

I have done a big refactor of how attach() works. This is the main change here, everything else is mostly tests.

Also fixed the timeouts for all the async fit integ tests. Previously we allowed 15 min timeout for training, and 20 min for hosting. With async fit the sections are split so we allow 5 min timeout for the intial fit call and setup. And then 35 min for the attach() + hosting calls. The total runtime is the same just split differently for async tests.

Fix the PCA and factorization machines async fit integration tests and add an exception when running Tensorboard with async fit.

mvsusp

We need to update the Readme.md as well.

mvsusp · 2018-01-29T18:20:07Z

src/sagemaker/amazon/amazon_estimator.py

@@ -64,6 +64,20 @@ def data_location(self, data_location):
            data_location = data_location + '/'
        self._data_location = data_location

+    @classmethod
+    def _from_training_job(cls, init_params, hyperparameters, image, sagemaker_session):


Can you add Docstrings to this method?

mvsusp · 2018-01-29T18:23:57Z

src/sagemaker/amazon/amazon_estimator.py

+        # and pass the correct name to the constructor.
+
+        for attribute, value in cls.__dict__.items():
+            if isinstance(value, hp):


Accessing dict to find a class attribute shows that _from_training_job should not be a class method but an instance method.

I think this is fine here, hyperparameters are implemented as descriptor objects.

mvsusp · 2018-01-29T18:25:36Z

src/sagemaker/estimator.py

+        a SageMaker Endpoint and return a ``Predictor``.
+
+        If the training job is in progress, attach will block and display log messages
+        from the training job, until the training job completes.


Can you add some code examples here?

will add an example.

mvsusp · 2018-01-29T18:26:19Z

src/sagemaker/estimator.py

+            sagemaker_session (sagemaker.session.Session): Session object which manages interactions with
+                Amazon SageMaker APIs and any other AWS services needed. If not specified, the estimator creates one
+                using the default AWS configuration chain.
+            **kwargs: Additional kwargs passed to the :class:`~sagemaker.estimator.Estimator` constructor.


kwargs is not in the method signature

will get rid of it. It used to be and I forgot to get rid of it here.

mvsusp · 2018-01-29T18:26:59Z

src/sagemaker/estimator.py

+        """
+        sagemaker_session = sagemaker_session or Session()
+
+        if training_job_name is not None:


if not training_job_name

mvsusp · 2018-01-29T18:33:19Z

src/sagemaker/estimator.py


        # drop json and remove other SageMaker specific additions
-        hyperparameters = {entry: json.loads(hp[entry]) for entry in hp}
-        framework_init_params['hyperparameters'] = hyperparameters
+        hp_map = {entry: json.loads(hyperparameters[entry]) for entry in hyperparameters}


hp is a map already. Maybe rename to deseriealized_hps or something in this lines?
My following up question is why is this method responsible to deserialize the hps?

I will rename it :D

this was already done by attach(). I don't think its worth to move this deserialization to its own method.

mvsusp · 2018-01-29T18:38:18Z

tests/integ/test_factorization_machines.py

+        time.sleep(20)
+        print("attaching now...")
+
+    with timeout_and_delete_endpoint_by_name(endpoint_name, sagemaker_session, minutes=35):


35 minutes is too long. Can we make a test under 15 minutes?

The test itself is taking the same time as the synchronous fit one, the difference is this timeout is accounting for fit() + deploy(). If you notice the timeout is consistent with the synchronous tests that we have.

The difference is the call to

with timeout_and_delete_endpoint_by_name() is spending a lot of time on attach() this time is usually spent in fit() in the synchronous tests. In this case the fit() returns right away so it doesn't account for much of the runtime. So the test takes the same amount of time to complete.

mvsusp · 2018-01-29T18:39:16Z

tests/integ/test_kmeans.py

+        time.sleep(20)
+        print("attaching now...")
+
+    with timeout_and_delete_endpoint_by_name(endpoint_name, sagemaker_session, minutes=35):


owen-t · 2018-01-29T21:30:54Z

src/sagemaker/amazon/amazon_estimator.py

+        # and pass the correct name to the constructor.
+
+        for attribute, value in cls.__dict__.items():
+            if isinstance(value, hp):


I think this is fine here, hyperparameters are implemented as descriptor objects.

owen-t · 2018-01-29T21:31:54Z

src/sagemaker/estimator.py

@@ -152,8 +152,46 @@ def fit(self, inputs, wait=True, logs=True, job_name=None):
        self.latest_training_job = _TrainingJob.start_new(self, inputs)
        if wait:
            self.latest_training_job.wait(logs=logs)
+
+    @classmethod
+    def _from_training_job(cls, init_params, hyperparameters, image, sagemaker_session):


Add documentation here, it's important. You're introducing a protocol that subclasses need to follow, so this should be documented.

owen-t

Looks like BYO is missing implementation.

In addition - please resolve the specific issues raised by MVS.

owen-t · 2018-01-29T22:03:00Z

src/sagemaker/estimator.py

        else:
-            raise NotImplemented('Asynchronous fit not available')
+            raise ValueError('must specify training_job name')


IMO, this isn't necessary, just let the underlying call fail.

BYO was missing an implementation of _from_training_job(). This adds that as well as an integration test to verify that. Also addressed the PR comments and added information to the README.

_prepare_init_params_from_job_description() is now a classmethod instead of being a static method. Each class is responsible to implement their specific logic to convert a training job description into arguments that can be passed to its own __init__()

comments already addressed. Got approval from main reviewer Owen too.

* Add data_type to hyperparameters (aws#54) When we describe a training job the data type of the hyper parameters is lost because we use a dict[str, str]. This adds a new field to Hyperparameter so that we can convert the datatypes at runtime. instead of validating with isinstance(), we cast the hp value to the type it is meant to be. This enforces a "strongly typed" value. When we deserialize from the API string responses it becomes easier to deal with too. * Add wrapper for LDA. (aws#56) Update CHANGELOG and bump the version number. * Add support for async fit() (aws#59) when calling fit(wait=False) it will return immediately. The training job will carry on even if the process exits. by using attach() the estimator can be retrieved by providing the training job name. _prepare_init_params_from_job_description() is now a classmethod instead of being a static method. Each class is responsible to implement their specific logic to convert a training job description into arguments that can be passed to its own __init__() * Fix Estimator role expansion (aws#68) Instead of manually constructing the role ARN, use the IAM boto client to do it. This properly expands service-roles and regular roles. * Add FM and LDA to the documentation. (aws#66) * Fix description of an argument of sagemaker.session.train (aws#69) * Fix description of an argument of sagemaker.session.train 'input_config' should be an array which has channel objects. * Add a link to the botocore docs * Use 'list' instead of 'array' in the description * Add ntm algorithm with doc, unit tests, integ tests (aws#73) * JSON serializer: predictor.predict accepts dictionaries (aws#62) Add support for serializing python dictionaries to json Add prediction with dictionary in tf iris integ test * Fixing timeouts for PCA async integration test. (aws#78) Execute tf_cifar test without logs to eliminate delay to detect that job has finished. * Fixes in LinearLearner and unit tests addition. (aws#77) * Print out billable seconds after training completes (aws#30) * Added: print out billable seconds after training completes * Fixed: test_session.py to pass unit tests * Fixed: removed offending tzlocal() * Use sagemaker_timestamp when creating endpoint names in integration tests. (aws#81) * Support TensorFlow-1.5.0 and MXNet-1.0.0 (aws#82) * Update .gitignore to ignore pytest_cache. * Support TensorFlow-1.5.0 and MXNet-1.0.0 * Update and refactor tests. Add tests for fw_utils. * Fix typo. * Update changelog for 1.1.0 (aws#85)

Update tensorflow_resnet_cifar10_with_tensorboard.ipynb

Add support for async fit()

79bac68

when calling fit(wait=False) it will return immediately. The training job will carry on even if the process exits. by using attach() the estimator can be retrieved by providing the training job name.

iquintero requested a review from owen-t January 25, 2018 18:57

Fix flake8 and remove unused **kwargs

ec53250

iquintero force-pushed the async_fit branch 3 times, most recently from 55abeba to a082319 Compare January 25, 2018 22:14

iquintero force-pushed the async_fit branch from a082319 to e595cc5 Compare January 25, 2018 23:19

Add exception for async tensorboard and fix tests.

0e50790

Fix the PCA and factorization machines async fit integration tests and add an exception when running Tensorboard with async fit.

mvsusp previously requested changes Jan 29, 2018

View reviewed changes

owen-t reviewed Jan 29, 2018

View reviewed changes

owen-t suggested changes Jan 29, 2018

View reviewed changes

iquintero force-pushed the async_fit branch 2 times, most recently from ec18b1f to a21d2e6 Compare January 30, 2018 01:23

Add BYOA implementation and missing docs.

24ae51e

BYO was missing an implementation of _from_training_job(). This adds that as well as an integration test to verify that. Also addressed the PR comments and added information to the README.

iquintero force-pushed the async_fit branch 2 times, most recently from 5bedd29 to 8d96cec Compare January 31, 2018 22:12

iquintero force-pushed the async_fit branch from 8d96cec to 7918d8b Compare January 31, 2018 22:39

owen-t approved these changes Jan 31, 2018

View reviewed changes

Merge branch 'master' into async_fit

03fa7b7

iquintero merged commit e1d79d5 into aws:master Feb 1, 2018

iquintero deleted the async_fit branch February 1, 2018 18:02

apacker pushed a commit to apacker/sagemaker-python-sdk that referenced this pull request Nov 15, 2018

Merge pull request aws#59 from awslabs/mvs-fixed-tensorboardbug

6131e71

Update tensorflow_resnet_cifar10_with_tensorboard.ipynb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for async fit() #59

Add support for async fit() #59

iquintero commented Jan 25, 2018 •

edited

Loading

iquintero commented Jan 25, 2018

mvsusp left a comment

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

owen-t Jan 29, 2018

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

iquintero Jan 29, 2018

mvsusp Jan 29, 2018

owen-t Jan 29, 2018

owen-t Jan 29, 2018

owen-t left a comment

owen-t Jan 29, 2018

Add support for async fit() #59

Add support for async fit() #59

Conversation

iquintero commented Jan 25, 2018 • edited Loading

iquintero commented Jan 25, 2018

mvsusp 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

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

owen-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iquintero commented Jan 25, 2018 •

edited

Loading