change: download_and_extract local tar file #194
Conversation
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 |
mvsusp
left a comment
mvsusp
left a comment
There was a problem hiding this comment.
Let's write a functional tests for this feature as well.
src/sagemaker_containers/_files.py
Outdated
| if os.path.exists(path): | ||
| shutil.rmtree(path) | ||
| shutil.move(uri, path) | ||
| elif os.path.exists(uri) and tarfile.is_tarfile(uri): |
There was a problem hiding this comment.
You don't need to check if the exists because line 118 creates the path if it does not exist.
There was a problem hiding this comment.
line 118 checks for the path, not the uri. We should probably move the os.path.exists(uri) check before line 135, but after the s3 check on line 122.
…e exists in download_and_extract
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 |
|
The above tests that failed in the most recent build all pass locally on my machine. Going to root cause the failure |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| data = '\n'.join(_file.data) | ||
| f.write(data) | ||
| tar.add(name=name, arcname=_file.name) | ||
| os.remove(name) |
There was a problem hiding this comment.
You dont need to delete the file here because everything is being written inside the tmp dir anyways.
There was a problem hiding this comment.
We need this because that tmpdir gets deleted when the function returns if you use a tmpdir for the tar file location. We use the contents of the tar file for the test functional/test_download_and_impor.py::test_import_module_with_local_tar_via_download_and_extract so in that use case, we are using the current directory (not tmpdir)
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
3 participants
Description of changes:
Merge 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.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.