sagemaker-python-sdk icon indicating copy to clipboard operation
sagemaker-python-sdk copied to clipboard

Published 20 hours ago • verified publisher icon aws

Fix FrameworkProcessor for SageMaker Pipelines

Open athewsey opened this issue 2 years ago • 42 comments

Issue #, if available: #2656, #2763

Description of changes:

A slimmed-down alternative to #2633, reducing unnecessary change to ScriptProcessor to try and speed up acceptance so we can defer longer-term refactoring proposals to later.

  • Fix FrameworkProcessor-based processors' interactions with SageMaker Pipelines ProcessingStep (which currently don't work together at all per the linked issue)
  • Enable source_dir via ProcessingStep, so Pipelines users can take full advantage of the FrameworkProcessor functionality.
  • Fix pass-through of KMS configuration from FrameworkProcessor to the underlying Framework Estimator.
  • Re-align FrameworkProcessor from /bin/bash to /bin/sh: Since some optimized, container-oriented Linux distributions (e.g. Alpine) may not include bash.
  • Update the dev guide docs, which were never updated to include the FrameworkProcessor classes in API tables of contents or mention available frameworks in the overview.
  • (2022-01-27 update) Add code_location parameter to ScriptProcessor (including SKLearnProcessor) as well, to enable users of these processors to avoid using the SageMaker default bucket.

Testing done:

  • Added unit test to prove the fix (that _normalize_args as called by Pipelines should fully prepare a FrameworkProcessor)
  • Adjusted unit tests to further exercise FrameworkProcessor on other frameworks
  • Adjusted pipelines integration test for FrameworkProcessor to actually check the job is able to run and succeed.

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

  • [X] I have read the CONTRIBUTING doc
  • [X] 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
  • [X] I used the commit message format described in CONTRIBUTING
  • [X] I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
  • [X] I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

  • [X] I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • [X] I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
  • [X] I have checked that my tests are not configured for a specific region or account (if appropriate)
  • [X] 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.

athewsey avatar Sep 24 '21 19:09 athewsey

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Sep 24 '21 20:09 sagemaker-bot

Is this one needs to merged before #2564?

ahsan-z-khan avatar Oct 21 '21 17:10 ahsan-z-khan

hi, if this one is accepted, then #2564 is not needed.

verdimrc avatar Oct 21 '21 17:10 verdimrc

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Oct 21 '21 17:10 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Oct 22 '21 03:10 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Oct 22 '21 04:10 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Oct 22 '21 06:10 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Oct 22 '21 07:10 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Nov 02 '21 03:11 sagemaker-bot

-slow-tests timed out. -pr looks like an unrelated failure that predictor.delete_model() in test_kmeans should probably be in a try/except in case of timeout?

athewsey avatar Nov 03 '21 07:11 athewsey

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Nov 03 '21 08:11 sagemaker-bot

Codecov Report

Merging #2664 (123a8ca) into dev (21ac6fd) will increase coverage by 0.02%. The diff coverage is 95.91%.

:exclamation: Current head 123a8ca differs from pull request most recent head 6bc0d80. Consider uploading reports for the commit 6bc0d80 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #2664      +/-   ##
==========================================
+ Coverage   89.47%   89.50%   +0.02%     
==========================================
  Files         196      195       -1     
  Lines       16584    16581       -3     
==========================================
+ Hits        14839    14840       +1     
+ Misses       1745     1741       -4
Impacted Files Coverage Δ
src/sagemaker/fw_utils.py 90.00% <ø> (ø)
src/sagemaker/huggingface/processing.py 100.00% <ø> (ø)
src/sagemaker/mxnet/processing.py 100.00% <ø> (ø)
src/sagemaker/pytorch/processing.py 100.00% <ø> (ø)
src/sagemaker/sklearn/processing.py 100.00% <ø> (ø)
src/sagemaker/tensorflow/processing.py 100.00% <ø> (ø)
src/sagemaker/xgboost/processing.py 100.00% <ø> (ø)
src/sagemaker/processing.py 96.47% <95.55%> (+0.39%) :arrow_up:
src/sagemaker/workflow/steps.py 97.36% <100.00%> (+0.01%) :arrow_up:
src/sagemaker/clarify.py 92.59% <0.00%> (-0.42%) :arrow_down:
... and 10 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 21ac6fd...6bc0d80. Read the comment docs.

codecov-commenter avatar Nov 24 '21 09:11 codecov-commenter

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Nov 24 '21 09:11 sagemaker-bot

^ slow-tests timed out again

athewsey avatar Nov 25 '21 02:11 athewsey

The only notable conflict in this merge was that somebody had tried to introduce kms_key argument explicitly through ProcessingStep, whereas this PR was already proposing to allow **kwargs to pass through open-ended arguments to the Processor (which would have included support for kms_key).

Therefore to resolve the conflict I removed the new explicit kms_key argument to prevent inconsistency and bloat in the interface. This means users would still be able to pass kms_key in, but no longer be able to access the property step.kms_key (should have step.processing_kwargs["kms_key"] if they really needed it). If this is too much change (reviewers?) then I guess we'll have to go back to having both an explicit kms_key argument and kwargs support for all the others 🤢

I saw a weird error locally in tests/integ/test_workflow_with_clarify.py (execution.describe() only found one step instead of 2?) but hoping it's just an environment issue as don't think this merge should have affected it... Will see what CI finds.

athewsey avatar Jan 27 '22 11:01 athewsey

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 12:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 12:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 13:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 16:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 16:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 17:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 17:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 17:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 17:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 27 '22 17:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 31 '22 17:01 sagemaker-bot

The only notable conflict in this merge was that somebody had tried to introduce kms_key argument explicitly through ProcessingStep, whereas this PR was already proposing to allow **kwargs to pass through open-ended arguments to the Processor (which would have included support for kms_key).

Therefore to resolve the conflict I removed the new explicit kms_key argument to prevent inconsistency and bloat in the interface. This means users would still be able to pass kms_key in, but no longer be able to access the property step.kms_key (should have step.processing_kwargs["kms_key"] if they really needed it). If this is too much change (reviewers?) then I guess we'll have to go back to having both an explicit kms_key argument and kwargs support for all the others 🤢

I saw a weird error locally in tests/integ/test_workflow_with_clarify.py (execution.describe() only found one step instead of 2?) but hoping it's just an environment issue as don't think this merge should have affected it... Will see what CI finds.

I agree with keeping the interface clean and allowing this to be passed through kwargs if everything else also is atm, lets check in with @jayatalr to ensure nothing breaks on the pipelines end of things

shreyapandit avatar Jan 31 '22 17:01 shreyapandit

I see a bunch of other conflicts when I try to rebase with the latest dev branch, @athewsey can you please look into those? The main files with issues are steps.py, workflow.py and related tests. I gave a shot at resolving but you are the best judge of the changes that went in there. Once the file diff is a little cleaner, happy to re-review this again

shreyapandit avatar Jan 31 '22 18:01 shreyapandit

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 31 '22 18:01 sagemaker-bot

AWS CodeBuild CI Report

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

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

sagemaker-bot avatar Jan 31 '22 19:01 sagemaker-bot