fix: blacklist unknown xgboost image versions #1519

edwardjkim · 2020-05-21T07:09:59Z

Issue #, if available:

Description of changes:

When an unknown version is passed to the repo_version parameter of get_image_uri, the function prints a misleading warning and returns a completely wrong image URI.

print(get_image_uri('us-west-2', 'xgboost', '1.1-1'))

WARNING:root:There is a more up to date SageMaker XGBoost image. To use the newer image, please set 'repo_version'='1.0-1'. For example:
	get_image_uri(region, 'xgboost', '1.0-1').

'433757028032.dkr.ecr.us-west-2.amazonaws.com/xgboost:1.1-1'

Not only 1.1-1 is currently unavailable, the account 433757028032 and repository xgboost are for 1p xgboost, not OS XGBoost. This is an un undesired behavior.

Desired behavior:

Fix above behavior to return invalid version number with appropriate error message
If version is valid but not latest then above warning should be printed.
The container returned should be 1P XGBoost only when no version number is provided.

This PR changes get_image_uri() to blacklist unknown xgboost image versions and adds appropriate unit tests.

Testing done:

export IGNORE_COVERAGE=- ; tox -e black-format,flake8,pylint,py36 tests/unit/test_amazon_estimator.py; unset IGNORE_COVERAGE

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 any/all 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.

…into xgboost_get_image_uri

sagemaker-bot · 2020-05-21T07:23:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:29:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:38:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:43:59Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:44:27Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:50:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T07:55:10Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

edwardjkim · 2020-05-21T08:28:07Z

@balajitummala @aws-patlin Can you please review?

aws-patlin · 2020-05-21T16:52:20Z

src/sagemaker/amazon/amazon_estimator.py

+            if xgboost_version_matches:
+                # Assumes that XGBOOST_SUPPORTED_VERSION is sorted from oldest version to latest,
+                # and the latest version is at the end of the list.
+                repo_version = xgboost_version_matches[-1]


For backwards compatibility, I don't think this is the behavior we want. If a customer is setting "0.90" as the repo_version and was previously getting the 0.90-1 image, we don't want to suddenly give them the 0.90-2 image.

I think you are right. We don't want to change 0.90 -> 0.90-2 if customers were getting 0.90-1 previously. Will update.

aws-patlin · 2020-05-21T16:57:25Z

tests/unit/test_amazon_estimator.py

+        == "246618743249.dkr.ecr.us-west-2.amazonaws.com/sagemaker-xgboost:0.90-2-cpu-py3"
+    )
+    assert (
+        get_image_uri(REGION, "xgboost", "0.90")


Once again, we were previously returning the 0.90-1 image for this. Let's check with Rahul on whether or not we want this new behavior.

aws-patlin · 2020-05-21T17:08:55Z

tests/unit/test_amazon_estimator.py

    assert _is_latest_xgboost_version("0.90-1-cpu-py3") is False
-
+    assert _is_latest_xgboost_version("0.90-2-cpu-py3") is False


Can we just perform a loop over the supported versions, excluding the last entry? That way we don't need to add a new line each time there's a new version.

sagemaker-bot · 2020-05-21T21:22:02Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T21:26:31Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T21:31:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

aws-patlin · 2020-05-21T21:40:09Z

Looks like there's a formatting issue in one of the files. You'll need to run black to fix it.

sagemaker-bot · 2020-05-21T21:43:58Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T21:44:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T21:54:39Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T21:59:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T23:18:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T23:27:16Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-21T23:40:06Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-22T00:07:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

edwardjkim · 2020-05-22T00:11:36Z

Thanks! @aws-patlin

laurenyu · 2020-05-22T00:12:52Z

looks like @aws-patlin approved it earlier. let me know if you want any other approvals; otherwise, I'll treat it as ready to be merged in.

sagemaker-bot · 2020-05-22T00:25:20Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-22T00:46:43Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-22T00:48:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

sagemaker-bot · 2020-05-22T01:08:19Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

Kim added 2 commits May 20, 2020 23:40

change: blacklist unknown xgboost image versions

16768ff

Merge branch 'master' of https://github.com/aws/sagemaker-python-sdk …

69e6fe0

…into xgboost_get_image_uri

aws-patlin reviewed May 21, 2020

View reviewed changes

Kim added 2 commits May 21, 2020 14:07

Addressed comments

20fc81a

Add test for a warning with '0.90' since there is '0.90-2' available

5502167

aws-patlin previously approved these changes May 21, 2020

View reviewed changes

Ran black

96f9ec3

edwardjkim dismissed aws-patlin’s stale review via 96f9ec3 May 21, 2020 23:05

laurenyu approved these changes May 22, 2020

View reviewed changes

Merge branch 'master' into xgboost_get_image_uri

9629642

laurenyu merged commit 762b509 into aws:master May 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: blacklist unknown xgboost image versions #1519

fix: blacklist unknown xgboost image versions #1519

edwardjkim commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

edwardjkim commented May 21, 2020

aws-patlin May 21, 2020

edwardjkim May 21, 2020

aws-patlin May 21, 2020

aws-patlin May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

aws-patlin commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 21, 2020

sagemaker-bot commented May 22, 2020

edwardjkim commented May 22, 2020

laurenyu commented May 22, 2020

sagemaker-bot commented May 22, 2020

sagemaker-bot commented May 22, 2020

sagemaker-bot commented May 22, 2020

sagemaker-bot commented May 22, 2020

		assert _is_latest_xgboost_version("0.90-1-cpu-py3") is False

		assert _is_latest_xgboost_version("0.90-2-cpu-py3") is False

fix: blacklist unknown xgboost image versions #1519

fix: blacklist unknown xgboost image versions #1519

Conversation

edwardjkim commented May 21, 2020

Merge Checklist

General

Tests

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

edwardjkim commented May 21, 2020

aws-patlin May 21, 2020

Choose a reason for hiding this comment

edwardjkim May 21, 2020

Choose a reason for hiding this comment

aws-patlin May 21, 2020

Choose a reason for hiding this comment

aws-patlin May 21, 2020

Choose a reason for hiding this comment

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

aws-patlin commented May 21, 2020

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 21, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 22, 2020

AWS CodeBuild CI Report

edwardjkim commented May 22, 2020

laurenyu commented May 22, 2020

sagemaker-bot commented May 22, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 22, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 22, 2020

AWS CodeBuild CI Report

sagemaker-bot commented May 22, 2020

AWS CodeBuild CI Report