feature: allow setting the default bucket in Session #1176
Conversation
Default bucket not created on init.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
src/sagemaker/session.py
Outdated
| default_bucket = self._desired_default_bucket_name | ||
| region = self.boto_session.region_name | ||
| account = self.boto_session.client( | ||
| "sts", region_name=region, endpoint_url=sts_regional_endpoint(region) | ||
| ).get_caller_identity()["Account"] | ||
| default_bucket = "sagemaker-{}-{}".format(region, account) | ||
|
|
||
| if not default_bucket: | ||
| account = self.boto_session.client( | ||
| "sts", region_name=region, endpoint_url=sts_regional_endpoint(region) | ||
| ).get_caller_identity()["Account"] | ||
| default_bucket = "sagemaker-{}-{}".format(region, account) |
There was a problem hiding this comment.
nit: I'd move l. 334 to be right before the if statement because it's clearer that's it's basically just an if/else there. Or just make it an if/else:
if self._desired_default_bucket_name
default_bucket = self._desired_default_bucket_name
else
...There was a problem hiding this comment.
I moved the line closer, but I like to avoid ifelses when possible.
I think it's more readable to have a default case get overridden under specific circumstances. Also guarantees that the variable always gets set.
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 |
| :func:`default_bucket`). | ||
| If not provided, a default bucket will be created based on the following format: | ||
| "sagemaker-{region}-{aws-account-id}". | ||
| Example: "sagemaker-my-custom-bucket". |
There was a problem hiding this comment.
nit: if you want the "example" block to be formatted, you need a newline before it. (if you were going for having it in the same paragraph, then ignore this comment.)
There was a problem hiding this comment.
Gotcha. I actually want this example to match default_bucket's param precisely.
| self._initialize( | ||
| boto_session=boto_session, | ||
| sagemaker_client=sagemaker_client, | ||
| sagemaker_runtime_client=sagemaker_runtime_client, | ||
| ) |
There was a problem hiding this comment.
guessing black-format will put this back in one line?
There was a problem hiding this comment.
Don't think so? Based on my git hooks, I wouldn't have been able to commit it if black-check didn't return 1.
Unit tests also passed.
Am I misunderstanding?
Edit: return 0 *
There was a problem hiding this comment.
never mind. it looked strange to me that there should be any diff remaining in this part of the code, and my understanding was that black had a strict threshold for when it moved to multiple lines (for these kinds of cases)
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 |
Default bucket not created on init.
Issue #, if available: #1174
Description of changes: Identical to previous change except default bucket is no longer created on init.
Testing done: Shored up integ tests to automatically catch this in the future.
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.