feature: Estimator.fit like logs for transformer #782

imujjwal96 · 2019-05-07T17:05:09Z

Issue #, if available:

Description of changes:

Estimator.fit has a logs flag attribute but that is not the same for Transformer. We need to create the same experience in the 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 used the commit message format described in CONTRIBUTING
I have added tests that prove my fix is effective or that my feature works (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.

jesterhazy · 2019-05-07T17:06:40Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-05-07T17:40:33Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-05-09T19:07:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu · 2019-05-15T00:16:00Z

src/sagemaker/session.py

+            wait (bool): Whether to keep looking for new log entries until the job completes (default: False).
+            poll (int): The interval in seconds between polling for new log entries and job completion (default: 5).
+        Raises:
+            ValueError: If waiting and the transform job fails.


I'd remove "waiting and"

I referred the annotation of logs_for_job. Since there isn't much difference in their logic, let me know if its annotation should be updated as well.

good call - yeah, I'd update both spots

src/sagemaker/session.py

laurenyu · 2019-05-15T00:17:47Z

src/sagemaker/session.py

+        instance_count = description['TransformResources']['InstanceCount']
+        status = description['TransformJobStatus']
+
+        stream_names = []  # The list of log streams


is there enough common logic between this method and logs_for_job that could be refactored into a helper method?

refactored the two methods...

Agreed with @laurenyu here. It seems that you can refactor this even more, only the describe job call seems to be different here.

Most of this function still is a duplication of logs_for_job. Let's refactored the common logic in a private method.

laurenyu · 2019-05-15T00:19:39Z

src/sagemaker/transformer.py

@@ -79,7 +79,7 @@ def __init__(self, model_name, instance_count, instance_type, strategy=None, ass
        self.sagemaker_session = sagemaker_session or Session()

    def transform(self, data, data_type='S3Prefix', content_type=None, compression_type=None, split_type=None,
-                  job_name=None):
+                  wait=True, logs=True, job_name=None):


generally we try to add new args to the end of the list in case anyone is calling the method by relying on the order of the arguments (e.g. Python lets you call transform(data, 'S3Prefix', 'text/csv') without specifying the kwarg name)

jesterhazy · 2019-05-15T15:32:36Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-05-15T19:12:28Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu

lgtm, but with one concern around if this would be too big of a change in behavior with making transform() blocking

laurenyu · 2019-05-15T19:40:44Z

src/sagemaker/transformer.py

@@ -79,7 +79,7 @@ def __init__(self, model_name, instance_count, instance_type, strategy=None, ass
        self.sagemaker_session = sagemaker_session or Session()

    def transform(self, data, data_type='S3Prefix', content_type=None, compression_type=None, split_type=None,
-                  job_name=None):
+                  job_name=None, wait=True, logs=True):


I think this might constitute a breaking change since this method used to not be blocking, but I do like mimicking the behavior of fit()...

@mvsusp for a second opinion?

I do like mimicking the behavior of fit as well. IMO it is okay to add wait and logs with default values if these defaults keep the original behaviour, i.e. wait=False.

We need do add an integration test with wait=true as well.

mvsusp · 2019-05-21T17:07:46Z

src/sagemaker/transformer.py

@@ -79,7 +79,7 @@ def __init__(self, model_name, instance_count, instance_type, strategy=None, ass
        self.sagemaker_session = sagemaker_session or Session()

    def transform(self, data, data_type='S3Prefix', content_type=None, compression_type=None, split_type=None,
-                  job_name=None):
+                  job_name=None, wait=True, logs=True):


I do like mimicking the behavior of fit as well. IMO it is okay to add wait and logs with default values if these defaults keep the original behaviour, i.e. wait=False.

mvsusp · 2019-05-21T17:09:46Z

src/sagemaker/session.py

+        instance_count = description['TransformResources']['InstanceCount']
+        status = description['TransformJobStatus']
+
+        stream_names = []  # The list of log streams


Agreed with @laurenyu here. It seems that you can refactor this even more, only the describe job call seems to be different here.

mvsusp · 2019-05-21T17:10:33Z

src/sagemaker/session.py

+                print()
+            # Customers are not billed for hardware provisioning, so billable time is less than total time
+            billable_time = (description['TransformEndTime'] - description['TransformStartTime']) * instance_count
+            print('Billable seconds:', int(billable_time.total_seconds()) + 1)


I don't think you should calculate the billable seconds of the transform job here, I am unsure if it is calculated that way as well.

mvsusp · 2019-05-21T17:11:10Z

src/sagemaker/transformer.py

@@ -79,7 +79,7 @@ def __init__(self, model_name, instance_count, instance_type, strategy=None, ass
        self.sagemaker_session = sagemaker_session or Session()

    def transform(self, data, data_type='S3Prefix', content_type=None, compression_type=None, split_type=None,
-                  job_name=None):
+                  job_name=None, wait=True, logs=True):


We need do add an integration test with wait=true as well.

jesterhazy · 2019-05-24T19:24:48Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-06-02T10:44:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

imujjwal96 · 2019-06-04T16:51:36Z

Hey @mvsusp
I've made the requested changes. Please review them.

mvsusp

Besides the common logic between logs_for_transform_job and logs_for_job we are good to go!

mvsusp · 2019-06-07T18:07:24Z

src/sagemaker/session.py

+        instance_count = description['TransformResources']['InstanceCount']
+        status = description['TransformJobStatus']
+
+        stream_names = []  # The list of log streams


Most of this function still is a duplication of logs_for_job. Let's refactored the common logic in a private method.

jesterhazy · 2019-06-08T09:45:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

imujjwal96 · 2019-06-18T17:35:32Z

Hi @mvsusp
I had done some refactoring. Can you please review it and see if it makes sense?

icywang86rui · 2019-06-26T18:36:22Z

Please resolve the conflicts and we will take another look.

jesterhazy · 2019-06-30T14:00:21Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-06-30T14:24:33Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-07-01T15:28:06Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

jesterhazy · 2019-07-03T14:42:57Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T07:47:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T08:00:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T08:10:52Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T08:37:11Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T20:18:08Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T20:44:01Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T21:16:30Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-05T21:16:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T18:46:49Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T19:01:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T19:10:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T19:16:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T20:23:32Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T20:39:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T20:47:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

sagemaker-bot · 2019-09-06T20:51:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu · 2019-09-06T20:52:51Z

thanks for the contribution!

imujjwal96 changed the title ~~feature: Estimator.fit like logs for transformer~~ [WIP] feature: Estimator.fit like logs for transformer May 7, 2019

imujjwal96 force-pushed the mytest branch from 157ce3f to 25bf803 Compare May 7, 2019 17:39

imujjwal96 force-pushed the mytest branch from 25bf803 to bff4112 Compare May 9, 2019 17:19

imujjwal96 changed the title ~~[WIP] feature: Estimator.fit like logs for transformer~~ feature: Estimator.fit like logs for transformer May 9, 2019

laurenyu reviewed May 15, 2019

View reviewed changes

imujjwal96 force-pushed the mytest branch from bff4112 to 03d7a05 Compare May 15, 2019 13:46

imujjwal96 force-pushed the mytest branch from 03d7a05 to 29e2210 Compare May 15, 2019 17:15

laurenyu reviewed May 15, 2019

View reviewed changes

mvsusp suggested changes May 21, 2019

View reviewed changes

imujjwal96 force-pushed the mytest branch from 29e2210 to 263c12f Compare May 24, 2019 17:29

imujjwal96 force-pushed the mytest branch from 263c12f to 0e97f1b Compare June 2, 2019 08:52

mvsusp suggested changes Jun 7, 2019

View reviewed changes

imujjwal96 force-pushed the mytest branch from 0e97f1b to 1dcb705 Compare June 8, 2019 07:31

imujjwal96 force-pushed the mytest branch from 1dcb705 to 97e199a Compare June 30, 2019 13:58

imujjwal96 force-pushed the mytest branch from 97e199a to f4f3e51 Compare June 30, 2019 14:22

imujjwal96 added 6 commits September 5, 2019 05:04

feature: Estimator.fit like logs for transformer

14e52e2

Add integrations tests and refactor

3b72dd3

Refactor code

3b13e7d

black check

30fb37c

linting fixes

d178174

new

c18a62d

imujjwal96 dismissed mvsusp’s stale review via c18a62d September 5, 2019 07:42

imujjwal96 force-pushed the mytest branch from 68f445e to c18a62d Compare September 5, 2019 07:42

Merge branch 'master' into mytest

edfecbe

laurenyu approved these changes Sep 5, 2019

View reviewed changes

Merge branch 'master' into mytest

f09fcd9

Merge branch 'master' into mytest

19c5fd3

laurenyu merged commit c364fd1 into aws:master Sep 6, 2019

feature: Estimator.fit like logs for transformer #782

feature: Estimator.fit like logs for transformer #782

Conversation

imujjwal96 commented May 7, 2019 • edited Loading

Merge Checklist

jesterhazy commented May 7, 2019

AWS CodeBuild CI Report

jesterhazy commented May 7, 2019

AWS CodeBuild CI Report

jesterhazy commented May 9, 2019

AWS CodeBuild CI Report

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

jesterhazy commented May 15, 2019

AWS CodeBuild CI Report

jesterhazy commented May 15, 2019

AWS CodeBuild CI Report

laurenyu 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

jesterhazy commented May 24, 2019

AWS CodeBuild CI Report

jesterhazy commented Jun 2, 2019

AWS CodeBuild CI Report

imujjwal96 commented Jun 4, 2019

mvsusp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesterhazy commented Jun 8, 2019

AWS CodeBuild CI Report

imujjwal96 commented Jun 18, 2019

icywang86rui commented Jun 26, 2019

jesterhazy commented Jun 30, 2019

AWS CodeBuild CI Report

jesterhazy commented Jun 30, 2019

AWS CodeBuild CI Report

jesterhazy commented Jul 1, 2019

AWS CodeBuild CI Report

jesterhazy commented Jul 3, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 5, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

sagemaker-bot commented Sep 6, 2019

AWS CodeBuild CI Report

imujjwal96 commented May 7, 2019 •

edited

Loading