fix: repack_model script used in pipelines to support source_dir and dependencies #2645

staubhp · 2021-09-17T21:28:36Z

Issue #, if available:
N/A

Description of changes:
When creating a model, users can optionally specify a custom entry point as well as a source directory and a list of dependencies. These custom scripts are "repacked" into the model tarball by the SDK.

Because Pipelines execute outside of the context of the SDK, they add a "RepackModel" step when users are registering a model that uses script-mode. This repack step starts a training job that runs the _repack_model.py script which puts the custom scripts into the tarball.

Previously, this script only handled custom entry_points, allowing users to customize inference code but not allowing them to add custom dependencies. This change adds support for repacking the dependencies and source_dir args the same way that the SDK does.

Testing done:
Unit, manual.

Note that the code being tested is a training job script that works with absolute root filepaths. For that reason, the unit tests are not enabled because they will fail in most cases due to permissions to the /opt/ directory.

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.

…encies or source_dir arguments, which are used for script-mode in models

sagemaker-bot · 2021-09-17T21:55:30Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-17T22:53:39Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-17T23:48:52Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-18T00:04:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-18T00:27:15Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-18T00:42:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-19T06:19:50Z

AWS CodeBuild CI Report

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

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

danabens · 2021-09-20T18:25:06Z

src/sagemaker/workflow/_repack_model.py

+    Args:
+        inference_script (str): The path to the custom entry point.
+        model_archive (str): The name of the model TAR archive.
+        dependencies (str): A space-delimited string of paths to custom dependencies.


nit this description makes more sense on a cli argument than a method parameter

danabens · 2021-09-20T18:34:58Z

src/sagemaker/workflow/_repack_model.py

+                if os.path.isdir(actual_dependency_path):
+                    shutil.copytree(
+                        actual_dependency_path,
+                        os.path.join(lib_dir, os.path.basename(actual_dependency_path)),


why is os.path.join needed here but not on line 77?

os.path.join is being used for both calls, but on line 77 it's being done in the assignment of the s and d variables

danabens · 2021-09-20T18:36:33Z

src/sagemaker/workflow/_repack_model.py

+    parser.add_argument("--inference_script", type=str, default="inference.py")
+    parser.add_argument("--dependencies", type=str, default=None)
+    parser.add_argument("--source_dir", type=str, default=None)
+    parser.add_argument("--model_archive", type=str, default="model.tar.gz")


this also has a help param if that would benefit users

Ack. I don't think it would be much use since this script is intended for, and will only work in, a training job

danabens · 2021-09-20T18:39:17Z

tests/unit/sagemaker/workflow/test_repack_model_script.py

+    )
+
+    # repack
+    _repack_model.repack(inference_script="inference.py", model_archive=model_tar_name)


nice, would be nice to also cover __main__

One of the reasons I moved the logic to a function was because I couldn't figure out how to invoke the __main__ of a module from within python :) As far as I understand,__name__ is only set to __main__ if it's called from a command line. Suggestions welcome

sagemaker-bot · 2021-09-20T23:11:39Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-09-20T23:13:28Z

AWS CodeBuild CI Report

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

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

src/sagemaker/workflow/_repack_model.py

sagemaker-bot · 2021-10-12T01:56:29Z

AWS CodeBuild CI Report

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

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

…dependencies (aws#2645) Co-authored-by: Payton Staub <[email protected]> Co-authored-by: Shreya Pandit <[email protected]> Co-authored-by: Ahsan Khan <[email protected]> Co-authored-by: Jeniya Tabassum <[email protected]>

Payton Staub added 2 commits September 17, 2021 14:22

fix: fix _repack_model script used in pipelines not supporting depend…

5b16b4f

…encies or source_dir arguments, which are used for script-mode in models

Fix pylint

db68957

Fix unit tests

ecbc5e5

fix black-check

bf0899b

Payton Staub added 2 commits September 17, 2021 17:18

Fix unit tests

1326181

Fix black-check

70e22f5

Merge branch 'master' into fix-repack-model-pipelines

0d38b3a

staubhp requested a review from danabens September 20, 2021 17:07

danabens reviewed Sep 20, 2021

View reviewed changes

danabens previously approved these changes Sep 20, 2021

View reviewed changes

staubhp and others added 2 commits September 20, 2021 15:55

Merge branch 'aws:master' into fix-repack-model-pipelines

7ffed8c

Fix source_directory handling

c6c1f0a

staubhp dismissed danabens’s stale review via c6c1f0a September 20, 2021 23:02

shreyapandit marked this pull request as draft September 21, 2021 03:24

staubhp added 2 commits September 21, 2021 15:02

Merge branch 'master' into fix-repack-model-pipelines

2e469ab

Merge branch 'master' into fix-repack-model-pipelines

4bc1550

staubhp marked this pull request as ready for review September 24, 2021 18:07

Merge branch 'master' into fix-repack-model-pipelines

2dedfd3

danabens previously approved these changes Oct 8, 2021

View reviewed changes

jeniyat suggested changes Oct 8, 2021

View reviewed changes

src/sagemaker/workflow/_repack_model.py Outdated Show resolved Hide resolved

Remove commented code

82d795e

staubhp dismissed danabens’s stale review via 82d795e October 8, 2021 17:33

staubhp and others added 2 commits October 11, 2021 09:30

Merge branch 'master' into fix-repack-model-pipelines

558cdfe

Merge branch 'master' into fix-repack-model-pipelines

e472fbe

jeniyat approved these changes Oct 12, 2021

View reviewed changes

danabens approved these changes Oct 12, 2021

View reviewed changes

shreyapandit changed the title ~~fix: fix _repack_model script used in pipelines not supporting depend…~~ fix: repack_model script used in pipelines to support source_dir and dependencies Oct 12, 2021

ahsan-z-khan approved these changes Oct 12, 2021

View reviewed changes

ahsan-z-khan merged commit 50840db into aws:master Oct 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: repack_model script used in pipelines to support source_dir and dependencies #2645

fix: repack_model script used in pipelines to support source_dir and dependencies #2645

staubhp commented Sep 17, 2021 •

edited by shreyapandit

Loading

sagemaker-bot commented Sep 17, 2021

sagemaker-bot commented Sep 17, 2021

sagemaker-bot commented Sep 17, 2021

sagemaker-bot commented Sep 18, 2021

sagemaker-bot commented Sep 18, 2021

sagemaker-bot commented Sep 18, 2021

sagemaker-bot commented Sep 19, 2021

danabens Sep 20, 2021

danabens Sep 20, 2021

staubhp Sep 20, 2021

danabens Sep 20, 2021

staubhp Sep 20, 2021

danabens Sep 20, 2021

staubhp Sep 20, 2021

sagemaker-bot commented Sep 20, 2021

sagemaker-bot commented Sep 20, 2021

sagemaker-bot commented Oct 12, 2021

fix: repack_model script used in pipelines to support source_dir and dependencies #2645

fix: repack_model script used in pipelines to support source_dir and dependencies #2645

Conversation

staubhp commented Sep 17, 2021 • edited by shreyapandit Loading

Merge Checklist

General

Tests

sagemaker-bot commented Sep 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 18, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 18, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 18, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 19, 2021

AWS CodeBuild CI Report

danabens Sep 20, 2021

Choose a reason for hiding this comment

danabens Sep 20, 2021

Choose a reason for hiding this comment

staubhp Sep 20, 2021

Choose a reason for hiding this comment

danabens Sep 20, 2021

Choose a reason for hiding this comment

staubhp Sep 20, 2021

Choose a reason for hiding this comment

danabens Sep 20, 2021

Choose a reason for hiding this comment

staubhp Sep 20, 2021

Choose a reason for hiding this comment

sagemaker-bot commented Sep 20, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Sep 20, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Oct 12, 2021

AWS CodeBuild CI Report

staubhp commented Sep 17, 2021 •

edited by shreyapandit

Loading