feature: adding resourcekey and tags for api in config for SDK defaults #3915

akrishna1995 · 2023-06-07T20:31:35Z

Issue #, if available:

Description of changes:

adding resourcekey and tags for api in config for intelligent defaults

Testing done:

unit tests

tested in greygoose

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

[x ] I have read the CONTRIBUTING doc
[ x] I certify that the changes I am introducing will be backward compatible, and I have discussed concerns about this, if any, with the Python SDK team
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

[X ] I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have added unit and/or integration tests as appropriate to ensure backward compatibility of the changes
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.

sagemaker-bot · 2023-06-07T20:36:25Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-07T21:13:06Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-07T21:14:22Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-07T21:20:42Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-07T21:23:00Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-08T21:39:29Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-08T21:46:38Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-08T22:02:38Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: bb683d0
Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-06-08T22:20:48Z

AWS CodeBuild CI Report

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

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

codecov-commenter · 2023-06-08T22:23:57Z

Codecov Report

Merging #3915 (31dd5da) into master (12b19ee) will decrease coverage by 0.70%.
The diff coverage is 100.00%.

❗ Current head 31dd5da differs from pull request most recent head dacac74. Consider uploading reports for the commit dacac74 to get more accurate results

@@            Coverage Diff             @@
##           master    #3915      +/-   ##
==========================================
- Coverage   90.07%   89.38%   -0.70%     
==========================================
  Files        1156      271     -885     
  Lines      106844    26420   -80424     
==========================================
- Hits        96245    23616   -72629     
+ Misses      10599     2804    -7795

Impacted Files	Coverage Δ
src/sagemaker/config/config_schema.py	`100.00% <100.00%> (ø)`
src/sagemaker/model.py	`94.13% <100.00%> (ø)`
src/sagemaker/session.py	`78.67% <100.00%> (ø)`

... and 1424 files with indirect coverage changes

sagemaker-bot · 2023-06-09T00:22:41Z

AWS CodeBuild CI Report

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

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

knikure · 2023-06-09T18:41:43Z

tests/data/config/config.yaml

+    RoleArn: 'arn:aws:iam::555555555555:role/IMRole'
+    ResourceKey: 'kmskeyid1'
+  PythonSDK:


Why this is needed here?
This is already in the file from line 3:28.

Thanks - Yes. it is not needed, got incorrectly added as part of a git merge conflict resolve. Will remove it in the next commit

knikure · 2023-06-09T18:49:34Z

tests/unit/test_session.py

@@ -3102,7 +3109,7 @@ def test_endpoint_from_production_variants_with_sagemaker_config_injection(
    sagemaker_session.sagemaker_client.create_endpoint.assert_called_with(
        EndpointConfigName="some-endpoint",
        EndpointName="some-endpoint",
-        Tags=expected_tags,  # from config
+        Tags=[],


Can you write a unit-test when both CreateEndpoint and CreateEndpointConfig is passed with some Tags Config and see respective configs are applied?

Added a new UT with the combination in the next commit :)

src/sagemaker/model.py

knikure · 2023-06-09T19:03:48Z

src/sagemaker/session.py

+            log_group,
+            dot,
+            color_wrap,
+        ) = _logs_init(self.boto_session, description, job="AutoML")


Is there a reason we are moving every single param to next line? Is it a best practice for Python code development?
If not, let's remove this formatting since it will just increase the number of lines in this huge file(~7K lines). AFAIK, we have a line length limit of 100, if it satisfies that, lets keep it as original.
If there are any black-check or pylint errors we need to resolve, we can move them to next line.

Thanks I believe this was the result of the black-format command to auto fix the file, I can double check and revert this

knikure · 2023-06-09T19:06:58Z

Kindly follow this and write a meaningful header for this PR?

sagemaker-bot · 2023-06-09T19:25:12Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-09T20:59:10Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-09T21:09:19Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-09T21:10:44Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-09T21:22:07Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-09T21:44:45Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T20:14:35Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T20:19:48Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T20:56:54Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T21:05:47Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: 31dd5da
Result: FAILED
Build Logs (available for 30 days)

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

…t defaults Adding ResourceKey to the Config schema for the createEdgePackaging Job API Adding a new entry into the Config Schema for CreateEndpoint API with the addition of tags attribute

… intelligent defaults Added Unit tests to test the config injection for SAGEMAKER_CONFIG_ENDPOINT Fixed a couple of unit tests

Making changes wrt formatting to fix tox issues

fixed comments on PR, added UTs, style changes

sagemaker-bot · 2023-06-12T21:54:52Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T22:04:15Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: dacac74
Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-06-12T22:10:19Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T22:26:08Z

AWS CodeBuild CI Report

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

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

akrishna1995

/bot run pr

akrishna1995 · 2023-06-12T22:38:53Z

src/sagemaker/config/__init__.py

@@ -48,6 +48,7 @@
    COMPILATION_JOB_VPC_CONFIG_PATH,
    COMPILATION_JOB,
    EDGE_PACKAGING_ROLE_ARN_PATH,
+    EDGE_PACKAGING_RESOURCE_KEY_PATH,


/bot run pr

sagemaker-bot · 2023-06-12T22:42:29Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T23:32:04Z

AWS CodeBuild CI Report

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

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

sagemaker-bot · 2023-06-12T23:34:40Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: dacac74
Result: FAILED
Build Logs (available for 30 days)

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

sagemaker-bot · 2023-06-12T23:34:42Z

AWS CodeBuild CI Report

CodeBuild project: sagemaker-python-sdk-pr
Commit ID: dacac74
Result: FAILED
Build Logs (available for 30 days)

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

rubanh

Looks good! Just a couple minor comments

rubanh · 2023-06-13T19:30:16Z

src/sagemaker/model.py

@@ -300,7 +314,9 @@ def __init__(
        self._base_name = None
        self.sagemaker_session = sagemaker_session
        self.role = resolve_value_from_config(
-            role, MODEL_EXECUTION_ROLE_ARN_PATH, sagemaker_session=self.sagemaker_session
+            role,


qq: did some configuration change recently for how we do spacing? Odd that non-touched things are changing

True - I think this is a result of tox -e black-format

rubanh · 2023-06-13T19:33:37Z

src/sagemaker/session.py

@@ -4026,6 +4033,9 @@ def create_endpoint(self, endpoint_name, config_name, tags=None, wait=True):

        tags = tags or []
        tags = _append_project_tags(tags)
+        tags = self._append_sagemaker_config_tags(
+            tags, "{}.{}.{}".format(SAGEMAKER, ENDPOINT, TAGS)


nit: Can use ENDPOINT_TAGS_PATH right?

Yeah, will make the change in a quick follow up PR

rubanh · 2023-06-13T19:36:00Z

src/sagemaker/session.py

+        endpoint_config_tags = _append_project_tags(tags)
+        endpoint_tags = _append_project_tags(tags)
+        endpoint_config_tags = self._append_sagemaker_config_tags(
+            endpoint_config_tags, "{}.{}.{}".format(SAGEMAKER, ENDPOINT_CONFIG, TAGS)


nit: can use the path directly here also

rubanh · 2023-06-13T19:37:11Z

src/sagemaker/session.py

+        endpoint_tags = _append_project_tags(tags)
+        endpoint_config_tags = self._append_sagemaker_config_tags(
+            endpoint_config_tags, "{}.{}.{}".format(SAGEMAKER, ENDPOINT_CONFIG, TAGS)
+        )


not injecting endpoint tags from config here because that is in create_endpoint right? That makes sense. (optional) Could have a small comment here mentioning that

Makes sense, will make the change in a quick follow up PR

rubanh · 2023-06-13T19:39:35Z

src/sagemaker/session.py


        Returns:
            str: Name of the ``Endpoint`` that is created.
        """
        model_environment_vars = model_environment_vars or {}
        name = name or name_from_image(image_uri)
        model_vpc_config = vpc_utils.sanitize(model_vpc_config)
-
+        endpoint_config_tags = _append_project_tags(tags)
+        endpoint_tags = _append_project_tags(tags)


Should this line be skipped if its also in create_endpoint? (also, does calling this function twice cause issues, or is it smart enough to handle that scenario?)

Thanks for this comment :) - You're right, we can omit it - the _append_project_tags will boot out similar values so it wouldn't be an issue. Although it is not as smart as _append_sagemaker_config_tags which explicitly checks keys. Will update to skip it as a follow up along with the integ test change

rubanh · 2023-06-13T19:42:21Z

src/sagemaker/session.py

+        endpoint_config_tags = _append_project_tags(tags)
+        endpoint_tags = _append_project_tags(tags)


There should be an integ test for sagemaker_config where EndpointConfig tags are passed in thru the config (dont remember which Session method they end up calling). Can we add Endpoint tags to that test as well and verify that Endpoint and EndpointConfig end up with different tag values?

Will add this as part of a new PR 👍

akrishna1995 added the do-not-merge Use when PR needs to be marked as do not merge label Jun 7, 2023

akrishna1995 force-pushed the intelligent-defaults-main branch from 4bb64d7 to bb683d0 Compare June 8, 2023 20:53

akrishna1995 marked this pull request as ready for review June 8, 2023 21:13

akrishna1995 requested a review from a team as a code owner June 8, 2023 21:13

akrishna1995 requested review from trajanikant, rubanh and knikure and removed request for a team and trajanikant June 8, 2023 21:13

knikure reviewed Jun 9, 2023

View reviewed changes

src/sagemaker/model.py Show resolved Hide resolved

knikure reviewed Jun 9, 2023

View reviewed changes

knikure self-assigned this Jun 9, 2023

akrishna1995 changed the title ~~Intelligent defaults main~~ feature: adding resourcekey and tags for api in config for SDK defaults Jun 9, 2023

akrishna1995 force-pushed the intelligent-defaults-main branch from bb683d0 to 31dd5da Compare June 12, 2023 19:31

akrishna1995 and others added 4 commits June 12, 2023 21:16

feature: adding resourcekey and tags for api in config for intelligen…

c9d25f6

…t defaults Adding ResourceKey to the Config schema for the createEdgePackaging Job API Adding a new entry into the Config Schema for CreateEndpoint API with the addition of tags attribute

fix: adding Unit tests for resourcekey and tags for api in config for…

ebd7872

… intelligent defaults Added Unit tests to test the config injection for SAGEMAKER_CONFIG_ENDPOINT Fixed a couple of unit tests

fix: SDK defaults formatting changes

72b00b6

Making changes wrt formatting to fix tox issues

fix: comments from PR

dacac74

fixed comments on PR, added UTs, style changes

akrishna1995 force-pushed the intelligent-defaults-main branch from 31dd5da to dacac74 Compare June 12, 2023 21:16

akrishna1995 commented Jun 12, 2023

View reviewed changes

akrishna1995 requested a review from knikure June 13, 2023 17:43

knikure approved these changes Jun 13, 2023

View reviewed changes

rubanh approved these changes Jun 13, 2023

View reviewed changes

akrishna1995 added status: ready for merge and removed do-not-merge Use when PR needs to be marked as do not merge labels Jun 13, 2023

akrishna1995 merged commit 1f38b07 into aws:master Jun 13, 2023

akrishna1995 deleted the intelligent-defaults-main branch June 28, 2023 22:58

		endpoint_config_tags = _append_project_tags(tags)
		endpoint_tags = _append_project_tags(tags)

feature: adding resourcekey and tags for api in config for SDK defaults #3915

feature: adding resourcekey and tags for api in config for SDK defaults #3915

Uh oh!

Conversation

akrishna1995 commented Jun 7, 2023

Merge Checklist

General

Tests

Uh oh!

sagemaker-bot commented Jun 7, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 7, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 7, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 7, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 7, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 8, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 8, 2023

AWS CodeBuild CI Report

Uh oh!

codecov-commenter commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knikure Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knikure commented Jun 9, 2023

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 9, 2023

AWS CodeBuild CI Report

Uh oh!

sagemaker-bot commented Jun 12, 2023

AWS CodeBuild CI Report

codecov-commenter commented Jun 8, 2023 •

edited

Loading

knikure Jun 9, 2023 •

edited

Loading