Replaced generic ValueError with custom subclass when reporting unexpected resource status by arodiss · Pull Request #855 · aws/sagemaker-python-sdk

arodiss · 2019-06-14T13:29:14Z

Issue #, if available:
None

Description of changes:
Replaced generic ValueError with custom subclass when reporting unexpected resource status

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-06-14T13:30:47Z

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-14T15:35:45Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

mvsusp · 2019-06-14T18:36:01Z

Hello @arodiss, thanks for the contribution. Any special reasoning behinds creating a new special exception? I would tend to just use ValueError here otherwise.

mvsusp

Hello @arodiss, thanks for the contribution. Any special reasoning behinds creating a new special exception? I would tend to just use ValueError here otherwise.

arodiss · 2019-06-17T10:50:40Z

@mvsusp
I want to catch this specific situation in my client script, and to distinguish between "batch transformation status is failed because some records are malformed" and "ValueError was raised by one of the 10000 possible sources" I have to parse error message, which is meh.
Generally, I think that specific exception classes are strictly preferable over built-ins - they allow something for a client but do not demand anything from him.
Here my custom exception is inherited from the ValueError, so if somebody is not interested in specific handling - his code will work just as it did before

mvsusp · 2019-06-17T19:04:37Z

Agreed that is helpful in your use case. Thanks for your contribution!

mvsusp · 2019-06-17T18:58:01Z

@@ -0,0 +1,9 @@
+from __future__ import absolute_import


Please add copyright.

mvsusp · 2019-06-17T19:00:56Z

+            )
        return desc

    def create_endpoint_config(self, name, model_name, initial_instance_count, instance_type,


Please include unit tests, example

sagemaker-python-sdk/tests/unit/test_create_deploy_entities.py

Line 61 in 2763f9a

def test_create_endpoint_config(sagemaker_session):

mvsusp · 2019-06-17T19:02:01Z

+                message='Error for {} {}: {} Reason: {}'.format(job_type, job, status, reason),
+                allowed_statuses=['Completed', 'Stopped'],
+                actual_status=status
+            )


We need unit tests here as well.

mvsusp · 2019-06-17T19:02:21Z

+                allowed_statuses=['InService'],
+                actual_status=status
+            )
        return desc


Please include unit tests.

mvsusp · 2019-06-17T19:03:45Z

 from sagemaker import vpc_utils
 from sagemaker.user_agent import prepend_user_agent
 from sagemaker.utils import name_from_image, secondary_training_status_changed, secondary_training_status_message
+from sagemaker.exceptions import UnexpectedStatusException


Let's try to follow Google Python Style guide, http://google.github.io/styleguide/pyguide.html#224-decision

from sagemaker import exceptions

updated.
I have to mention that other imports in the same file are quite far from the guide

jesterhazy · 2019-06-18T10:09:47Z

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-18T10:29:49Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

…ng unexpected resource status

jesterhazy · 2019-06-18T12:51:47Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

arodiss · 2019-06-19T12:06:01Z

@mvsusp
All done.
Test failures are same as on the master

jesterhazy · 2019-07-02T23:57:38Z

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-10T13:40:54Z

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-10T13:40:56Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

…ng unexpected resource status

jesterhazy · 2019-07-10T13:41:17Z

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-10T13:42: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-07-10T13:42: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

jesterhazy · 2019-07-10T13:42:50Z

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-10T13:43:25Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

arodiss · 2019-07-10T13:59:28Z

Moved to #919

mvsusp self-requested a review June 14, 2019 18:36

mvsusp suggested changes Jun 14, 2019

View reviewed changes

mvsusp suggested changes Jun 17, 2019

View reviewed changes

change: Replaced generic ValueError with custom subclass when reporti…

ffec164

…ng unexpected resource status

Merge branch 'master' into master

e24271a

change: Replaced generic ValueError with custom subclass when reporti…

dfe1fd9

…ng unexpected resource status

arodiss closed this Jul 10, 2019

Conversation

arodiss commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

Uh oh!

jesterhazy commented Jun 14, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jun 14, 2019

AWS CodeBuild CI Report

Uh oh!

mvsusp commented Jun 14, 2019

Uh oh!

mvsusp left a comment

Choose a reason for hiding this comment

Uh oh!

arodiss commented Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mvsusp commented Jun 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jesterhazy commented Jun 18, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jun 18, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jun 18, 2019

AWS CodeBuild CI Report

Uh oh!

arodiss commented Jun 19, 2019

Uh oh!

jesterhazy commented Jul 2, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

jesterhazy commented Jul 10, 2019

AWS CodeBuild CI Report

Uh oh!

arodiss commented Jul 10, 2019

Uh oh!

Reviewers

Assignees

arodiss commented Jun 14, 2019 •

edited

Loading

arodiss commented Jun 17, 2019 •

edited

Loading