feature: add 1p algorithm image_uris migration tool #1792

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.

Sign up for GitHub

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

Jump to bottom

Merged

laurenyu merged 6 commits into aws:zwei from chuyang-deng:image-uris-migration

Aug 3, 2020

Contributor

chuyang-deng commented Jul 31, 2020

Issue #, if available:
#1478

Description of changes:
Add image_uris Refactor and Renamer to v2 migration tool

Testing done:
unit tests

Merge Checklist

Put an x in 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

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have passed the region in to all S3 and STS clients that I've initialized as part of this change.
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)
I have used unique_name_from_base to 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.

Chuyang Deng added 2 commits

July 30, 2020 10:20


          add migration tool for image_uris.retrieve

b34d680


          feature: add 1p algorithm image_uris migration tool

3f08e2e

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 3f08e2e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

chuyang-deng changed the title ~~Image uris migration~~ feature: add 1p algorithm image_uris migration tool

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 3f08e2e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

ajaykarpur previously approved these changes

View reviewed changes

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 3f08e2e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-zwei-pr
Commit ID: 3f08e2e
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu reviewed

View reviewed changes

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Outdated

Comment on lines 13 to 15

+              """Classes to modify image_uris.retrieve() code to be compatible
+              with version 2.0 and later of the SageMaker Python SDK.
+              """

Contributor

laurenyu Jul 31, 2020

(blame Eric) this should be a one-liner according to pep, apparently

Contributor Author

chuyang-deng Jul 31, 2020

updated.

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Outdated Show resolved Hide resolved


          update docstring and remove unused algorithm name

2d05070

chuyang-deng dismissed ajaykarpur’s stale review via

2d05070

July 31, 2020 21:31

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 2d05070
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu reviewed

View reviewed changes

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Outdated

+                      for kw in node.keywords:
+                          if kw.arg == "repo_name":
+                              arg = kw.value.s
+                              modified_arg = ALGORITHM_NAME_FROM_REPO[arg]

Contributor

laurenyu Jul 31, 2020

I don't think you need the dictionary at all - even for XGBoost as a framework, get_image_uri accepted just "xgboost": https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/amazon/amazon_estimator.py#L660

Contributor Author

chuyang-deng Aug 3, 2020

updated!

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Show resolved Hide resolved

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Outdated

Comment on lines 112 to 114

+                      if matching.matches_name(node, GET_IMAGE_URI_NAME) or matching.matches_attr(
+                          node, GET_IMAGE_URI_NAME
+                      ):

Contributor

laurenyu Jul 31, 2020

since this code shouldn't be called unless node_should_be_modified is True, is there a point to this check?

Contributor Author

chuyang-deng Aug 3, 2020

You are right.

src/sagemaker/cli/compatibility/v2/modifiers/image_uris.py Outdated

+                      if matching.matches_name(node, GET_IMAGE_URI_NAME) or matching.matches_attr(
+                          node, GET_IMAGE_URI_NAME
+                      ):
+                          node_components = list(pasta.dump(node).split("."))

Contributor

laurenyu Jul 31, 2020

using pasta to check the attributes strikes me as a bit odd

Contributor Author

chuyang-deng Aug 3, 2020

updated using func.value to check for "sagemaker"

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 2d05070
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Jul 31, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 2d05070
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 1, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-zwei-pr
Commit ID: 2d05070
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository


          remove framework mapping and use func.value to check for 'sagemaker'

b9899ec

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: b9899ec
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: b9899ec
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-zwei-pr
Commit ID: b9899ec
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: b9899ec
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository


          remove unnecessary check

9546f96

laurenyu approved these changes

View reviewed changes


          Merge branch 'zwei' into image-uris-migration

da5d048

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: 9546f96
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-unit-tests
Commit ID: da5d048
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: 9546f96
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-notebook-tests
Commit ID: da5d048
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: 9546f96
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-local-mode-tests
Commit ID: da5d048
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Collaborator

sagemaker-bot commented Aug 3, 2020

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-zwei-pr
Commit ID: da5d048
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

laurenyu merged commit 4fb16f2 into aws:zwei

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet