Update byo integ test to use sagemaker upload_data method #341
Conversation
There was a problem hiding this comment.
can you also make this change in the BYO hyperparameter tuning test? https://github.com/aws/sagemaker-python-sdk/blob/master/tests/integ/test_tuner.py#L360
| s3_train_data = 's3://{}/{}/train/{}'.format(bucket, prefix, key) | ||
|
|
||
| s3_train_data = sagemaker_session.upload_data(path=training_data_path, | ||
| key_prefix=os.path.join(prefix, 'train', key)) |
There was a problem hiding this comment.
since key_prefix is for the S3 path, you don't have to use os.path.join
There was a problem hiding this comment.
Did I understand it incorrectly? Should I just put everything in the uri after bucket for this arg?
There was a problem hiding this comment.
no you have the right thing for the arg, but because S3 always uses '/' regardless of the OS you're running on, you don't need to os.path.join - you can just do '{}/train/{}'.format(prefix, key) or whatever
There was a problem hiding this comment.
Oh got it.
I actually just copied codes from old lines using os.join. I guess it's not very different? Do I need to send another PR?
There was a problem hiding this comment.
afaik, it'll fail only on Windows, and I'm sure this isn't the only place where we have this issue. It should probably be fixed when we fix our Windows support.
Issue #, if available:
Description of changes:
1, Use sagemaker api instead of boto api. This reduces the credential error we got from canary test.
2, Add a dummy tensor binary data file for byo integ test to upload to s3. No need to generate it all the time.
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.