[lucasjia-aws]

Issue

Fixes #5760. Supersedes #5910.

Problem

ModelBuilder.build() fails when image_uri is a pipeline variable such as ParameterString under a PipelineSession, even though ModelBuilder.image_uri is typed Optional[StrPipeVar]. This blocks V2 -> V3 pipeline migrations that parameterize the serving image.

Root cause

The build path treats image_uri as a plain string in two places. First, _build_validations calls is_1p_image_uri(self.image_uri), which slices the value with image_uri[0:12] and raises TypeError: 'ParameterString' object is not subscriptable (the error in the issue). Second, once that is fixed, _create_sagemaker_model evaluates "nova-" in resolved_image_uri for the Nova network-isolation default and raises TypeError: argument of type 'ParameterString' is not iterable. PipelineVariable intentionally defines neither __getitem__ nor __contains__, and its __str__ raises.

#5910 only addresses the first failure, so the reporter's snippet still fails with it applied.

Fix

• is_1p_image_uri returns False for any non-string value. A pipeline variable is only resolved at execution time, so its account cannot be inspected at build time. On the image-only passthrough path both branches of _build_validations lead to passthrough, so the result is unchanged; with a model and no model_server, users now get the existing "Model_server must be set" error instead of a TypeError.
• The Nova network-isolation default in _create_sagemaker_model only inspects string images. For a pipeline-variable image, set enable_network_isolation explicitly if needed.
• The pipeline variable is passed through unchanged, so the pipeline definition renders "Image": {"Get": "Parameters.<name>"}.

Testing

• New tests/unit/test_model_builder_pipeline_variable_image_uri.py: _build_validations passthrough and model_server cases, the _create_sagemaker_model network-isolation check with a pipeline-variable image and a Nova 1P string image (regression guard), and an end-to-end build() under a real PipelineSession asserting the CreateModel request keeps the ParameterString.
• tests/unit/validations/test_check_image_uri.py: ParameterString (with a 1P default value), Join, and None inputs.
• The new tests fail on master and pass with this change. The sagemaker-serve unit test failure set is identical before and after the change.
• Manually verified that a ModelStep built from the result produces a pipeline definition with the parameterized image.

[github-actions]

Claude finished @lucasjia-aws's task in 1m 22s —— View job

---

Review

I reviewed the diff against origin/master and traced every caller of the changed function. This is a focused, correct fix. Summary: looks good to merge.

(Note: the inline-comment tool isn't available in this run, so findings are consolidated here with file/line references.)

Correctness ✅

• is_1p_image_uri guard (check_image_uri.py:330-331): Returning False for any non-str is the right call. A PipelineVariable is unresolved at build time, so its account genuinely can't be inspected, and False (not-1P) is the safe classification. I checked all four callers and none regress:
  • model_builder.py:2058 / :2068 — for the image-only case both branches set _passthrough = True regardless, so the result is unchanged (as the PR notes).
  • model_builder.py:2077 — a pipeline-variable image with a model but no model_server now raises the intended "Model_server must be set" ValueError instead of a TypeError. Good.
  • model_builder.py:3701 and model_server/torchserve/prepare.py:56 — the and "xgboost" in image_uri on line 56 is now short-circuited by the leading False, so it never hits the in-on-a-pipeline-var TypeError.
  • utils/telemetry_logger.py:295 — unaffected; also now more robust to None.
• Nova network-isolation guard (model_builder.py:3148): adding isinstance(resolved_image_uri, str) before "nova-" in resolved_image_uri correctly prevents the __contains__ TypeError. The 1P-string regression path is preserved and covered by a test.

Types

• Optional[StrPipeVar] where StrPipeVar = Union[str, PipelineVariable] (pipeline_variable.py:84) is accurate for the callers that pass self.image_uri (typed Optional[StrPipeVar]) and for the None case now handled.

Tests ✅

• Good coverage: _build_validations passthrough + model_server cases, the _create_sagemaker_model isolation check for both a pipeline-var image and a Nova 1P string (regression guard), plus a real-PipelineSession end-to-end build() asserting the ParameterString survives into the CreateModel request. The unit-level check_image_uri tests cover ParameterString (with a 1P default), Join, and None.

Minor observations (non-blocking)

• The new test module uses unittest.TestCase while most of sagemaker-serve/tests/unit uses plain pytest functions. Both run under pytest, so this is purely a style consistency nit.
• test_is_1p_image_uri_pipeline_variable_returns_false deliberately gives the ParameterString a 1P default value to prove the default is not inspected — a nice touch worth keeping.

No security or performance concerns; the change removes failure modes rather than adding surface area, and adheres to the repo's v3-by-default guidance (all examples/tests use sagemaker.serve.ModelBuilder and sagemaker.core.workflow).

[mohamedzeidan2021]

Approving. Reproduced TypeError: 'ParameterString' object is not subscriptable on master and its absence after, plus end-to-end through ModelStep + Pipeline.definition() correctly rendering {"Get": "Parameters.ServingImageUri"}, and adversarially across Join, ExecutionVariable, Properties, and a ParameterString whose default value is itself a 1P image (correctly not consulted). All 7 new tests fail on reverted source. The unit-test diff is decisive: 26 failures pre-fix vs 23 post-fix, with the 23 an identical set — no regressions.

Skip is the right semantics, and I checked the alternatives rather than taking it on faith. Deferring resolution is impossible: EnableNetworkIsolation is a static boolean baked into the pipeline definition at definition time, while the image string only exists at execution time, so there's no expression form for "1P-ness of the resolved image". Erroring would break the legitimate case the issue asks for. I also confirmed the skip is a genuine no-op on the passthrough path — 1P and 3P literal images produce byte-identical CreateModel requests, because both branches set _passthrough = True and return.

The #5910 comparison holds up: restoring only check_image_uri.py still fails at the Nova check with TypeError: argument of type 'ParameterString' is not iterable, so this PR's claim that #5910 is insufficient is independently verified. Nothing of substance is lost — the only thing #5910 had that this drops is hardening of validate_image_uri_and_hardware, which has zero src/ callers. (Minor correction for the record: #5910 was authored by sagemaker-bot, not a community contributor, so no human attribution is owed.)

One request I'd like before merge, not blocking this approval: the Nova network-isolation default is now silently skipped for a pipeline-variable image, with only an internal code comment. Nova 1P account IDs are region-specific, so parameterising the image to switch regions is realistic, and ModelBuilder(image_uri=ParameterString(...)) with a Nova default yields a definition with no EnableNetworkIsolation where the literal yields True — the user finds out at pipeline execution time. A one-line logger.warning when the image is a pipeline variable and _enable_network_isolation is unset would close it, matching the existing convention around model_builder.py:2726 for pipeline-variable model_data.

Two smaller notes: consider using the already-imported is_pipeline_variable() (line 152, used at 2675/2726/2895/2911, and the same guard sagemaker-core uses in fw_utils.py/common_utils.py) instead of hand-rolled isinstance(x, str) — functionally a superset so there's no bug, but it diverges from convention and silently reclassifies genuinely wrong types (None, an int, a Model) from fail-fast to "not 1P". And there's no guard for a pipeline-variable image_uri under a non-PipelineSession, where the user now gets an opaque __str__ failure deep inside botocore instead of the old clear TypeError; model_builder.py:2727 has the established warn-pattern for that.