Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 #9310

clarkh-ncino · 2023-08-14T17:23:50Z

Related Issues/PRs

Resolve #9159

What changes are proposed in this pull request?

Fixing a bug where tags were not being added to sagemaker endpoints during deployment. Also added logic to add custom tags to sagemaker config resources as well.

How is this patch tested?

Existing unit/integration tests
New unit/integration tests
Manual tests (describe details, including test results, below)

Does this PR change the documentation?

No. You can skip the rest of this section.
Yes. Make sure the changed pages / sections render correctly in the documentation preview.

Release Notes

Is this a user-facing change?

No. You can skip the rest of this section.
Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)

Users of the SageMaker deployment functionality can now configure custom tags for both endpoints and endpoint configuration resources from both the CLI and Python SDK.

What component(s), interfaces, languages, and integrations does this PR affect?

Components

Interface

area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
area/windows: Windows support

Language

language/r: R APIs and clients
language/java: Java APIs and clients
language/new: Proposals for new client languages

Integrations

integrations/azure: Azure and Azure ML integrations
integrations/sagemaker: SageMaker integrations
integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
rn/feature - A new user-facing feature worth mentioning in the release notes
rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
rn/documentation - A user-facing documentation change worth mentioning in the release notes

mlflow-automation · 2023-08-14T19:46:03Z

Documentation preview for 7acc172 will be available here when this CircleCI job completes successfully.

More info

Ignore this comment if this PR does not change the documentation.
It takes a few minutes for the preview to be available.
The preview is updated when a new commit is pushed to this PR.
This comment was created by https://github.com/mlflow/mlflow/actions/runs/7132140037.

chenmoneygithub

Thanks for the PR!

Overall looks good to me! only dropped some comments on the style.

Btw, could you help edit the title to include the context of this PR? It would be easier to track in the history (after squash and merge).

chenmoneygithub · 2023-08-15T18:38:34Z

mlflow/sagemaker/__init__.py

+def _get_sagemaker_config_tags(endpoint_name):
+    return [{"Key": "app_name", "Value": endpoint_name}]
+
+def _add_sagemaker_tags(tags, config_tags):


The args are little bit confusing, it's hard to tell the diff between tags and config_tags. We can either add a docstring to explain or use a more explicit name, e.g., tags and app_name_tag.

chenmoneygithub · 2023-08-15T18:42:49Z

mlflow/sagemaker/__init__.py

+
+def _add_sagemaker_tags(tags, config_tags):
+    """
+    Convert dict of tags to list for SageMaker and adds to config tags list


nit: period at the end

chenmoneygithub · 2023-08-15T18:46:59Z

tests/sagemaker/mock/__init__.py

-        model = next(model for model in self.models.values() if model.arn == resource_arn)
-        return model.resource.tags
+        resource_values = getattr(self, resource_type).values()
+        sagemaker_resource = next(


Usually if we need to break inline code to multiple lines, it indicates we should replace the inline style with the normal for loop for readability. could you help refactor? thanks!

BenWilson2 · 2023-08-16T16:55:10Z

mlflow/sagemaker/__init__.py

+    """
+    if sagemaker_tags:
+        sagemaker_tags = [{"Key": key, "Value": str(value)} for key, value in sagemaker_tags.items()]
+        config_tags.extend(sagemaker_tags)


What does this line do?
The return fo the private function is sagemaker_tags, which is handled entirely by lines 1349 and 1350.
Was the intention to return config_tags ?

This adds any custom tags that are passed in the initial create_deployment function (example here ) to also be added to the SageMaker configuration tags. With the current functionality, the only tags being added to the SageMaker config are manually set here

_prepare_sagemaker_tags({"a_config_tag": "a_value", "my_custom_tag": "my_custom_value"}, [{"Key": "app_name", "Value": "my_app"}])

[{'Key': 'a_config_tag', 'Value': 'a_value'}, {'Key': 'my_custom_tag', 'Value': 'my_custom_value'}]

If this is the intended output, then there is no need to have config_tags (which is the structure [{"Key": "app_name", "Value": <endpoint_name>}] defined and returned within _get_sagemaker_config_tags)

BenWilson2 · 2023-08-16T17:05:40Z

mlflow/sagemaker/__init__.py

+    return [{"Key": "app_name", "Value": endpoint_name}]
+
+def _prepare_sagemaker_tags(
+  sagemaker_tags: Dict[str, str],


What happens if the dict sagemaker_tags contains the key "app_name"? What does AWS do with multiple conflicting tags?

Good point, looks like the request would be rejected according to this documentation. I'll add a check for that.

BenWilson2 · 2023-08-16T17:26:59Z

mlflow/sagemaker/__init__.py

+        sagemaker_tags = [{"Key": key, "Value": str(value)} for key, value in sagemaker_tags.items()]
+        config_tags.extend(sagemaker_tags)
+
+    return sagemaker_tags


Would something like this perhaps be a little bit more robust?

from mlflow import MlflowException from typing import List, Dict, Optional SAGEMAKER_APP_NAME_TAG_KEY = "app_name" def _get_sagemaker_config_tags(endpoint_name): return [{"Key": SAGEMAKER_APP_NAME_TAG_KEY, "Value": endpoint_name}] def _prepare_sagemaker_tags( config_tags: List[Dict[str, str]], sagemaker_tags: Optional[Dict[str, str]]=None, ): if not sagemaker_tags: return config_tags if SAGEMAKER_APP_NAME_TAG_KEY in sagemaker_tags: raise MlflowException.invalid_parameter_value(f"Duplicate tag provided for '{SAGEMAKER_APP_NAME_TAG_KEY}'") parsed = [{"Key": key, "Value": str(value)} for key, value in sagemaker_tags.items()] return config_tags + parsed

Validating the behavior:

_prepare_sagemaker_tags([{"Key": "app_name", "Value": "my_app"}], {"a": "1", "b": "2", "c": "3"})

[{'Key': 'app_name', 'Value': 'my_app'}, {'Key': 'a', 'Value': '1'}, {'Key': 'b', 'Value': '2'}, {'Key': 'c', 'Value': '3'}]

_prepare_sagemaker_tags([{"Key": "app_name", "Value": "my_app"}], {"app_name": "my_better_app", "b": "2", "c": "3"})

--------------------------------------------------------------------------- MlflowException Traceback (most recent call last) /var/folders/cd/n8n0rm2x53l_s0xv_j_xklb00000gp/T/ipykernel_12446/3588563336.py in <cell line: 1>() ----> 1 _prepare_sagemaker_tags([{"Key": "app_name", "Value": "my_app"}], {"app_name": "my_better_app", "b": "2", "c": "3"}) /var/folders/cd/n8n0rm2x53l_s0xv_j_xklb00000gp/T/ipykernel_12446/3590447905.py in _prepare_sagemaker_tags(config_tags, sagemaker_tags) 14 15 if SAGEMAKER_APP_NAME_TAG_KEY in sagemaker_tags: ---> 16 raise MlflowException.invalid_parameter_value(f"Duplicate tag provided for '{SAGEMAKER_APP_NAME_TAG_KEY}'") 17 parsed = [{"Key": key, "Value": str(value)} for key, value in sagemaker_tags.items()] 18 MlflowException: Duplicate tag provided for 'app_name'

_prepare_sagemaker_tags([{"Key": "app_name", "Value": "my_app"}], None) _prepare_sagemaker_tags([{"Key": "app_name", "Value": "my_app"}], {})

[{'Key': 'app_name', 'Value': 'my_app'}]

Does this reflect the intended functionality for the _prepare_sagemaker_tags() function?

Yep, this looks great.

BenWilson2 · 2023-08-16T17:33:12Z

tests/sagemaker/mock/__init__.py

+        if "model" in arn:
+            sagemaker_resource = "models"
+        elif "endpoint" in arn:
+            sagemaker_resource = "endpoints"


This can be modified to a ternary operator.

BenWilson2 · 2023-08-16T17:38:16Z

tests/sagemaker/mock/__init__.py

-        model = next(model for model in self.models.values() if model.arn == resource_arn)
-        return model.resource.tags
+        resource_values = getattr(self, resource_type).values()
+        for sagemaker_resource in resource_values:


why is this being modified to a for loop? is self.resource_type.values() not accessible?

It is not accessible because resource_type is not a valid attribute name. It's a variable containing a string value which is why line 526 was added. The original inline code was broken out to a for loop based on a previous review to improve readability.

BenWilson2

Please add tests that validate the correctness of supplied tags and that parsing is performed correctly for the conversion from dict -> AWS-specified List[Dict[str, str]] format. The current implementation seems like it is dropping data.

BenWilson2 · 2023-08-23T15:03:05Z

Hi @clarkh-ncino could you rebase to master to address the merge conflicts (and then I can initiate the CI suite for you)

Signed-off-by: Clark Hollar <[email protected]>

This reverts commit 3225a11. Signed-off-by: Clark Hollar <[email protected]>

…or loop for readability Signed-off-by: Clark Hollar <[email protected]>

Signed-off-by: Clark Hollar <[email protected]>

clarkh-ncino · 2023-08-24T17:46:37Z

Hi @clarkh-ncino could you rebase to master to address the merge conflicts (and then I can initiate the CI suite for you)

Should be good to go whenever you have a moment. 👍 @BenWilson2

clarkh-ncino · 2023-11-06T19:13:16Z

@BenWilson2 @chenmoneygithub anything I can do here to help move this forward? Think we just need to kick of the CI build.

BenWilson2 · 2023-11-09T00:55:00Z

@clarkh-ncino could you rebase to master again? The issues that you're encountering should be resolved (you'll also need to fix those lint failures, though with import ordering and the black formatting issues). If you run the pre-commit hook (pre-commit run --all-files, as mentioned in https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.md#python )

Signed-off-by: Clark Hollar <[email protected]>

fixing lint issues and removing unused variable

clarkh-ncino · 2023-11-29T19:01:08Z

@BenWilson2 Should be good to go now 🤞

BenWilson2

LGTM!

OOO

…ow#9159 (mlflow#9310) Signed-off-by: Clark Hollar <[email protected]>

clarkh-ncino marked this pull request as ready for review August 14, 2023 18:10

clarkh-ncino mentioned this pull request Aug 14, 2023

[BUG] Tags are not added to sagemaker endpoints during deployment. #9159

Closed

23 tasks

clarkh-ncino force-pushed the master branch from 0196c0f to 0d20315 Compare August 14, 2023 20:02

github-actions bot added area/scoring MLflow Model server, model deployment tools, Spark UDFs integrations/sagemaker Sagemaker integrations rn/bug-fix Mention under Bug Fixes in Changelogs. labels Aug 15, 2023

chenmoneygithub previously requested changes Aug 15, 2023

View reviewed changes

clarkh-ncino changed the title ~~Issue-9159~~ Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 Aug 16, 2023

clarkh-ncino mentioned this pull request Aug 16, 2023

issue-9159 updates ncino/mlflow#2

Merged

BenWilson2 reviewed Aug 16, 2023

View reviewed changes

BenWilson2 requested changes Aug 16, 2023

View reviewed changes

clarkh-ncino mentioned this pull request Aug 18, 2023

adding exception for duplicate tags and tests ncino/mlflow#3

Merged

clarkh-ncino force-pushed the master branch from 623e80d to 9f36f95 Compare August 23, 2023 17:29

clarkh-ncino and others added 9 commits August 23, 2023 13:57

adding ability to include tagging for sagemaker endpoints

3b075c9

Signed-off-by: Clark Hollar <[email protected]>

updating test

61c3252

Signed-off-by: Clark Hollar <[email protected]>

adding helper functions

ce8a35f

Signed-off-by: Clark Hollar <[email protected]>

adding codeowners file

7d336b7

Signed-off-by: Clark Hollar <[email protected]>

Revert "adding codeowners file"

6b8858f

This reverts commit 3225a11. Signed-off-by: Clark Hollar <[email protected]>

updating helper function name and args for clarity and implementing f…

c778dc1

…or loop for readability Signed-off-by: Clark Hollar <[email protected]>

adding exception for duplicate tags and tests

ef272eb

Signed-off-by: Clark Hollar <[email protected]>

removing white space and adding tern on one line

99d4d35

Signed-off-by: Clark Hollar <[email protected]>

removing comment

88a4ff0

Signed-off-by: Clark Hollar <[email protected]>

clarkh-ncino force-pushed the master branch from 9f36f95 to 88a4ff0 Compare August 23, 2023 17:59

clarkh-ncino added 2 commits September 22, 2023 11:11

Merge branch 'mlflow:master' into master

d9845ac

Merge branch 'mlflow:master' into master

3a7502c

clarkh-ncino and others added 3 commits November 28, 2023 13:33

Merge branch 'mlflow:master' into master

5762080

fixing lint issues and removing unused variable

aaa721c

Signed-off-by: Clark Hollar <[email protected]>

Merge pull request #6 from ncino/lint_fix

f5accd2

fixing lint issues and removing unused variable

Merge branch 'mlflow:master' into master

7acc172

BenWilson2 approved these changes Dec 7, 2023

View reviewed changes

BenWilson2 requested a review from chenmoneygithub December 7, 2023 18:20

BenWilson2 merged commit e4e23c3 into mlflow:master Jan 2, 2024

annzhang-db pushed a commit to annzhang-db/mlflow that referenced this pull request Jan 3, 2024

Adds tagging for sagemaker endpoints and sagemaker config. Issue mlfl…

0d1b929

…ow#9159 (mlflow#9310) Signed-off-by: Clark Hollar <[email protected]>

B-Step62 pushed a commit to B-Step62/mlflow that referenced this pull request Jan 9, 2024

Adds tagging for sagemaker endpoints and sagemaker config. Issue mlfl…

d3f6faa

…ow#9159 (mlflow#9310) Signed-off-by: Clark Hollar <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 #9310

Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 #9310

clarkh-ncino commented Aug 14, 2023 •

edited by harupy

Loading

mlflow-automation commented Aug 14, 2023 •

edited by github-actions bot

Loading

chenmoneygithub left a comment •

edited

Loading

chenmoneygithub Aug 15, 2023

chenmoneygithub Aug 15, 2023

chenmoneygithub Aug 15, 2023

BenWilson2 Aug 16, 2023

clarkh-ncino Aug 16, 2023

BenWilson2 Aug 16, 2023

BenWilson2 Aug 16, 2023

clarkh-ncino Aug 16, 2023

BenWilson2 Aug 16, 2023

clarkh-ncino Aug 17, 2023

BenWilson2 Aug 16, 2023

BenWilson2 Aug 16, 2023

clarkh-ncino Aug 17, 2023

BenWilson2 left a comment

BenWilson2 commented Aug 23, 2023

clarkh-ncino commented Aug 24, 2023 •

edited

Loading

clarkh-ncino commented Nov 6, 2023

BenWilson2 commented Nov 9, 2023

clarkh-ncino commented Nov 29, 2023

BenWilson2 left a comment

Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 #9310

Adds tagging for sagemaker endpoints and sagemaker config. Issue #9159 #9310

Conversation

clarkh-ncino commented Aug 14, 2023 • edited by harupy Loading

Related Issues/PRs

What changes are proposed in this pull request?

How is this patch tested?

Does this PR change the documentation?

Release Notes

Is this a user-facing change?

What component(s), interfaces, languages, and integrations does this PR affect?

How should the PR be classified in the release notes? Choose one:

mlflow-automation commented Aug 14, 2023 • edited by github-actions bot Loading

chenmoneygithub left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenWilson2 left a comment

Choose a reason for hiding this comment

BenWilson2 commented Aug 23, 2023

clarkh-ncino commented Aug 24, 2023 • edited Loading

clarkh-ncino commented Nov 6, 2023

BenWilson2 commented Nov 9, 2023

clarkh-ncino commented Nov 29, 2023

BenWilson2 left a comment

Choose a reason for hiding this comment

clarkh-ncino commented Aug 14, 2023 •

edited by harupy

Loading

mlflow-automation commented Aug 14, 2023 •

edited by github-actions bot

Loading

chenmoneygithub left a comment •

edited

Loading

clarkh-ncino commented Aug 24, 2023 •

edited

Loading