feature: Allow custom output for RepackModelStep #2804

tuliocasagrande · 2021-12-17T15:49:15Z

Issue #, if available:
#2803

Description of changes:
As proposed in #2803:

Add new parameter repack_output_path (default None) to workflow.step_collections.RegisterModel, workflow.step_collections.EstimatorTransformer and workflow._utils._RepackModelStep.
On internal _RepackModelStep() calls, handover repack_output_path=repack_output_path received from public RegisterModel() and EstimatorTransformer()
Inside of _RepackModelStep, send output_path=repack_output_path and code_location=repack_output_path to the internal SKLearn() call.

Testing done:

Tested RegisterModel without repack_output_path (current behavior):

    step_register = RegisterModel(
        name="RegisterModel",
        model=pipeline_model,
        content_types=["text/csv"],
        response_types=["text/csv"],
        inference_instances=["ml.t2.medium", "ml.m5.large"],
        transform_instances=["ml.m5.large"],
        model_package_group_name=model_package_group_name,
        approval_status=model_approval_status,
        model_metrics=model_metrics,
    )

Current behaviour stayed the same on the generated pipeline:

...
"OutputDataConfig": {
    "S3OutputPath": "s3://<my-default-bucket>/"
},
...

Tested RegisterModel with repack_output_path:

    step_register = RegisterModel(
        name="RegisterModel",
        model=pipeline_model,
        content_types=["text/csv"],
        response_types=["text/csv"],
        inference_instances=["ml.t2.medium", "ml.m5.large"],
        transform_instances=["ml.m5.large"],
        model_package_group_name=model_package_group_name,
        approval_status=model_approval_status,
        model_metrics=model_metrics,
        repack_output_path=f"s3://{default_bucket}/{base_job_prefix}/repack",
    )

New output on RepackModel step:

...
"OutputDataConfig": {
    "S3OutputPath": "s3://<my-default-bucket>/<my-base-job-prefix>/repack"
},
...

Added unit test tests/unit/sagemaker/workflow/test_utils.py::test_repack_model_step_with_output_path.

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 backword 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.

aws#2790)

…ries (aws#2755) Co-authored-by: Payton Staub <[email protected]> Co-authored-by: Ahsan Khan <[email protected]>

Co-authored-by: Shreya Pandit <[email protected]>

sagemaker-bot · 2021-12-17T16:09:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-12-17T20:02:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-12-17T20:47:47Z

AWS CodeBuild CI Report

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

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

Fixes aws#2803

sagemaker-bot · 2021-12-17T21:21:26Z

AWS CodeBuild CI Report

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

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

tuliocasagrande · 2021-12-17T21:26:06Z

@staubhp I finished this proposal.
I'd love to hear any feedback when you have the chance. Thanks for reviewing!

staubhp · 2022-01-03T18:11:58Z

Looks good to me, thanks for the contribution. Would you mind adding unit tests for RegisterModel & EstimatorTransformer step here just to confirm the repack_path is being wired through to _RepackModelStep?

sagemaker-bot · 2022-01-04T22:13:17Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T22:15:01Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T22:58:57Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T22:58:57Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T23:09:27Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T23:09:27Z

AWS CodeBuild CI Report

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

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

While writing the unit test for EstimatorTransformer with repack model and a custom output output_path, I discovered that sending an entry_point to EstimatorTransformer was raising an "unexpected keyword argument 'entry_point'" on Model.__init__. Using code from RegisterModel as a base, I removed the entry_point and other repack variables from kwargs. Also implemented unit tests for this case.

sagemaker-bot · 2022-01-04T23:37:04Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T23:37:04Z

AWS CodeBuild CI Report

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

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

tuliocasagrande · 2022-01-04T23:42:29Z

hey @staubhp, it was a great suggestion to add unit tests for RegisterModel & EstimatorTransformer.

I eventually discovered an issue with the entry_point parameter on EstimatorTransformer being propagated until Model and generating an error. We also didn't have unit tests for EstimatorTransformer with _RepackModelStep.

So I split the new changes in two commits:

Removed entry_point from kwargs after being used (similar to RegisterModel) and added unit test for this case: ee6afcf
Added unit tests for RegisterModel & EstimatorTransformer with custom repack output path: faf4ad5

Please let me know what you think. Thanks for reviewing!

codecov-commenter · 2022-01-14T22:35:23Z

Codecov Report

Merging #2804 (3a2491d) into dev (4325fcd) will decrease coverage by 1.14%.
The diff coverage is 80.48%.

❗ Current head 3a2491d differs from pull request most recent head ed9131b. Consider uploading reports for the commit ed9131b to get more accurate results

@@            Coverage Diff             @@
##              dev    #2804      +/-   ##
==========================================
- Coverage   89.82%   88.67%   -1.15%     
==========================================
  Files         196      173      -23     
  Lines       16564    15423    -1141     
==========================================
- Hits        14878    13677    -1201     
- Misses       1686     1746      +60

Impacted Files	Coverage Δ
src/sagemaker/workflow/_repack_model.py	`21.73% <0.00%> (-78.27%)`	⬇️
src/sagemaker/workflow/_utils.py	`85.36% <ø> (-2.54%)`	⬇️
src/sagemaker/workflow/step_collections.py	`92.55% <ø> (-6.39%)`	⬇️
src/sagemaker/dataset_definition/inputs.py	`100.00% <100.00%> (ø)`
src/sagemaker/workflow/steps.py	`96.72% <100.00%> (-1.10%)`	⬇️
src/sagemaker/workflow/utilities.py	`91.66% <100.00%> (-8.34%)`	⬇️
src/sagemaker/huggingface/model.py	`32.78% <0.00%> (-45.91%)`	⬇️
src/sagemaker/lineage/query.py	`75.72% <0.00%> (-19.25%)`	⬇️
... and 64 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4325fcd...ed9131b. Read the comment docs.

sagemaker-bot · 2022-01-14T22:42:29Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T23:29:09Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-14T23:33:49Z

AWS CodeBuild CI Report

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

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

eugeneteoh · 2022-02-08T21:42:57Z

src/sagemaker/workflow/step_collections.py

+            repack_model = True
+            entry_point = kwargs.pop("entry_point", None)
+            source_dir = kwargs.pop("source_dir", None)
+            dependencies = kwargs.pop("dependencies", None)


Suggested change

dependencies = kwargs.pop("dependencies", None)

dependencies = kwargs.pop("dependencies", None)

code_location = kwargs.pop("code_location", None)

eugeneteoh

https://github.com/aws/sagemaker-python-sdk/pull/2804/files#diff-043e8c79b0b9ccd41f8164a29ccc1f9e67f38e349aafffb96cd91762c3824bc7R364 (step_collections.py L364)
I suggest changing this to image_uri=image_uri. Using training_image_uri() for inference doesn't actually work.

eugeneteoh · 2022-02-08T21:44:49Z

src/sagemaker/workflow/step_collections.py

            repack_model_step = _RepackModelStep(
                name=f"{name}RepackModel",
                depends_on=depends_on,
                retry_policies=repack_model_step_retry_policies,
                sagemaker_session=estimator.sagemaker_session,
-                role=estimator.sagemaker_session,
+                role=estimator.role,
                model_data=model_data,
                entry_point=entry_point,
                source_dir=source_dir,


Suggested change

source_dir=source_dir,

source_dir=source_dir,

code_location=code_location,

eugeneteoh · 2022-02-08T21:57:22Z

src/sagemaker/workflow/step_collections.py

@@ -336,6 +349,8 @@ def __init__(
                security_group_ids=estimator.security_group_ids,
                description=description,
                display_name=display_name,
+                repack_output_path=repack_output_path,


Can you add a repack_image_uri argument? My customer is facing an issue where the python version SKLearn (3.7) estimator is different from the training image uri. That caused dependency errors when trying to install packages that are available on (for example) Python 3.8. Another option would be equating it to estimator.training_image_uri().

Suggested change

repack_output_path=repack_output_path,

repack_output_path=repack_output_path,

image_uri=estimator.training_image_uri(),

sagemaker-bot · 2022-05-13T14:35:07Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-13T14:35:54Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-13T15:01:15Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-13T15:20:58Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-05-13T15:33:59Z

AWS CodeBuild CI Report

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

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

qidewenwhen · 2022-08-05T22:04:22Z

Update: this recently released commit has made the repack step output path align with model repack path. In other words, users can use the code_location argument in model class to customize the output for RepackModelStep.

Thus, we can close this PR.

tuliocasagrande · 2022-08-06T18:13:37Z

Agree. Superseded by #3257.

staubhp and others added 4 commits December 15, 2021 10:45

fix: Set ProcessingStep upload locations deterministically to avoid c… (

dde8d00

aws#2790)

fix: Prevent repack_model script from referencing nonexistent directo…

0f72907

…ries (aws#2755) Co-authored-by: Payton Staub <[email protected]> Co-authored-by: Ahsan Khan <[email protected]>

fix: S3Input - add support for instance attributes (aws#2754)

0bae071

fix: typos and broken link (aws#2765)

17fe93e

Co-authored-by: Shreya Pandit <[email protected]>

tuliocasagrande force-pushed the repack_output branch from abc7cd5 to 7087c5a Compare December 17, 2021 20:30

tuliocasagrande marked this pull request as ready for review December 17, 2021 21:02

tuliocasagrande changed the title ~~Repack output~~ feature: Allow custom output for RepackModelStep Dec 17, 2021

feature: Add output path parameter for _RepackModelStep

f0efd27

Fixes aws#2803

tuliocasagrande force-pushed the repack_output branch from 7087c5a to 4643394 Compare December 17, 2021 21:05

mufaddal-rohawala force-pushed the dev branch from beeabbd to b026769 Compare January 4, 2022 17:09

fix: Fix role parameter for _RepackModelStep

7a1f4f8

tuliocasagrande force-pushed the repack_output branch from f5b4263 to 3a2491d Compare January 4, 2022 21:58

tuliocasagrande added 2 commits January 4, 2022 20:28

feature: Add tests for RegisterModel with repack output

faf4ad5

tuliocasagrande force-pushed the repack_output branch from 3a2491d to faf4ad5 Compare January 4, 2022 23:29

Merge branch 'dev' into repack_output

0489b59

shreyapandit force-pushed the dev branch from 903205c to b796a00 Compare January 18, 2022 20:09

BasilBeirouti force-pushed the dev branch from 814dd3d to 70308b1 Compare January 24, 2022 20:10

jeniyat force-pushed the dev branch 3 times, most recently from f5e6849 to 34b07c0 Compare January 29, 2022 23:28

mufaddal-rohawala force-pushed the dev branch 3 times, most recently from 96c01d7 to c437191 Compare February 5, 2022 00:08

jeniyat force-pushed the dev branch from 9d612a7 to bb7563f Compare February 7, 2022 23:40

eugeneteoh reviewed Feb 8, 2022

View reviewed changes

eugeneteoh suggested changes Feb 8, 2022

View reviewed changes

eugeneteoh mentioned this pull request Feb 8, 2022

fix: EstimatorTransformer entry point error #2905

Closed

9 tasks

navinsoni force-pushed the dev branch from 3e68e14 to 668359f Compare February 22, 2022 02:34

BasilBeirouti force-pushed the dev branch from 0e4fd55 to 26b984a Compare February 23, 2022 02:20

Merge remote-tracking branch 'upstream/dev' into repack_output

ed9131b

tuliocasagrande closed this Aug 6, 2022

tuliocasagrande deleted the repack_output branch August 6, 2022 18:13

	dependencies = kwargs.pop("dependencies", None)
	dependencies = kwargs.pop("dependencies", None)
	code_location = kwargs.pop("code_location", None)

	source_dir=source_dir,
	source_dir=source_dir,
	code_location=code_location,

	repack_output_path=repack_output_path,
	repack_output_path=repack_output_path,
	image_uri=estimator.training_image_uri(),

feature: Allow custom output for RepackModelStep #2804

feature: Allow custom output for RepackModelStep #2804

Uh oh!

Conversation

tuliocasagrande commented Dec 17, 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 Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

Uh oh!

tuliocasagrande commented Dec 17, 2021

Uh oh!

staubhp commented Jan 3, 2022

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

Uh oh!

tuliocasagrande commented Jan 4, 2022

Uh oh!

codecov-commenter commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jan 14, 2022

AWS CodeBuild CI Report

Uh oh!

eugeneteoh Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

eugeneteoh 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!

eugeneteoh Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

eugeneteoh Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented May 13, 2022

AWS CodeBuild CI Report

tuliocasagrande commented Dec 17, 2021 •

edited

Loading

codecov-commenter commented Jan 14, 2022 •

edited

Loading

eugeneteoh left a comment •

edited

Loading

qidewenwhen commented Aug 5, 2022 •

edited

Loading