feat: SDK Defaults - DebugHookConfig defaults in TrainingJob API #3947

knikure · 2023-06-21T05:46:58Z

Issue #, if available:

Description of changes:
Add support for DebugHookConfig defaults in TrainingJob API as part of SDK defaults feature.

Testing done:

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 certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
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 added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
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 · 2023-06-21T06:23:47Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T06:25:30Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T06:36:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T07:15:29Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2023-06-21T07:16:48Z

Codecov Report

Merging #3947 (b7c6083) into master (6d42cc8) will decrease coverage by 0.71%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3947      +/-   ##
==========================================
- Coverage   90.29%   89.58%   -0.71%     
==========================================
  Files        1292      305     -987     
  Lines      112904    27941   -84963     
==========================================
- Hits       101947    25032   -76915     
+ Misses      10957     2909    -8048

Impacted Files	Coverage Δ
src/sagemaker/clarify.py	`97.47% <100.00%> (ø)`
src/sagemaker/config/config_schema.py	`100.00% <100.00%> (ø)`
src/sagemaker/estimator.py	`91.52% <100.00%> (ø)`
src/sagemaker/image_uris.py	`98.50% <100.00%> (ø)`

... and 1593 files with indirect coverage changes

sagemaker-bot · 2023-06-21T09:22:30Z

AWS CodeBuild CI Report

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

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

knikure

/bot run notebook-tests

sagemaker-bot · 2023-06-21T16:03:07Z

AWS CodeBuild CI Report

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

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

tests/unit/test_estimator.py

src/sagemaker/config/config_schema.py

akrishna1995

Minor comments for UT, rest is LGTM

sagemaker-bot · 2023-06-21T20:40:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T21:09:57Z

AWS CodeBuild CI Report

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

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

knikure

/bot run pr, notebook-tests

sagemaker-bot · 2023-06-21T21:17:40Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T21:22:26Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T21:52:41Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T22:03:27Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-21T22:22:13Z

AWS CodeBuild CI Report

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

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

akrishna1995

Requested 1 change post discussion - approving for the rest as LGTM

sagemaker-bot · 2023-06-22T00:17:21Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T00:24:38Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T00:32:23Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T01:00:26Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T02:55:40Z

AWS CodeBuild CI Report

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

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

rubanh

Overall looks good to me. I think there might be some scenarios missing in the unit tests though. Parameterizing the tests will make it easier for us to test all the cases, and to validate that all the combinations are tested

src/sagemaker/estimator.py

tests/unit/sagemaker/config/test_config_schema.py

tests/unit/test_estimator.py

src/sagemaker/config/config_schema.py

sagemaker-bot · 2023-06-22T06:15:10Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T06:22:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T06:28:12Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T07:09:05Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-22T09:21:29Z

AWS CodeBuild CI Report

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

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

rubanh · 2023-06-23T22:37:20Z

tests/unit/test_estimator.py

+    [
+        (None, None, S3_OUTPUT_PATH_FROM_SESSION_S3_DEFAULT_CONFIG),
+        (True, None, S3_OUTPUT_PATH_FROM_SESSION_S3_DEFAULT_CONFIG),
+        (False, None, False),


Optional: For the expected value, would we want to compare against the final value passed to the training job API (rather than the value the constructor is set with)? That way we could make sure the behavior from initialization of the object to where its actually used in the end?

rubanh · 2023-06-23T22:37:24Z

tests/unit/test_estimator.py

+    if expected_debugger_hook_config_output is False:
+        assert fw.debugger_hook_config == expected_debugger_hook_config_output
+    else:
+        assert fw.debugger_hook_config.s3_output_path == expected_debugger_hook_config_output


optional: might be even more straightforward to just have the entire debug hook config in the parameterization above? That way we could also validate that the rest of the config also matches, not just the s3 path.

The comment makes sense but I wanted to make the validation more focused towards the defaults s3 path behavior. Since we already know resolve_value_from_config always takes the direct_input as final value whenever provided.

sagemaker-bot · 2023-06-23T23:00:34Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-23T23:06:48Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-23T23:15:13Z

AWS CodeBuild CI Report

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

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

knikure

/bot run notebook-tests, pr

knikure

/bot run notebook-tests, pr

sagemaker-bot · 2023-06-23T23:23:53Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-23T23:55:46Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-23T23:55:57Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-23T23:56:01Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-24T00:03:49Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-25T21:50:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-25T21:52:45Z

AWS CodeBuild CI Report

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

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

knikure requested a review from a team as a code owner June 21, 2023 05:46

knikure requested review from zuoyuanh and removed request for a team June 21, 2023 05:46

knikure added the do-not-merge Use when PR needs to be marked as do not merge label Jun 21, 2023

knikure requested review from rubanh and akrishna1995 and removed request for zuoyuanh June 21, 2023 05:49

knikure commented Jun 21, 2023

View reviewed changes

akrishna1995 reviewed Jun 21, 2023

View reviewed changes

tests/unit/test_estimator.py Show resolved Hide resolved

akrishna1995 reviewed Jun 21, 2023

View reviewed changes

src/sagemaker/config/config_schema.py Show resolved Hide resolved

akrishna1995 approved these changes Jun 21, 2023

View reviewed changes

knikure removed the do-not-merge Use when PR needs to be marked as do not merge label Jun 21, 2023

knikure force-pushed the sdk-defaults-debug-hook-config-v2 branch from 3880209 to 47b2ba4 Compare June 21, 2023 20:32

knikure commented Jun 21, 2023

View reviewed changes

knikure force-pushed the sdk-defaults-debug-hook-config-v2 branch from 47b2ba4 to 519af2e Compare June 21, 2023 23:29

akrishna1995 approved these changes Jun 21, 2023

View reviewed changes

knikure force-pushed the sdk-defaults-debug-hook-config-v2 branch from 519af2e to 2a6f23c Compare June 22, 2023 05:38

rubanh reviewed Jun 22, 2023

View reviewed changes

knikure added 2 commits June 23, 2023 22:20

feat: SDK Defaults - DebugHookConfig

7552b97

Address comments and add parameterized tests

b7c6083

knikure force-pushed the sdk-defaults-debug-hook-config-v2 branch from 2a6f23c to b7c6083 Compare June 23, 2023 22:21

rubanh approved these changes Jun 23, 2023

View reviewed changes

knikure commented Jun 23, 2023

View reviewed changes

knikure merged commit 4955d22 into aws:master Jun 26, 2023

knikure deleted the sdk-defaults-debug-hook-config-v2 branch June 26, 2023 18:44

feat: SDK Defaults - DebugHookConfig defaults in TrainingJob API #3947

feat: SDK Defaults - DebugHookConfig defaults in TrainingJob API #3947

Uh oh!

Conversation

knikure commented Jun 21, 2023

Merge Checklist

General

Tests

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

codecov-commenter commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

knikure left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

Uh oh!

Uh oh!

akrishna1995 left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

knikure left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 21, 2023

AWS CodeBuild CI Report

Uh oh!

akrishna1995 left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Jun 22, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 22, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 22, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 22, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 22, 2023

codecov-commenter commented Jun 21, 2023 •

edited

Loading

knikure left a comment •

edited

Loading

rubanh left a comment •

edited

Loading