Fix: Update Airflow prepare_framework Function to not use os.path.join #3393

GroovyDan · 2022-10-05T15:10:17Z

Issue #, if available:

Description of changes:
Using os.path.join in the prepare_framework function can cause an issue on windows. The sagemaker_submit_directory hyperparameter value gets incorrectly set with the current implementation. Since key should refer to an S3 key, we should be able to join on / instead. Specifically, this issue cropped up when I was using the aws-step-functions-data-science-sdk-python to orchestrate a training job. They are building out their training definition using the ariflow training configs which references this code. (See here: https://github.com/aws/aws-step-functions-data-science-sdk-python/blob/main/src/stepfunctions/steps/sagemaker.py#L111)

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

vikas0203

/bot run all

sagemaker-bot · 2022-10-19T20:27:56Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-19T20:52:20Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-19T21:05:50Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-19T21:26:01Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2022-10-19T23:31:10Z

Codecov Report

Merging #3393 (e6aab74) into master (9e8230c) will decrease coverage by 0.84%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #3393      +/-   ##
==========================================
- Coverage   89.66%   88.82%   -0.85%     
==========================================
  Files         872      205     -667     
  Lines       77968    19258   -58710     
==========================================
- Hits        69913    17106   -52807     
+ Misses       8055     2152    -5903

Impacted Files	Coverage Δ
src/sagemaker/workflow/airflow.py	`91.61% <50.00%> (ø)`
...ckages/sagemaker/model_monitor/monitoring_files.py
...lib/python3.8/site-packages/sagemaker/predictor.py
...ython3.10/site-packages/sagemaker/amazon/kmeans.py
...thon3.7/site-packages/sagemaker/jumpstart/types.py
...agemaker/async_inference/async_inference_config.py
...n3.10/site-packages/sagemaker/apiutils/__init__.py
...9/lib/python3.9/site-packages/sagemaker/clarify.py
...thon3.8/site-packages/sagemaker/jumpstart/utils.py
...ython3.9/site-packages/sagemaker/mxnet/defaults.py
... and 1068 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

sagemaker-bot · 2022-10-19T23:31:11Z

AWS CodeBuild CI Report

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

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

vikas0203

/bot run all

sagemaker-bot · 2022-10-26T00:37:46Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-26T00:38:08Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-26T00:48:57Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-26T01:13:11Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-10-26T01:49:51Z

AWS CodeBuild CI Report

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

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

Co-authored-by: Mufaddal Rohawala <muffi179@gmail.com>

Co-authored-by: Basil Beirouti <BasilBeirouti@gmail.com> Co-authored-by: Kalyani Nikure <110067132+knikure@users.noreply.github.com>

…aws#3438) Co-authored-by: Keshav Chandak <chakesh@amazon.com>

Co-authored-by: Navin Soni <navinns@amazon.com>

* Add DXB region * Remove change from neuron * Adding DXB to TF 2.1.0 and 2.1.1

Co-authored-by: Brock Wade <bwayde@amazon.com> Co-authored-by: Mufaddal Rohawala <89424143+mufaddal-rohawala@users.noreply.github.com>

…ws#3492)

mufaddal-rohawala · 2023-08-14T16:35:51Z

@GroovyDan apologies for late review on this PR, can you help rebase this PR with latest master?

GroovyDan · 2023-08-16T19:22:31Z

@mufaddal-rohawala looks like the issue this PR was fixing has already been fixed with the introduction of the s3.s3_path_join function. Closing this PR.

fix: do not use in airflow prepare_framework function

3a8e471

claytonparnell force-pushed the fix_airflow_prepare_framework_windows_bug branch from c0de21e to 3a8e471 Compare October 19, 2022 17:12

vikas0203 requested review from claytonparnell and mufaddal-rohawala October 19, 2022 20:20

vikas0203 reviewed Oct 19, 2022

View reviewed changes

Merge branch 'master' into fix_airflow_prepare_framework_windows_bug

e6aab74

vikas0203 reviewed Oct 26, 2022

View reviewed changes

Merge branch 'master' into fix_airflow_prepare_framework_windows_bug

d0d0bb6

vikas0203 requested review from a team, claytonparnell and knikure and removed request for claytonparnell, mufaddal-rohawala and a team November 23, 2022 20:37

vikas0203 assigned claytonparnell Nov 23, 2022

NivekNey and others added 15 commits December 2, 2022 12:48

fix: type hint of PySparkProcessor __init__ (aws#3297)

c97c467

fix: fix PySparkProcessor __init__ params type (aws#3354)

de58941

fix: Allow Py 3.7 for MMS Test Docker env (aws#3080)

41dd330

Co-authored-by: Mufaddal Rohawala <muffi179@gmail.com>

refactoring : using with statement (aws#3286)

1e23a3f

Update local_requirements.txt PyYAML version (aws#3095)

19efadf

Co-authored-by: Basil Beirouti <BasilBeirouti@gmail.com> Co-authored-by: Kalyani Nikure <110067132+knikure@users.noreply.github.com>

feature: Update TF 2.9 and TF 2.10 inference DLCs (aws#3465)

76f7782

feature: Added transform with monitoring pipeline step in transformer (…

fde0738

…aws#3438) Co-authored-by: Keshav Chandak <chakesh@amazon.com>

fix: Fix bug forcing uploaded tar to be named sourcedir (aws#3412)

7f9f3b0

feature: Add Code Owners file (aws#3503)

5d59767

Co-authored-by: Navin Soni <navinns@amazon.com>

prepare release v2.119.0

0f5cf18

update development version to v2.119.1.dev0

f1f0013

feature: Add DXB region to frameworks by DLC (aws#3387)

bb4b689

* Add DXB region * Remove change from neuron * Adding DXB to TF 2.1.0 and 2.1.1

fix: support idempotency for framework and spark processors (aws#3460)

b68bcd9

Co-authored-by: Brock Wade <bwayde@amazon.com> Co-authored-by: Mufaddal Rohawala <89424143+mufaddal-rohawala@users.noreply.github.com>

feature: Update registries with new region account number mappings. (a…

32969da

…ws#3492)

Merge branch 'master' into fix_airflow_prepare_framework_windows_bug

32bca2a

GroovyDan requested a review from a team as a code owner December 6, 2022 17:31

mufaddal-rohawala force-pushed the master branch from bae3b8b to d000bdc Compare December 20, 2022 00:31

mufaddal-rohawala assigned mufaddal-rohawala and unassigned claytonparnell Aug 14, 2023

GroovyDan closed this Aug 16, 2023

GroovyDan deleted the fix_airflow_prepare_framework_windows_bug branch August 16, 2023 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: Update Airflow prepare_framework Function to not use os.path.join #3393

Fix: Update Airflow prepare_framework Function to not use os.path.join #3393

Uh oh!

GroovyDan commented Oct 5, 2022

Uh oh!

vikas0203 left a comment

Uh oh!

sagemaker-bot commented Oct 19, 2022

Uh oh!

sagemaker-bot commented Oct 19, 2022

Uh oh!

sagemaker-bot commented Oct 19, 2022

Uh oh!

sagemaker-bot commented Oct 19, 2022

Uh oh!

codecov-commenter commented Oct 19, 2022 •

edited

Loading

Uh oh!

sagemaker-bot commented Oct 19, 2022

Uh oh!

vikas0203 left a comment

Uh oh!

sagemaker-bot commented Oct 26, 2022

Uh oh!

sagemaker-bot commented Oct 26, 2022

Uh oh!

sagemaker-bot commented Oct 26, 2022

Uh oh!

sagemaker-bot commented Oct 26, 2022

Uh oh!

sagemaker-bot commented Oct 26, 2022

Uh oh!

mufaddal-rohawala commented Aug 14, 2023

Uh oh!

GroovyDan commented Aug 16, 2023

Uh oh!

Uh oh!

Fix: Update Airflow prepare_framework Function to not use os.path.join #3393

Fix: Update Airflow prepare_framework Function to not use os.path.join #3393

Uh oh!

Conversation

GroovyDan commented Oct 5, 2022

Merge Checklist

General

Tests

Uh oh!

vikas0203 left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Oct 19, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 19, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 19, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 19, 2022

AWS CodeBuild CI Report

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Oct 19, 2022

AWS CodeBuild CI Report

Uh oh!

vikas0203 left a comment

Choose a reason for hiding this comment

Uh oh!

sagemaker-bot commented Oct 26, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 26, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 26, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 26, 2022

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Oct 26, 2022

AWS CodeBuild CI Report

Uh oh!

mufaddal-rohawala commented Aug 14, 2023

Uh oh!

GroovyDan commented Aug 16, 2023

Uh oh!

Uh oh!

codecov-commenter commented Oct 19, 2022 •

edited

Loading