[e-davidson]

Issue

In the v2 SDK, ModelBuilder selected the model spec's default instance type when the user did not specify compute. In v3 this silently regressed: every no-compute JumpStart build falls back to ml.m5.large regardless of what the model spec recommends. This affects public hub deploys as well.

Root cause

__post_init__ calls _initialize_compute_config() before self.region is assigned. The spec-resolution chain inside it raises AttributeError, which a bare except Exception: pass swallows, so the builder silently takes the hardcoded ml.m5.large fallback instead of the spec default.

Fix

• Extract _initialize_region() and call it first in __post_init__, so region is available before compute config initialization.
• Replace the bare except Exception: pass with a warning log, so future failures in the spec-resolution chain are visible instead of silently changing behavior.

Tests

Mirrors master-v2's testing approach for this behavior: unit tests with mocking, no E2E.

• Fixture: a real JumpStartModelSpecs built from master-v2 constants.py BASE_SPEC; JumpStartModelsAccessor.get_model_specs is patched to return it (the real spec-resolution chain runs, only the network fetch is mocked).
• The fixture's default instance type is set to ml.g5.12xlarge with ml.m5.large excluded from supported types, so the tests discriminate the buggy fallback from the correct spec default.
• 10 unit tests, red-green verified against current master: 8 fail on baseline (ml.m5.large != ml.g5.12xlarge), 10/10 pass with the fix. Existing private hub unit suite (17 tests) unaffected.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

Claude finished @evakravi's task in 1m 42s —— View job

---

Review tasks

• Read the PR diff
• Inspect changed source (model_builder.py, model_builder_utils.py) in context
• Assess correctness, best practices, tests
• Post review feedback

Review summary

The fix is correct and well-targeted. The root-cause diagnosis holds up against the code: _initialize_compute_config() (model_builder.py:506) calls _get_default_instance_type() → _get_jumpstart_recommended_instance_type() (model_builder_utils.py:311), which reads self.region. Previously region was only assigned inside _initialize_defaults(), which runs after _initialize_compute_config(), so the JumpStart lookup raised AttributeError, got swallowed by the bare except Exception: pass, and the builder silently fell through to the ml.m5.large default (model_builder_utils.py:385).

Extracting _initialize_region() and calling it first in __post_init__ (model_builder.py:476) resolves the ordering, and replacing the bare except with a logger.warning restores visibility. Both changes are sound.

Correctness / behavior

• The ordering fix is right. self.model and self.compute are dataclass fields, so _is_jumpstart_model_id() and _initialize_compute_config() have everything they need once region exists.
• _initialize_region() is idempotent (guarded by if not hasattr(self, "region") or not self.region), so the behavior for an explicitly-passed region or sagemaker_session is unchanged — this is purely a bug fix for the no-compute JumpStart path.
• The warning-log change only fires on the JumpStart branch (it's reached solely via _get_default_instance_type's _is_jumpstart_model_id() guard), and self.model there is always a string model id, so the log formatting is safe.
• This does change the resolved default for no-compute JumpStart builds (ml.m5.large → spec default). That's the intended restoration of v2 behavior and is the point of the PR, not a regression.

Tests — thorough and genuinely discriminating. Overriding the fixture default to ml.g5.12xlarge and dropping ml.m5.large from the supported list is the right way to tell the buggy fallback apart from a correct spec resolution. TestSpecFixtureDrivenResolution exercising the real get_deploy_kwargs → retrieve_default chain (mocking only the spec fetch) is the strongest guard here. I confirmed the logger assertion in test_spec_lookup_failure_logs_warning_and_falls_back is correct: model_builder_utils.py imports logger from sagemaker.core.utils.utils (model_builder_utils.py:58), matching assertLogs("sagemaker.core.utils.utils", ...).

Minor observations (non-blocking)

• _initialize_region() is now invoked twice per construction — once in __post_init__ (model_builder.py:476) and again inside _initialize_defaults() (model_builder.py:577). The second call is a guarded no-op; the inline comment explains it keeps the existing test_initialize_defaults_region_from_boto3 coverage test working. Harmless, just slightly redundant — fine to leave as documented.
• The new logger.warning will now surface on every construction where the JumpStart spec lookup fails (e.g. gated or unavailable models), where before it was silent. That's the intended tradeoff (visibility over silent misbehavior) and the message correctly describes the fallback, but it's a small increase in log noise worth being aware of.

No correctness, security, or backward-compatibility concerns beyond the above. Nice red/green discipline on the tests.

Note: the inline-comment tool wasn't available in this run, so the per-location notes are consolidated here with file:line references.

[github-actions]

Claude finished @evakravi's task in 1m 8s —— View job

---

PR Review: fix(serve): restore JumpStart default instance type from model spec

• Read the PR diff
• Inspect changed source in context (model_builder.py, model_builder_utils.py)
• Assess correctness, best practices, tests
• Post review feedback

---

Verdict: LGTM. The fix is correct, minimal, and well-targeted. The root-cause diagnosis holds up against the code.

Correctness

• Ordering fix is right. _initialize_compute_config() (model_builder.py:507) calls _get_default_instance_type() → _get_jumpstart_recommended_instance_type() (model_builder_utils.py:311), which reads self.region. Previously self.region was only assigned inside _initialize_defaults(), which ran after _initialize_compute_config(), so the lookup raised AttributeError, got swallowed by the bare except, and the builder fell through to the hardcoded ml.m5.large (model_builder_utils.py:386). Extracting _initialize_region() and calling it first in __post_init__ (model_builder.py:477) fixes the regression.
• _initialize_region() is idempotent (guarded by if not hasattr(self, "region") or not self.region), so an explicitly-passed region/sagemaker_session is unaffected — this is purely a bug fix for the no-compute JumpStart path.
• Warning-log formatting is safe. The JumpStart branch is only reached via _get_default_instance_type's isinstance(self.model, str) and self._is_jumpstart_model_id() guard (model_builder_utils.py:353), so self.model is always a string model id when the warning fires. The lazy %s formatting is the right logging style.
• Behavior change is intentional. No-compute JumpStart builds now resolve the spec default instead of ml.m5.large — the restoration of v2 behavior this PR targets, not a regression.

Tests — thorough and genuinely discriminating

• Overriding the fixture default to ml.g5.12xlarge and dropping ml.m5.large from the supported list is the correct way to tell the buggy fallback apart from a real spec resolution.
• TestSpecFixtureDrivenResolution exercising the real get_deploy_kwargs → retrieve_default chain (mocking only the spec fetch) is the strongest guard.
• Confirmed assertLogs("sagemaker.core.utils.utils", ...) in test_spec_lookup_failure_logs_warning_and_falls_back matches the actual logger import (model_builder_utils.py:58), so that assertion is sound.

Minor observations (non-blocking)

• _initialize_region() runs twice per construction — once in __post_init__ (model_builder.py:477) and again inside _initialize_defaults() (model_builder.py:578). The second is a guarded no-op; the inline comment documents it keeps a direct-_initialize_defaults test passing. Harmless, just slightly redundant.
• Increased log noise. The new logger.warning now fires on every no-instance-type construction where the spec lookup genuinely fails (e.g. gated/unavailable models), where before it was silent. That's the intended visibility tradeoff and the message correctly describes the fallback — just worth being aware of.
• Eager spec lookup at construction. Building a JumpStart ModelBuilder with no instance_type now performs the get_deploy_kwargs network/spec lookup during __post_init__ (it did before too — it just always failed). This matches v2 behavior; no action needed.

No correctness, security, or backward-compatibility concerns. Nice red/green discipline on the tests.

Note: the inline-comment tool wasn't available in this run, so per-location notes are consolidated here with file:line references.

[mohamedzeidan2021]

Test failures unrelated to the fix besides the codestyle. Can you make fixes that arise from codestyle-doc-tests (sagemaker-serve)