Skip to content

Commit 23cab69

Browse files
author
Rohan Gujarathi
committed
fix: propagate deploy tags on the model-customization endpoint paths
Tags passed to ModelBuilder.deploy() were accepted but never reached the endpoints created on the model-customization paths, so they could not be used for tag-based resource association. deploy() branches into _deploy_model_customization() for fine-tuned and Nova models and returns before reaching _deploy_core_endpoint(), which is the only place that merges the deploy-time tags kwarg into self._tags. Those paths build their endpoint tags from self._tags, so they saw only the tags ModelBuilder had added itself. Merge the kwarg into the accumulator on entry so all three endpoint-creation paths below pick it up. The restricted-model-package path created its endpoint with no tags at all; it now receives them like the other two. Adds _tags_as_key_value_list() to normalize tags into the lowercase key/value list the sagemaker.core.resources create() calls are typed for. self._tags can hold either casing or a bare {key: value} mapping, and the previous inline reads indexed tag["Key"] directly after format_tags(), which returns a list unchanged whatever its casing -- so a lowercase list raised KeyError. That was already reachable through build(tags=...) and add_tags().
1 parent a7bf7ea commit 23cab69

2 files changed

Lines changed: 166 additions & 11 deletions

File tree

‎sagemaker-serve/src/sagemaker/serve/model_builder.py‎

Lines changed: 53 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@
118118
from sagemaker.core.enums import EndpointType
119119
from sagemaker.core.common_utils import (
120120
Tags,
121+
TagsDict,
121122
ModelApprovalStatusEnum,
122123
_resolve_routing_config,
123124
format_tags,
@@ -180,6 +181,44 @@
180181
SAGEMAKER_OUTPUT_LOCATION = "sagemaker_s3_output"
181182

182183

184+
def _tags_as_key_value_list(tags: Optional[Tags]) -> List[TagsDict]:
185+
"""Normalize any accepted tag form into the ``{"key": ..., "value": ...}`` list form.
186+
187+
``deploy()`` and ``add_tags()`` accept tags either as a list of dicts using
188+
``key``/``value`` or ``Key``/``Value``, or as a single ``{key: value}`` mapping, so
189+
``self._tags`` can hold any of those. The ``sagemaker.core.resources`` ``create()``
190+
calls are typed ``List[Tag]`` whose fields are lowercase and which forbid extra
191+
fields, so they reject the capitalized form and reject a bare mapping outright.
192+
193+
Note this differs from :func:`format_tags`, which emits the capitalized form the
194+
legacy ``sagemaker_session.create_*`` APIs expect and passes a list through
195+
unchanged whatever its casing.
196+
197+
Args:
198+
tags: Tags in any accepted form, or None.
199+
200+
Returns:
201+
The tags as a list of lowercase key/value dicts; empty when none were supplied.
202+
Entries missing a key or value are skipped, since SageMaker rejects them.
203+
"""
204+
if not tags:
205+
return []
206+
if isinstance(tags, dict):
207+
return [{"key": k, "value": v} for k, v in tags.items()]
208+
209+
normalized = []
210+
for tag in tags:
211+
if isinstance(tag, dict):
212+
key = tag.get("key", tag.get("Key"))
213+
value = tag.get("value", tag.get("Value"))
214+
else:
215+
key = getattr(tag, "key", None)
216+
value = getattr(tag, "value", None)
217+
if key is not None and value is not None:
218+
normalized.append({"key": key, "value": value})
219+
return normalized
220+
221+
183222
@dataclass
184223
class ModelBuilder(_InferenceRecommenderMixin, _ModelBuilderServers, _ModelBuilderUtils):
185224
"""Unified interface for building and deploying machine learning models.
@@ -6185,6 +6224,12 @@ def _deploy_model_customization(
61856224
from sagemaker.core.resources import InferenceComponent
61866225
from sagemaker.core.resources import Tag as CoreTag
61876226

6227+
# Tags passed to deploy() arrive here in kwargs. Merge them into the tag
6228+
# accumulator so every endpoint-creation path below picks them up; the
6229+
# standard deploy path does the equivalent in _deploy_core_endpoint().
6230+
if kwargs.get("tags"):
6231+
self.add_tags(_tags_as_key_value_list(kwargs["tags"]))
6232+
61886233
# An inference_config of ResourceRequirements requests an inference
61896234
# component deployment; otherwise the model is placed directly on the
61906235
# production variant.
@@ -6226,7 +6271,9 @@ def _deploy_model_customization(
62266271
],
62276272
)
62286273
endpoint = Endpoint.create(
6229-
endpoint_name=endpoint_name, endpoint_config_name=endpoint_name
6274+
endpoint_name=endpoint_name,
6275+
endpoint_config_name=endpoint_name,
6276+
tags=_tags_as_key_value_list(getattr(self, "_tags", None)) or None,
62306277
)
62316278
if kwargs.get("wait", True):
62326279
endpoint.wait_for_status("InService")
@@ -6266,10 +6313,7 @@ def _deploy_model_customization(
62666313
# tag) to the endpoint so it is discoverable. Stored tags are in
62676314
# {"Key":..,"Value":..} form; normalize to the key/value form the
62686315
# core resource expects.
6269-
endpoint_tags = [
6270-
{"key": tag["Key"], "value": tag["Value"]}
6271-
for tag in format_tags(getattr(self, "_tags", None) or [])
6272-
]
6316+
endpoint_tags = _tags_as_key_value_list(getattr(self, "_tags", None))
62736317
endpoint = Endpoint.create(
62746318
endpoint_name=endpoint_name,
62756319
endpoint_config_name=endpoint_name,
@@ -6541,11 +6585,10 @@ def _deploy_nova_model(
65416585
# Those are stored in {"Key": ..., "Value": ...} form, so normalize to the
65426586
# {"key": ..., "value": ...} form Endpoint.create expects and de-duplicate.
65436587
existing_keys = {tag["key"] for tag in tags}
6544-
for tag in format_tags(getattr(self, "_tags", None) or []):
6545-
key = tag["Key"]
6546-
if key not in existing_keys:
6547-
tags.append({"key": key, "value": tag["Value"]})
6548-
existing_keys.add(key)
6588+
for tag in _tags_as_key_value_list(getattr(self, "_tags", None)):
6589+
if tag["key"] not in existing_keys:
6590+
tags.append(tag)
6591+
existing_keys.add(tag["key"])
65496592

65506593
endpoint = Endpoint.create(
65516594
endpoint_name=endpoint_name,

‎sagemaker-serve/tests/unit/test_model_builder_deploy.py‎

Lines changed: 113 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
from unittest.mock import Mock, patch, MagicMock, call
88
import tempfile
99

10-
from sagemaker.serve.model_builder import ModelBuilder
10+
from sagemaker.serve.model_builder import ModelBuilder, _tags_as_key_value_list
1111
from sagemaker.serve.utils.types import ModelServer
1212
from sagemaker.serve.mode.function_pointers import Mode
1313
from sagemaker.serve.constants import Framework
@@ -602,3 +602,115 @@ def test_reset_build_state_clears_upload_state(self):
602602

603603
if __name__ == "__main__":
604604
unittest.main()
605+
606+
607+
class TestTagsAsKeyValueList(unittest.TestCase):
608+
"""Tests for the tag normalization used by the model-customization deploy paths."""
609+
610+
def test_lowercase_list_passes_through(self):
611+
self.assertEqual(
612+
_tags_as_key_value_list([{"key": "a", "value": "b"}]), [{"key": "a", "value": "b"}]
613+
)
614+
615+
def test_capitalized_list_is_lowercased(self):
616+
self.assertEqual(
617+
_tags_as_key_value_list([{"Key": "a", "Value": "b"}]), [{"key": "a", "value": "b"}]
618+
)
619+
620+
def test_bare_mapping_is_expanded(self):
621+
self.assertEqual(_tags_as_key_value_list({"a": "b"}), [{"key": "a", "value": "b"}])
622+
623+
def test_none_and_empty_yield_empty_list(self):
624+
self.assertEqual(_tags_as_key_value_list(None), [])
625+
self.assertEqual(_tags_as_key_value_list([]), [])
626+
627+
def test_empty_value_is_kept(self):
628+
"""Tag.Value has a minimum length of 0, so an empty value is a legal tag."""
629+
self.assertEqual(
630+
_tags_as_key_value_list([{"key": "a", "value": ""}]), [{"key": "a", "value": ""}]
631+
)
632+
633+
def test_entry_missing_key_or_value_is_skipped(self):
634+
self.assertEqual(
635+
_tags_as_key_value_list([{"key": "a"}, {"value": "b"}, {"key": "c", "value": "d"}]),
636+
[{"key": "c", "value": "d"}],
637+
)
638+
639+
def test_output_validates_as_core_tag(self):
640+
"""The output must satisfy the List[Tag] shape the create() calls are typed with."""
641+
from pydantic import TypeAdapter
642+
from typing import List as TypingList
643+
from sagemaker.core.shapes import Tag
644+
645+
coerced = TypeAdapter(TypingList[Tag]).validate_python(
646+
_tags_as_key_value_list([{"Key": "a", "Value": "b"}])
647+
)
648+
self.assertEqual([(t.key, t.value) for t in coerced], [("a", "b")])
649+
650+
651+
class TestDeployNovaModelTags(unittest.TestCase):
652+
"""Tag propagation through the Nova model-customization deploy path.
653+
654+
``deploy(tags=...)`` previously never reached ``self._tags`` on the
655+
model-customization paths, so user tags were dropped before Endpoint.create.
656+
"""
657+
658+
PROJECT_TAG = {"key": "sagemaker:project-id", "value": "p-12345"}
659+
660+
def _make_builder(self, tags=None):
661+
builder = ModelBuilder.__new__(ModelBuilder)
662+
builder.instance_type = "ml.g5.12xlarge"
663+
builder.built_model = MagicMock(model_name="model-1")
664+
builder._tags = None
665+
base_model = MagicMock(hub_content_name="amazon-nova-lite-v1", recipe_name="nova-sft")
666+
package = MagicMock()
667+
package.inference_specification.containers = [MagicMock(base_model=base_model)]
668+
builder._fetch_model_package = MagicMock(return_value=package)
669+
builder._base_model_name = MagicMock(return_value="amazon-nova-lite-v1")
670+
builder._is_raw_s3_model = MagicMock(return_value=False)
671+
if tags:
672+
builder.add_tags(_tags_as_key_value_list(tags))
673+
return builder
674+
675+
def _created_tags(self, builder):
676+
with patch("sagemaker.serve.model_builder.EndpointConfig"), patch(
677+
"sagemaker.serve.model_builder.Endpoint"
678+
) as mock_endpoint:
679+
mock_endpoint.create.return_value = MagicMock()
680+
builder._deploy_nova_model(endpoint_name="endpoint-1", wait=False)
681+
return mock_endpoint.create.call_args.kwargs["tags"]
682+
683+
def test_user_tags_merged_with_jumpstart_tags(self):
684+
tags = self._created_tags(self._make_builder(tags=[self.PROJECT_TAG]))
685+
self.assertIn(self.PROJECT_TAG, tags)
686+
self.assertTrue(any("jumpstart-model-id" in tag["key"] for tag in tags))
687+
688+
def test_capitalized_user_tags_accepted(self):
689+
builder = self._make_builder(tags=[{"Key": "sagemaker:project-id", "Value": "p-12345"}])
690+
self.assertIn(self.PROJECT_TAG, self._created_tags(builder))
691+
692+
def test_no_user_tags_leaves_jumpstart_tags_only(self):
693+
tags = self._created_tags(self._make_builder())
694+
self.assertEqual(len(tags), 2)
695+
self.assertTrue(all(tag["key"].startswith("sagemaker-sdk:") for tag in tags))
696+
697+
def test_deploy_tags_reach_the_endpoint_through_model_customization(self):
698+
"""Covers the full chain: deploy(tags=...) -> kwargs -> self._tags -> Endpoint.create.
699+
700+
_deploy_model_customization() is the entry point the customization deploy path
701+
takes, and it is where the deploy-time tags are merged into the accumulator.
702+
"""
703+
builder = self._make_builder()
704+
builder._is_nova_model = MagicMock(return_value=True)
705+
706+
with patch("sagemaker.serve.model_builder.EndpointConfig"), patch(
707+
"sagemaker.serve.model_builder.Endpoint"
708+
) as mock_endpoint:
709+
mock_endpoint.create.return_value = MagicMock()
710+
builder._deploy_model_customization(
711+
endpoint_name="endpoint-1", wait=False, tags=[self.PROJECT_TAG]
712+
)
713+
tags = mock_endpoint.create.call_args.kwargs["tags"]
714+
715+
self.assertIn(self.PROJECT_TAG, tags)
716+
self.assertTrue(any("jumpstart-model-id" in tag["key"] for tag in tags))

0 commit comments

Comments
 (0)