Skip to content

Commit b56e329

Browse files
mohamedzeidan2021Mohamed Zeidan
andauthored
fix: accept core shapes and omitted content types in ModelBuilder.register (#6489)
ModelBuilder.register() had two independent AttributeError defects on the same registry/deploy-from-package workflow. 1. register() rejected the ModelMetrics type its own signature declares. The signature types model_metrics (and metadata_properties, drift_check_baselines, model_life_cycle) as sagemaker.core.shapes classes, but get_model_package_args() converted them by calling value._to_request_dict(). The core shapes are pydantic models that do not implement that method, so the documented type raised "AttributeError: 'ModelMetrics' object has no attribute '_to_request_dict'". Only the older sagemaker.core.model_metrics compatibility class, which is not the declared type, has the method. get_model_package_args() now routes these four arguments through a small _to_request_dict() helper that keeps using _to_request_dict() when the value implements it and otherwise falls back to the core serializer, sagemaker.core.utils.utils.serialize(). The compatibility classes are unaffected, so they stay supported for the rest of their deprecation period, and dicts already in request form pass through. Replacing the call with serialize() unconditionally, as suggested on the issue, was rejected: the compatibility classes carry flat fields (model_statistics, model_constraints, ...) that serialize() renders as ModelStatistics/ModelConstraints instead of the nested ModelQuality.Statistics block the API expects, which would have broken the path that works today. 2. register() failed when content_types and response_types were omitted. Both are declared optional with None defaults, but the method only assigned self.content_types / self.response_types when the arguments were not None and then read those attributes unconditionally, raising "AttributeError: 'ModelBuilder' object has no attribute 'content_types'" on a first registration that omits them. The read now uses getattr(self, ..., None). This keeps the existing behaviour of reusing values supplied to an earlier register() call, and avoids adding new ModelBuilder dataclass fields next to the unrelated, already-public content_type field. Fixes #6479 Fixes #6480 Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
1 parent ec68770 commit b56e329

4 files changed

Lines changed: 353 additions & 8 deletions

File tree

‎sagemaker-core/src/sagemaker/core/model_registry.py‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,33 @@
1515
MODEL_PACKAGE_VALIDATION_PROFILES_PATH,
1616
)
1717
from sagemaker.core.resources import ModelPackageModelCard
18+
from sagemaker.core.utils.utils import serialize
1819
from botocore.exceptions import ClientError
1920
import logging
2021

2122
logger = LOGGER = logging.getLogger("sagemaker")
2223

2324

25+
def _to_request_dict(value):
26+
"""Convert a model registry argument into its CreateModelPackage request shape.
27+
28+
Supports both the modern ``sagemaker.core.shapes`` pydantic shapes, which are
29+
converted with the core serializer, and the older compatibility classes in
30+
``sagemaker.core.model_metrics`` and friends, which implement their own
31+
``_to_request_dict()``. Plain dicts are assumed to already be in request form
32+
and are passed through the serializer unchanged.
33+
34+
Args:
35+
value: A core shape, a compatibility class instance, or a request dict.
36+
37+
Returns:
38+
dict: The serialized request shape.
39+
"""
40+
if hasattr(value, "_to_request_dict"):
41+
return value._to_request_dict()
42+
return serialize(value)
43+
44+
2445
def get_model_package_args(
2546
content_types=None,
2647
response_types=None,
@@ -76,11 +97,11 @@ def get_model_package_args(
7697
if model_package_group_name is not None:
7798
model_package_args["model_package_group_name"] = model_package_group_name
7899
if model_metrics is not None:
79-
model_package_args["model_metrics"] = model_metrics._to_request_dict()
100+
model_package_args["model_metrics"] = _to_request_dict(model_metrics)
80101
if drift_check_baselines is not None:
81-
model_package_args["drift_check_baselines"] = drift_check_baselines._to_request_dict()
102+
model_package_args["drift_check_baselines"] = _to_request_dict(drift_check_baselines)
82103
if metadata_properties is not None:
83-
model_package_args["metadata_properties"] = metadata_properties._to_request_dict()
104+
model_package_args["metadata_properties"] = _to_request_dict(metadata_properties)
84105
if approval_status is not None:
85106
model_package_args["approval_status"] = approval_status
86107
if description is not None:
@@ -102,7 +123,7 @@ def get_model_package_args(
102123
if source_uri is not None:
103124
model_package_args["source_uri"] = source_uri
104125
if model_life_cycle is not None:
105-
model_package_args["model_life_cycle"] = model_life_cycle._to_request_dict()
126+
model_package_args["model_life_cycle"] = _to_request_dict(model_life_cycle)
106127
if model_card is not None:
107128
original_req = {}
108129
if isinstance(model_card, ModelPackageModelCard):

‎sagemaker-core/tests/unit/test_model_registry.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,109 @@ def test_get_model_package_args_model_package_model_card(self):
158158
"ModelCardStatus": "Approved",
159159
}
160160

161+
def test_get_model_package_args_with_core_shape_model_metrics(self):
162+
"""Core shapes.ModelMetrics is serialized with the core serializer.
163+
164+
The public ModelBuilder.register() signature declares model_metrics as
165+
sagemaker.core.shapes.ModelMetrics, which has no _to_request_dict().
166+
"""
167+
from sagemaker.core.shapes import MetricsSource, ModelMetrics, ModelQuality
168+
169+
model_metrics = ModelMetrics(
170+
model_quality=ModelQuality(
171+
statistics=MetricsSource(
172+
content_type="application/json",
173+
s3_uri="s3://bucket/evaluation.json",
174+
)
175+
)
176+
)
177+
178+
args = get_model_package_args(
179+
image_uri="test-image:latest",
180+
model_metrics=model_metrics,
181+
)
182+
183+
assert args["model_metrics"] == {
184+
"ModelQuality": {
185+
"Statistics": {
186+
"ContentType": "application/json",
187+
"S3Uri": "s3://bucket/evaluation.json",
188+
}
189+
}
190+
}
191+
192+
def test_get_model_package_args_with_compatibility_model_metrics(self):
193+
"""The older compatibility class keeps using its own _to_request_dict()."""
194+
from sagemaker.core.model_metrics import MetricsSource, ModelMetrics
195+
196+
model_metrics = ModelMetrics(
197+
model_statistics=MetricsSource(
198+
content_type="application/json",
199+
s3_uri="s3://bucket/evaluation.json",
200+
)
201+
)
202+
203+
args = get_model_package_args(
204+
image_uri="test-image:latest",
205+
model_metrics=model_metrics,
206+
)
207+
208+
assert args["model_metrics"]["ModelQuality"] == {
209+
"Statistics": {
210+
"ContentType": "application/json",
211+
"S3Uri": "s3://bucket/evaluation.json",
212+
}
213+
}
214+
215+
def test_get_model_package_args_with_core_shape_drift_check_baselines(self):
216+
"""Core shapes.DriftCheckBaselines is serialized with the core serializer."""
217+
from sagemaker.core.shapes import (
218+
DriftCheckBaselines,
219+
DriftCheckModelQuality,
220+
MetricsSource,
221+
)
222+
223+
drift_check_baselines = DriftCheckBaselines(
224+
model_quality=DriftCheckModelQuality(
225+
statistics=MetricsSource(
226+
content_type="application/json",
227+
s3_uri="s3://bucket/statistics.json",
228+
)
229+
)
230+
)
231+
232+
args = get_model_package_args(
233+
image_uri="test-image:latest",
234+
drift_check_baselines=drift_check_baselines,
235+
)
236+
237+
assert args["drift_check_baselines"] == {
238+
"ModelQuality": {
239+
"Statistics": {
240+
"ContentType": "application/json",
241+
"S3Uri": "s3://bucket/statistics.json",
242+
}
243+
}
244+
}
245+
246+
def test_get_model_package_args_with_dict_model_metrics(self):
247+
"""A dict already in request form is passed through unchanged."""
248+
model_metrics = {
249+
"ModelQuality": {
250+
"Statistics": {
251+
"ContentType": "application/json",
252+
"S3Uri": "s3://bucket/evaluation.json",
253+
}
254+
}
255+
}
256+
257+
args = get_model_package_args(
258+
image_uri="test-image:latest",
259+
model_metrics=model_metrics,
260+
)
261+
262+
assert args["model_metrics"] == model_metrics
263+
161264
def test_get_create_model_package_request_minimal(self):
162265
"""Test get_create_model_package_request with minimal parameters"""
163266
request = get_create_model_package_request(

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6891,9 +6891,13 @@ def register(
68916891
68926892
Args:
68936893
content_types (list[str] or list[PipelineVariable]): The supported MIME types
6894-
for the input data.
6894+
for the input data. When omitted, any value supplied to an earlier
6895+
``register()`` call is reused, otherwise it is left out of the request
6896+
(default: None).
68956897
response_types (list[str] or list[PipelineVariable]): The supported MIME types
6896-
for the output data.
6898+
for the output data. When omitted, any value supplied to an earlier
6899+
``register()`` call is reused, otherwise it is left out of the request
6900+
(default: None).
68976901
inference_instances (list[str] or list[PipelineVariable]): A list of the instance
68986902
types that are used to generate inferences in real-time (default: None).
68996903
transform_instances (list[str] or list[PipelineVariable]): A list of the instance
@@ -7002,8 +7006,8 @@ def register(
70027006
container_def_list = container_def if isinstance(container_def, list) else [container_def]
70037007

70047008
model_pkg_args = get_model_package_args(
7005-
self.content_types,
7006-
self.response_types,
7009+
getattr(self, "content_types", None),
7010+
getattr(self, "response_types", None),
70077011
inference_instances=inference_instances,
70087012
transform_instances=transform_instances,
70097013
model_package_name=model_package_name,

0 commit comments

Comments
 (0)