fix: allow output_path without trailing slash in Local Mode training jobs #1439
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 |
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 |
| str: an S3 prefix of the form ``"path/job_name"`` | ||
| """ | ||
| path = path.strip("/") | ||
| return job_name if path == "" else "/".join((path, job_name)) |
There was a problem hiding this comment.
Just curious to know instead of doing string manipulation, wouldn't os.path.join or pathlib.Path a better options for correctly making unified paths for local and non-local modes?
There was a problem hiding this comment.
it would, except that S3 always uses "/" as a separator, so using os.path.join would fail for Windows users.
as far as I can tell, there's not an easy way of overriding the separator used in os.path.join (I wrote a little about this in #1435). The other option would be to use something like urlunparse, but I think it might be a little heavy-handed for this use-case.
edit: looking into pathlib
There was a problem hiding this comment.
PurePosixPath does actually look to solve the problem, except that it was introduced in Python 3.4 and never backported to Python 2.7. since this SDK still currently supports Python 2.7, we can't use it here either. however, I'll make a note in our internal backlog to use PurePosixPath after we do get around to dropping Python 2.7 support. Thanks for sharing that! We've had quite a few conversations on our team around finding a module that would work no matter the OS, but hadn't yet found pathlib.
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 |
Issue #, if available:
#1354
Description of changes:
Local Mode's logic for constructing the final output path assumes that the estimator's
output_pathended with a slash ("/"), which deviates from SageMaker's behavior (see #1354).Testing done:
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.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.