Fix FrameworkProcessor for SageMaker Pipelines #2664
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Is this one needs to merged before #2564? |
|
hi, if this one is accepted, then #2564 is not needed. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -1265,9 +1266,8 @@ class FeatureStoreOutput(ApiObject): | |||
| class FrameworkProcessor(ScriptProcessor): | |||
| """Handles Amazon SageMaker processing tasks for jobs using a machine learning framework.""" | |||
|
|
|||
| framework_entrypoint_command = ["/bin/bash"] | |||
| framework_entrypoint_command = ["/bin/sh"] | |||
There was a problem hiding this comment.
Is this a breaking change?
There was a problem hiding this comment.
This change will somewhat restrict the available syntax for the framework script, but that's private logic (_generate_framework_script). It won't change anything about the execution environment which is still python per line 1346 (should this actually be changed to python3 maybe?). The main effect it will have on users is compatibility with different container images by standardising on POSIX-compliant sh rather than the more advanced bash shell which some OSs don't offer out-of-the-box (e.g. Alpine Linux or others mentioned here). While I'm aware of multiple distros that offer some sh implementation but not bash in particular, I've not found any that go the other way around.
From my investigations, Ubuntu (16+) is used as a base by all of the DLCs, the Scikit-Learn framework container, the XGBoost framework container, and even the Chainer container. As per here, Ubuntu has supported both /bin/sh and /bin/bash since at least v6.
The Spark container (which isn't actually FrameworkProcessor-based) seems to use Amazon Linux 2, which also offers /bin/sh.
Overall from what I can tell this is not breaking and from a compatibility standpoint should be a step forward rather than back?
There was a problem hiding this comment.
Thanks, this makes sense
| dependencies, | ||
| git_config, | ||
| job_name, | ||
| source_dir=source_dir, |
There was a problem hiding this comment.
Are these equals = necessary for the four arguments?
There was a problem hiding this comment.
Shouldn't be necessary in the sense that the order here matches the function declaration so it'd still work.
I did it for these because:
- They're defined as keyword args and the order didn't seem obvious as a user
- The fact that FrameworkProcessor inserts
source_dirinto the arg order defined by the parent, rather than only appending arguments in the child class, meant that being a bit more explicit could maybe help avoid confusion.
They can go back to positional if needed I'd say
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
^ slow-tests timed out again |
|
The only notable conflict in this merge was that somebody had tried to introduce Therefore to resolve the conflict I removed the new explicit I saw a weird error locally in |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Co-authored-by: Mufaddal Rohawala <[email protected]> Co-authored-by: Ahsan Khan <[email protected]> Co-authored-by: Mufaddal Rohawala <[email protected]> Co-authored-by: Basil Beirouti <[email protected]> Co-authored-by: Payton Staub <[email protected]> Co-authored-by: Shreya Pandit <[email protected]> Co-authored-by: Basil Beirouti <[email protected]> Co-authored-by: Payton Staub <[email protected]> Co-authored-by: Mohamed Ali Jamaoui <[email protected]> Co-authored-by: ci <ci> Co-authored-by: Jeniya Tabassum <[email protected]> Co-authored-by: sreedes <[email protected]> Co-authored-by: Navin Soni <[email protected]> Co-authored-by: Miyoung <[email protected]> Co-authored-by: Ameen Khan <[email protected]> Co-authored-by: Zhankui Lu <[email protected]> Co-authored-by: Xiaoguang Chen <[email protected]> Co-authored-by: Jonathan Guinegagne <[email protected]> Co-authored-by: Zhankui Lu <[email protected]> Co-authored-by: Yifei Zhu <[email protected]> Co-authored-by: Qingzi-Lan <[email protected]> Co-authored-by: Navin Soni <[email protected]> Co-authored-by: marckarp <[email protected]> Co-authored-by: chenxy <[email protected]> Co-authored-by: Xinghan Chen <[email protected]> Co-authored-by: Tulio Casagrande <[email protected]> Co-authored-by: jerrypeng7773 <[email protected]> Co-authored-by: marckarp <[email protected]> Co-authored-by: jayatalr <[email protected]> Co-authored-by: bhaoz <[email protected]> Co-authored-by: Ethan Cheng <[email protected]> Co-authored-by: Xiaoguang Chen <[email protected]> Co-authored-by: keerthanvasist <[email protected]> Co-authored-by: Shreya Pandit <[email protected]> Co-authored-by: Ivy Bazan <[email protected]> Co-authored-by: IvyBazan <[email protected]> Co-authored-by: Benjamin Crabtree <[email protected]> Co-authored-by: iasoon <[email protected]> Co-authored-by: evakravi <[email protected]>
| @@ -74,6 +74,8 @@ | |||
| "2.6", | |||
| "2.6.0", | |||
| "2.6.2", | |||
| "2.7", | |||
| "2.7.0", | |||
There was a problem hiding this comment.
Has this changed since and we need to update it?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| @@ -2629,6 +2661,39 @@ | |||
| "us-west-2": "763104351884" | |||
| }, | |||
| "repository": "tensorflow-training" | |||
| }, | |||
| "2.7.0": { | |||
There was a problem hiding this comment.
DLC is yet to release these containers. Can we remove this and introduce this later when DLC officially makes them available?
| "registries": { | ||
| "af-south-1": "626614931356", | ||
| "ap-east-1": "871362719292", | ||
| "ap-northeast-1": "763104351884", |
There was a problem hiding this comment.
DLC is yet to release these containers. Can we remove this and introduce this later when DLC officially makes them available?
| @@ -279,7 +279,8 @@ | |||
| "2.3": "2.3.2", | |||
| "2.4": "2.4.3", | |||
| "2.5": "2.5.1", | |||
| "2.6": "2.6.0" | |||
| "2.6": "2.6.0", | |||
There was a problem hiding this comment.
Same as below, lets add this once TF 2.7 is out
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@athewsey can you help resolve the conflicts in this pr, by rebasing master on this. |
|
Hi, Do we know when is this planned to be rolled out / merged. |
vikas0203
requested review from
a team,
navinsoni and
knikure
and removed request for
a team
|
Closing as it's no longer reqd. |
|
why is this is not required any more, can we use script processor with code repo's now? |
|
There is a chain of merge requests which point to this MR as the one that should allow for passing additional modules/code to |
Issue #, if available: #2656, #2763
Description of changes:
A slimmed-down alternative to #2633, reducing unnecessary change to
ScriptProcessorto try and speed up acceptance so we can defer longer-term refactoring proposals to later.FrameworkProcessor-based processors' interactions with SageMaker PipelinesProcessingStep(which currently don't work together at all per the linked issue)source_dirviaProcessingStep, so Pipelines users can take full advantage of the FrameworkProcessor functionality.FrameworkProcessorto the underlying Framework Estimator./bin/bashto/bin/sh: Since some optimized, container-oriented Linux distributions (e.g. Alpine) may not include bash.code_locationparameter to ScriptProcessor (including SKLearnProcessor) as well, to enable users of these processors to avoid using the SageMaker default bucket.Testing done:
_normalize_argsas called by Pipelines should fully prepare aFrameworkProcessor)FrameworkProcessoron other frameworksMerge Checklist
Put an
xin 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
Tests
unique_name_from_baseto 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.