Added region name to the sagemaker deploy cli. #24
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
==========================================
- Coverage 69.6% 69.58% -0.03%
==========================================
Files 44 44
Lines 2527 2528 +1
==========================================
Hits 1759 1759
- Misses 768 769 +1
Continue to review full report at Codecov.
|
| @@ -57,7 +57,7 @@ def _deploy(role, container_name, app_name, model_s3_path, run_id): | |||
| :param run_id: | |||
| :return: | |||
| """ | |||
| sage_client = boto3.client('sagemaker', region_name="us-west-2") | |||
| sage_client = boto3.client('sagemaker', region_name) | |||
There was a problem hiding this comment.
Are you sure the second argument is region_name? This documentation would seem to indicate that everything but the name is a **kwargs, so it would need to be named -- i.e., region_name= region_name.
There was a problem hiding this comment.
Ah -- I see it now, it is actually the second argument from here. Since it has a default value of None, would you mind adding the explicit region_name= region_name anyway, just to be 100% obvious?
|
This LGTM. Were you able to manually QA it in a particular non-us-west-2 region? |
|
Also could you confirm the reason for this change? I assume sagemaker will deploy to the region you specify in boto (as opposed to something like S3 which is largely global, and so regional endpoints are of less use)? |
yep tested it on eu-west-1
to run it outside of us-west-2, the data I wanted to use is in eu-west-1
I hadn't thought of removing the region altogether but unless there's a good reason for it I prefer explicit config, especially when boto is involved as debugging permissions can be a faff |
|
OK - sounds good. Thanks for contribution, and the QA! Merging. |
No description provided.