Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BYOM Notebooks - Verbosity and other notes. #28

Merged
merged 12 commits into from
Nov 26, 2017
Merged

BYOM Notebooks - Verbosity and other notes. #28

merged 12 commits into from
Nov 26, 2017

Conversation

ragavvenkatesan
Copy link

Increased verbosity on all the notebooks so that there is a bit more detail on everything. Followed protocols in the checklist. Structured similar in style to other notebooks.

Copy link
Contributor

@djarpin djarpin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. It looks like the current kernel for both is Python 3 rather than conda_python3. Have you confirmed this works in a Notebook Instance without any installs? I'm not sure if the sagemaker library is available there by default.

},
"outputs": [],
"source": [
"role = '<your SageMaker execution role here>'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this to:

assumed_role = boto3.client('sts').get_caller_identity()['Arn']
role = re.sub(r'^(.+)sts::(\d+):assumed-role/(.+?)/.*$', r'\1iam::\2:role/\3', assumed_role)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I setup the conda_mxnet_py36 and conda_tf_py36 for both, I am not sure why that is not reflected here. Let me change and push again.

@@ -182,11 +279,16 @@
},
"outputs": [],
"source": [
"import sagemaker\n",
"\n",
"sagemaker.Session().delete_endpoint(predictor.endpoint)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to comment this out just so users don't hit "Run All" and then delete their endpoint before they've gotten a chance to experiment with it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@@ -19,13 +46,24 @@
},
"outputs": [],
"source": [
"role = <<Your_Sagemaker_Role>>"
"role = '<your SageMaker execution role here>'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update this to:

assumed_role = boto3.client('sts').get_caller_identity()['Arn']
role = re.sub(r'^(.+)sts::(\d+):assumed-role/(.+?)/.*$', r'\1iam::\2:role/\3', assumed_role)

Will also need to update metadata slightly then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed.

"shutil.rmtree('export')"
"shutil.rmtree('export')\n",
"\n",
"sagemaker.Session().delete_endpoint(predictor.endpoint)"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to comment this about, just to prevent users from hitting "Run All Cells" and accidentally deleting the endpoint before they've gotten a chance to experiment with it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

"role = <<Your_Sagemaker_Role>>"
"role = '<your SageMaker execution role here>'"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will want to add metadata to this cell of: "isConfigCell": true

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

"source": [
"role = '<your SageMaker execution role here>'"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will want to add metadata to this cell of: "isConfigCell": true

@ragavvenkatesan
Copy link
Author

All comments are accounted for, although there seems to be a potential new bug in the predict feeding pipeline to the SDK.

@djarpin djarpin merged commit d795f77 into master Nov 26, 2017
@djarpin djarpin deleted the byom-examples branch November 30, 2017 16:51
@igabr igabr mentioned this pull request Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants