fix: disable profiler by default for regions not support it #2134

guoren66 · 2021-02-11T00:37:03Z

In China regions (cn-north-1/cn-northwest-1), if sdk is updated from 1 to latest one and start a training job, an error occurrs:

ClientError: An error occurred (ValidationException) when calling the CreateTrainingJob operation: ProfilerConfig is currently not supported in cn-northwest-1.

Many customers have reported this error (No direct solution to it in the error log) and we have to help resolve it one by one, and this also brings potential customer's churn if we haven't covered them yet.

Description of changes:
add logic of judging whether region supports profiler, if not, set 'disable_profiler' to True.

Testing done:
Unit Testing
Integ Testing

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.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to create resource names in integ tests (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sagemaker-bot · 2021-02-11T00:45:54Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: eb52ef5
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T00:48:25Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: b3846a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T01:14:31Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: eb52ef5
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T01:17:43Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: b3846a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T01:19:28Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: eb52ef5
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T01:22:27Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: b3846a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-11T01:22:29Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: b3846a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

metrizable · 2021-02-11T17:10:09Z

src/sagemaker/estimator.py

@@ -349,6 +349,9 @@ def __init__(
        self.profiler_config = profiler_config
        self.disable_profiler = disable_profiler

+        if not _region_supports_profiler(self.sagemaker_session.boto_region_name):


Do you think a test exercising this branch is warranted?

Have been passed in Unit and integ test (mxnet using default profiler confit in test profiler) from my side. Do you think it's necessary to test the branch in environment of China regions?

sagemaker-bot · 2021-02-16T18:59:30Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 7a91c3f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-16T19:34:13Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 7a91c3f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-16T19:42:11Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 7a91c3f
Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-16T20:15:27Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 7a91c3f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

guoren66 · 2021-02-18T03:48:47Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests

Commit ID: 7a91c3f

Result: FAILED

Build Logs (available for 30 days)

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

@metrizable can you help check the failed test of './amazon-sagemaker-examples/sagemaker-python-sdk/mxnet_onnx_eia/mxnet_onnx_eia.ipynb' in sagemaker-python-sdk-notebook-tests? (container not passing health check), as this PR is not for deployment.
same issue is also found in this PR #2133. Maybe it's a general one.

I have tested 'amazon-sagemaker-examples/sagemaker-python-sdk/mxnet_onnx_eia/mxnet_onnx_eia.ipynb' on 2.24.5 in us-west-2, it's ok.

sagemaker-bot · 2021-02-24T20:07:17Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 3989034
Result: SUCCEEDED
Build Logs (available for 30 days)

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

ChoiByungWook

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?

I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

sagemaker-bot · 2021-02-24T20:29:17Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 3989034
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-24T20:31:06Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 3989034
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-24T20:38:08Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 3989034
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-24T20:43:30Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 3989034
Result: SUCCEEDED
Build Logs (available for 30 days)

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

guoren66 · 2021-02-25T13:34:36Z

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?

I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

Thanks for the concern.

I have received more than 10 cases from customers in China and the error log cannot directly show them how to handle this fail. I also worry about those who are new to SageMaker and we haven't touched directly in China and encounter this fail when start a very simple training job.(this may lead to user churn) It's better to change this parameter to 'True' for the region not supporting Profiler.

I also agree with your concern about those who set this to 'False' but not enabled. So I recommend that we can add a print log inside this 'if' logic, to tell the user that this region is not supporting Profiler and we automatically set this to ‘True’, to eliminate their confusion.

ChoiByungWook · 2021-02-25T18:42:17Z

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?
I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

Thanks for the concern.

I have received more than 10 cases from customers in China and the error log cannot directly show them how to handle this fail. I also worry about those who are new to SageMaker and we haven't touched directly in China and encounter this fail when start a very simple training job.(this may lead to user churn) It's better to change this parameter to 'False' for the region not supporting Profiler.

I also agree with your concern about those who set this to 'True' but not enabled. So I recommend that we can add a print log inside this 'if' logic, to tell the user that this region is not supporting Profiler and we automatically set this to ‘False’, to eliminate their confusion.

Is profiler on by default for all regions or is it because users are attempting to use it in an unsupported region?

guoren66 · 2021-02-26T00:02:58Z

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?
I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

Thanks for the concern.
I have received more than 10 cases from customers in China and the error log cannot directly show them how to handle this fail. I also worry about those who are new to SageMaker and we haven't touched directly in China and encounter this fail when start a very simple training job.(this may lead to user churn) It's better to change this parameter to 'True' for the region not supporting Profiler.
I also agree with your concern about those who set this to 'False' but not enabled. So I recommend that we can add a print log inside this 'if' logic, to tell the user that this region is not supporting Profiler and we automatically set this to ‘True’, to eliminate their confusion.

Is profiler on by default for all regions or is it because users are attempting to use it in an unsupported region?

(I correct my above comment for the value of this parameter)
Yes. The root cause is profiler is on by default for all the regions. (the 'disable_profiler' is False by default). So this PR is for changing this parameter to 'True' when in unsupported region. I recommend I can add another print in this 'if' logic to tell user the profiler is off automatically even though you configure it to 'False' manually and explicitly.

ChoiByungWook · 2021-02-26T02:40:43Z

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?
I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

Thanks for the concern.
I have received more than 10 cases from customers in China and the error log cannot directly show them how to handle this fail. I also worry about those who are new to SageMaker and we haven't touched directly in China and encounter this fail when start a very simple training job.(this may lead to user churn) It's better to change this parameter to 'True' for the region not supporting Profiler.
I also agree with your concern about those who set this to 'False' but not enabled. So I recommend that we can add a print log inside this 'if' logic, to tell the user that this region is not supporting Profiler and we automatically set this to ‘True’, to eliminate their confusion.

Is profiler on by default for all regions or is it because users are attempting to use it in an unsupported region?

(I correct my above comment for the value of this parameter)
Yes. The root cause is profiler is on by default for all the regions. (the 'disable_profiler' is False by default). So this PR is for changing this parameter to 'True' when in unsupported region. I recommend I can add another print in this 'if' logic to tell user the profiler is off automatically even though you configure it to 'False' manually and explicitly.

Well... since it is on by default, I don't think you can discern easily whether or not a user wants to enable this feature in an unsupported region in the current if statement. You would have to do it where the defaults happen.

At least how I understand it is, if we add print statement in here, everyone in an unsupported region will get this print region, even if they aren't aware of this feature.

If the feature doesn't exist in that region, then disabling it by default seems fine. I am wondering why we enabled this feature by default and didn't make it an opt-in feature at release.

guoren66 · 2021-02-26T23:54:28Z

Would it be better to throw an exception on the client side if the user attempts to use the profiler explicitly in an unsupported region?
I would rather the user's job request fail fast if they attempt to use a feature in an unsupported region, instead of defaulting everyone off and having the job succeed. The user may get confused about why the profiler wasn't enabled in their job, even though they set the flag.

Thanks for the concern.
I have received more than 10 cases from customers in China and the error log cannot directly show them how to handle this fail. I also worry about those who are new to SageMaker and we haven't touched directly in China and encounter this fail when start a very simple training job.(this may lead to user churn) It's better to change this parameter to 'True' for the region not supporting Profiler.
I also agree with your concern about those who set this to 'False' but not enabled. So I recommend that we can add a print log inside this 'if' logic, to tell the user that this region is not supporting Profiler and we automatically set this to ‘True’, to eliminate their confusion.

Is profiler on by default for all regions or is it because users are attempting to use it in an unsupported region?

(I correct my above comment for the value of this parameter)
Yes. The root cause is profiler is on by default for all the regions. (the 'disable_profiler' is False by default). So this PR is for changing this parameter to 'True' when in unsupported region. I recommend I can add another print in this 'if' logic to tell user the profiler is off automatically even though you configure it to 'False' manually and explicitly.

Well... since it is on by default, I don't think you can discern easily whether or not a user wants to enable this feature in an unsupported region in the current if statement. You would have to do it where the defaults happen.

At least how I understand it is, if we add print statement in here, everyone in an unsupported region will get this print region, even if they aren't aware of this feature.

If the feature doesn't exist in that region, then disabling it by default seems fine. I am wondering why we enabled this feature by default and didn't make it an opt-in feature at release.

I totally agree with you about why we enable this feature by default. I don't know the specific reason maybe have some other considerations. So in this PR I propose the commits after the default setting for unsupported regions, instead of changing the default parameter from 'False' to 'None' in the init function.

Changing the default opt is an important issue I don't think we can do it in this PR.

sagemaker-bot · 2021-02-27T00:06:12Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 6e884b4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-27T00:33:14Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-slow-tests
Commit ID: 6e884b4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-27T00:35:07Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 6e884b4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-27T00:38:33Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 6e884b4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2021-02-27T00:41:39Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 6e884b4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

guoren66 and others added 2 commits February 11, 2021 00:02

fix: disable profiler by default for regions not support it

eb52ef5

Merge branch 'master' into fix-disable-profiler-settings

b3846a9

metrizable reviewed Feb 11, 2021

View reviewed changes

Merge branch 'master' into fix-disable-profiler-settings

7a91c3f

Merge branch 'master' into fix-disable-profiler-settings

3989034

ChoiByungWook reviewed Feb 24, 2021

View reviewed changes

Merge branch 'master' into fix-disable-profiler-settings

6e884b4

ChoiByungWook approved these changes Feb 26, 2021

View reviewed changes

ChoiByungWook merged commit 2904cd3 into aws:master Feb 27, 2021

fix: disable profiler by default for regions not support it #2134

fix: disable profiler by default for regions not support it #2134

Uh oh!

Conversation

guoren66 commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 11, 2021

AWS CodeBuild CI Report

Uh oh!

metrizable Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

guoren66 Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Feb 16, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 16, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 16, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 16, 2021

AWS CodeBuild CI Report

Uh oh!

guoren66 commented Feb 18, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 24, 2021

AWS CodeBuild CI Report

Uh oh!

ChoiByungWook left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Feb 24, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 24, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 24, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Feb 24, 2021

AWS CodeBuild CI Report

Uh oh!

guoren66 commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChoiByungWook commented Feb 25, 2021

Uh oh!

guoren66 commented Feb 26, 2021

Uh oh!

ChoiByungWook commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guoren66 commented Feb 11, 2021 •

edited

Loading

guoren66 commented Feb 25, 2021 •

edited

Loading

ChoiByungWook commented Feb 26, 2021 •

edited

Loading