fix: local mode deletion of temp files #2644

jmahlik · 2021-09-16T20:03:16Z

Issue #, if available:

Description of changes:

Add try except around file cleanup in sagemaker.local.utils.copy_directory_structure. This was causing local mode to fail when docker (running as root) writes files to a mounted dir and the user does not have permission to remove the files.

The same pattern is followed here.

Testing done:

Add test for failed removal, behaves as expected.

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.

src/sagemaker/local/utils.py

shreyapandit

Comments inline, otherwise looks good. Thanks for the fix!

sagemaker-bot · 2021-09-22T14:03:28Z

AWS CodeBuild CI Report

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

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

jmahlik · 2021-11-03T22:22:47Z

Changes made and rebased master if someone would like to give a review.

sagemaker-bot · 2021-11-03T22:37:11Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2021-11-03T23:06:32Z

Codecov Report

Merging #2644 (a601198) into dev (bb7563f) will decrease coverage by 0.52%.
The diff coverage is 92.00%.

❗ Current head a601198 differs from pull request most recent head 8adc449. Consider uploading reports for the commit 8adc449 to get more accurate results

@@            Coverage Diff             @@
##              dev    #2644      +/-   ##
==========================================
- Coverage   89.49%   88.96%   -0.53%     
==========================================
  Files         195      178      -17     
  Lines       16567    15764     -803     
==========================================
- Hits        14826    14025     -801     
+ Misses       1741     1739       -2

Impacted Files	Coverage Δ
src/sagemaker/local/utils.py	`90.74% <80.00%> (-2.60%)`	⬇️
src/sagemaker/clarify.py	`92.91% <100.00%> (-0.09%)`	⬇️
src/sagemaker/lineage/context.py	`81.98% <100.00%> (ø)`
src/sagemaker/huggingface/model.py	`32.78% <0.00%> (-45.91%)`	⬇️
src/sagemaker/huggingface/estimator.py	`88.37% <0.00%> (-3.49%)`	⬇️
src/sagemaker/model.py	`89.05% <0.00%> (-2.36%)`	⬇️
src/sagemaker/tensorflow/model.py	`89.13% <0.00%> (-1.09%)`	⬇️
src/sagemaker/workflow/steps.py	`96.78% <0.00%> (-0.56%)`	⬇️
... and 31 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 bb7563f...8adc449. Read the comment docs.

sagemaker-bot · 2021-11-03T23:07:26Z

AWS CodeBuild CI Report

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

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

marcelgwerder · 2021-12-02T15:37:40Z

@shreyapandit We're facing the same issue, have been for a while. My understanding is that this fix is already mostly reviewed. Anything that still needs to be done or can this be merged?

EmadGohariTR · 2021-12-02T16:59:13Z

I am also facing this issue, can this PR be merged or any other work is required?

jmahlik · 2021-12-06T16:59:20Z

@shreyapandit We're facing the same issue, have been for a while. My understanding is that this fix is already mostly reviewed. Anything that still needs to be done or can this be merged?

This should be good to go, all the comments have been addressed. I can rebase again if needed but it seems alright to me. We are currently manually applying this patch to our installs.

diegodebrito · 2021-12-07T18:53:56Z

This would be very useful, and it's a simple fix that seems ready to be merged.

sagemaker-bot · 2021-12-17T05:10:12Z

AWS CodeBuild CI Report

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

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

jmahlik · 2021-12-17T18:24:51Z

Messed up the rebase with a trailing / . Should be good now.

sagemaker-bot · 2021-12-17T18:38:18Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-12-17T19:18:40Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2021-12-17T19:31:24Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-03T15:35:14Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-04T15:42:59Z

AWS CodeBuild CI Report

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

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

jmahlik · 2022-01-14T02:38:50Z

Some unrelated ones are still failing. Probably ok to merge?

FAILED tests/integ/test_model_quality_monitor.py::test_run_model_quality_monitor_baseline

There's a large number of data scientists internally experiencing the issue resolved in this PR when testing out new code.

ahsan-z-khan

@jmahlik Can you resolve the conflicts?

jmahlik · 2022-01-24T14:09:18Z

@jmahlik Can you resolve the conflicts?

rebased

sagemaker-bot · 2022-01-24T14:28:52Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-24T15:23:29Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-24T15:47:00Z

AWS CodeBuild CI Report

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

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

jmahlik · 2022-01-26T18:03:42Z

@jmahlik Can you resolve the conflicts?

rebased

It seems like the changes keep getting out of sync due to force pushes on dev.

Trying merging now to hopefully allow these changes to not stack up. We aren't able to turn off the org setting to allow maintainers to update the branch. If someone wants to just take this code and submit another PR that can be edited by the maintainers, feel free.

sagemaker-bot · 2022-01-26T18:23:46Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-26T19:16:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-01-26T19:50:27Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-02-10T22:03:41Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2022-02-10T22:44:03Z

AWS CodeBuild CI Report

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

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

jmahlik mentioned this pull request Sep 21, 2021

local mode permission denied when file is written from running job to mounted volume #2647

Closed

shreyapandit reviewed Sep 21, 2021

View reviewed changes

src/sagemaker/local/utils.py Outdated Show resolved Hide resolved

shreyapandit suggested changes Sep 21, 2021

View reviewed changes

shreyapandit marked this pull request as draft September 21, 2021 20:05

jmahlik force-pushed the fix-local-mode-root-files branch from b4e5139 to c3cdfa6 Compare September 22, 2021 13:45

jmahlik marked this pull request as ready for review September 22, 2021 13:46

jmahlik requested a review from shreyapandit September 23, 2021 19:41

jmahlik force-pushed the fix-local-mode-root-files branch from c3cdfa6 to 50b52b4 Compare November 3, 2021 22:20

jmahlik force-pushed the fix-local-mode-root-files branch from 50b52b4 to 1520cb2 Compare December 17, 2021 04:50

jmahlik force-pushed the fix-local-mode-root-files branch from 1520cb2 to 1a533e8 Compare December 17, 2021 18:24

navinsoni changed the base branch from master to dev December 30, 2021 00:32

mufaddal-rohawala requested a review from jeniyat January 3, 2022 21:09

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

jmahlik force-pushed the fix-local-mode-root-files branch from 4c3ec7d to 9426d56 Compare January 5, 2022 18:36

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

ahsan-z-khan suggested changes Jan 22, 2022

View reviewed changes

jmahlik force-pushed the fix-local-mode-root-files branch from 799ff53 to 85db47c Compare January 24, 2022 14:08

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 removed their request for review February 7, 2022 22:11

jeniyat added the needs contributor changes label Feb 7, 2022

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

fix: local mode deletion of temp files

8adc449

jmahlik force-pushed the fix-local-mode-root-files branch from a601198 to 8adc449 Compare February 10, 2022 21:44

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

jmahlik mentioned this pull request Mar 22, 2022

fix: local mode deletion of temp files on job end #3017

Merged

9 tasks

jmahlik closed this Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: local mode deletion of temp files #2644

fix: local mode deletion of temp files #2644

jmahlik commented Sep 16, 2021

shreyapandit left a comment

sagemaker-bot commented Sep 22, 2021

jmahlik commented Nov 3, 2021

sagemaker-bot commented Nov 3, 2021

codecov-commenter commented Nov 3, 2021 •

edited

Loading

sagemaker-bot commented Nov 3, 2021

marcelgwerder commented Dec 2, 2021

EmadGohariTR commented Dec 2, 2021

jmahlik commented Dec 6, 2021

diegodebrito commented Dec 7, 2021

sagemaker-bot commented Dec 17, 2021

jmahlik commented Dec 17, 2021

sagemaker-bot commented Dec 17, 2021

sagemaker-bot commented Dec 17, 2021

sagemaker-bot commented Dec 17, 2021

sagemaker-bot commented Jan 3, 2022

sagemaker-bot commented Jan 4, 2022

jmahlik commented Jan 14, 2022

ahsan-z-khan left a comment

jmahlik commented Jan 24, 2022

sagemaker-bot commented Jan 24, 2022

sagemaker-bot commented Jan 24, 2022

sagemaker-bot commented Jan 24, 2022

jmahlik commented Jan 26, 2022 •

edited

Loading

sagemaker-bot commented Jan 26, 2022

sagemaker-bot commented Jan 26, 2022

sagemaker-bot commented Jan 26, 2022

sagemaker-bot commented Feb 10, 2022

sagemaker-bot commented Feb 10, 2022

fix: local mode deletion of temp files #2644

fix: local mode deletion of temp files #2644

Conversation

jmahlik commented Sep 16, 2021

Merge Checklist

General

Tests

shreyapandit left a comment

Choose a reason for hiding this comment

sagemaker-bot commented Sep 22, 2021

AWS CodeBuild CI Report

jmahlik commented Nov 3, 2021

sagemaker-bot commented Nov 3, 2021

AWS CodeBuild CI Report

codecov-commenter commented Nov 3, 2021 • edited Loading

Codecov Report

sagemaker-bot commented Nov 3, 2021

AWS CodeBuild CI Report

marcelgwerder commented Dec 2, 2021

EmadGohariTR commented Dec 2, 2021

jmahlik commented Dec 6, 2021

diegodebrito commented Dec 7, 2021

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

jmahlik commented Dec 17, 2021

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Dec 17, 2021

AWS CodeBuild CI Report

sagemaker-bot commented Jan 3, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jan 4, 2022

AWS CodeBuild CI Report

jmahlik commented Jan 14, 2022

ahsan-z-khan left a comment

Choose a reason for hiding this comment

jmahlik commented Jan 24, 2022

sagemaker-bot commented Jan 24, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jan 24, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jan 24, 2022

AWS CodeBuild CI Report

jmahlik commented Jan 26, 2022 • edited Loading

sagemaker-bot commented Jan 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jan 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Jan 26, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Feb 10, 2022

AWS CodeBuild CI Report

sagemaker-bot commented Feb 10, 2022

AWS CodeBuild CI Report

codecov-commenter commented Nov 3, 2021 •

edited

Loading

jmahlik commented Jan 26, 2022 •

edited

Loading