fix: preserve all core container fields in ModelBuilder CreateModel requests

[mohamedzeidan2021]

Issue #, if available: Fixes #6476

Description

ModelBuilder._core_container_to_dict() rebuilt each core ContainerDefinition by hand, copying only Image, ModelDataUrl, Environment and ImageConfig into container_def(). Everything else a ContainerDefinition can carry was discarded. For ModelBuilder(model=[<core Model>, ...]) that produced two distinct failures:

• Model-package reference. A container that references an approved model package has ModelPackageName and no Image. The converter dropped ModelPackageName and, because container_def() always writes an Image key, emitted {"Image": Unassigned(), "Environment": {}} — the container's only identifying field was gone and the request carried an internal sentinel object in a string field.
• ModelDataSource. Such a container keeps its image, so model creation proceeded, but the generated CreateModel request silently lost the model data source — the model would be created pointing at no artifacts.

While reproducing this I found the same field copy also drops AdditionalModelDataSources, Mode, ContainerHostname, InferenceSpecificationName, MultiModelConfig and ContainerMetricsConfig, and passes ImageConfig through as a pydantic shape object instead of a dict (also rejected by boto).

Why this fix. sagemaker.core.utils.utils.serialize() already emits the exact CreateModel request shape for every ContainerDefinition field — it is what sagemaker.core itself uses to build requests — so the container is now routed through it rather than rebuilt field by field. _core_container_to_dict() is reached only from _prepare_pipeline_container_defs() (the list-of-core-Models path); the single-model and framework paths still go through container_def() unchanged. I verified that sagemaker.core.resources.ContainerDefinition is sagemaker.core.shapes.ContainerDefinition, so there is no shadowed-class hazard here, and serialize() is a plain function (not pydantic.validate_call-wrapped), so it accepts the shape as given.

A container legitimately has no image when it references a model package, so the two container_def[...]["Image"] subscripts in _create_sagemaker_model() would now raise KeyError: 'Image'. They are replaced by a single .get("Image") computed once and reused by both the interactive-session base-name lookup and the Nova network-isolation check (previously the same expression twice).

botocore shape validation (before vs after). Using botocore.validate.ParamValidator against the real CreateModel input shape from botocore.session.get_session().get_service_model("sagemaker") — no network, no credentials — over nine container shapes routed through _create_sagemaker_model():

container	before	after
model-package reference	INVALID — Invalid type for parameter Containers[0].Image, value: , type: <class 'sagemaker.core.utils.utils.Unassigned'>, valid types: <class 'str'>	VALID — [{"ModelPackageName": "arn:...:model-package/example/1"}]
ModelDataSource	VALID but lossy — ModelDataSource absent from the request	VALID, ModelDataSource preserved
ImageConfig	INVALID — Invalid type for parameter Containers[0].ImageConfig ... type: <class 'sagemaker.core.shapes.shapes.ImageConfig'>, valid types: <class 'dict'>	VALID — {"ImageConfig": {"RepositoryAccessMode": "Vpc"}}
inference pipeline with a model-package container	INVALID — Containers[1].Image	VALID
AdditionalModelDataSources	VALID but lossy — field absent	VALID, field preserved
Mode: MultiModel	VALID but lossy — field absent	VALID, field preserved
plain image + ModelDataUrl + Environment	VALID	VALID, identical fields
2-container inference pipeline (plain images)	VALID	VALID, identical fields
image only, no environment	VALID	VALID (Environment is no longer emitted as an empty dict; it is optional in the API)

The only behavior change for containers that already worked is that an empty Environment is no longer emitted as {}. CreateModel treats Environment as optional, and the only consumers that index ["Environment"] (sagemaker.core.local.entities, _create_model's LOCAL_CONTAINER branch) are fed by the single-model container_def() path, which is untouched.

Testing

All offline. There is no live CreateModel round-trip in this evidence — I had no usable AWS credentials, so correctness against the service is argued via boto's own client-side parameter validator and the service model, not via a real API call.

Reproduced first against installed 3.25.0 with the issue's scenario under a PipelineSession: the ModelDataSource container produced Containers: [{"Image": "...", "Environment": {"EXAMPLE": "true"}}] with the data source gone, and the model-package container produced Containers: [{"Image": Unassigned(), "Environment": {}}]. Note: on 3.25.0 / master the model-package case does not raise KeyError: 'Image' as the issue reports — container_def() always inserts an Image key, so the failure instead surfaces as a botocore ParamValidationError at call time with ModelPackageName already lost. The KeyError is what the issue's suggested serialize()-only diff would introduce, which is why the .get("Image") guards are part of this change. The defect itself reproduces exactly as described.

Added TestCoreContainerDefinitionFields to sagemaker-serve/tests/unit/test_model_builder_deploy.py (6 tests), driving the real _create_sagemaker_model() and asserting on the container_defs handed to create_model:

• model-package container builds [{"ModelPackageName": ...}]
• ModelDataSource container keeps its data source
• plain image container is unchanged
• ImageConfig reaches the request as a dict
• model-package container works outside a PipelineSession (the base-name lookup path)
• the before/after payloads against the real CreateModel input shape via botocore.validate.ParamValidator

test_core_container_to_dict in the same file was rewritten: it asserted that container_def was called, which is exactly the implementation detail this change removes. It now asserts the resulting dict for a plain container and passes both before and after the fix.

Negative check (fix reverted, tests kept): 5 of the 6 new tests fail; test_plain_image_container_is_unchanged passes both ways by design, as does the rewritten test_core_container_to_dict.

# sagemaker-serve, fix applied
pytest tests/unit/test_model_builder_deploy.py   ->  30 passed, 6 skipped

# sagemaker-serve, fix reverted (new tests kept)
pytest tests/unit/test_model_builder_deploy.py   ->   5 failed, 25 passed, 6 skipped

Full sagemaker-serve regression suite, with and without the fix:

with fix:      70 failed, 2076 passed, 25 skipped
without fix:   70 failed, 2070 passed, 25 skipped   (the 6 new tests absent)

The two failure sets are identical. All 70 are environmental in my sandbox: 65 fail with ClientError: ExpiredToken on GetCallerIdentity (no valid credentials) and 5 with ModuleNotFoundError: No module named 'tensorflow'. No other package was touched.

Lint on both changed files: black --line-length 100 --check clean, flake8 --max-line-length 120 clean, pydocstyle clean on the src file. pylint --rcfile=.pylintrc on src/sagemaker/serve/model_builder.py: 9.84/10 before and after, with the 29 findings identical apart from one line number shifting by 1.

Merge Checklist

Put an x in the boxes that apply.

General

• I have read the CONTRIBUTING doc
• I certify that the changes I'm 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

• 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 you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[github-actions]

🤖 AI code review

This PR fixes ModelBuilder._core_container_to_dict() so it routes core ContainerDefinition objects through sagemaker.core.utils.utils.serialize() instead of rebuilding a fixed subset of fields, preserving ModelPackageName, ModelDataSource, ImageConfig (as a dict) and other fields in CreateModel requests. It also replaces two container_def[...]["Image"] subscripts in _create_sagemaker_model() with a single .get("Image") lookup so model-package references without an Image no longer raise KeyError. The change is correct, consistent with how sagemaker.core builds requests, and is backed by thorough unit tests including real botocore param validation. No blocking issues found.

_{Reviewed commit 2e87f0d. Automated review; verify before acting.}