Skip to content

fix(serve): restore JumpStart default instance type from model spec - #6045

Open
e-davidson wants to merge 4 commits into
aws:masterfrom
e-davidson:fix/jumpstart-default-instance-type-v2
Open

fix(serve): restore JumpStart default instance type from model spec#6045
e-davidson wants to merge 4 commits into
aws:masterfrom
e-davidson:fix/jumpstart-default-instance-type-v2

Conversation

@e-davidson

@e-davidson e-davidson commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

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.

Eli Davidson
fix(serve): restore JumpStart default instance type from model spec
89bf2d8 
In v2, ModelBuilder selected the model spec's default instance type
when the user did not specify compute. In v3 this silently regressed:
__post_init__ calls _initialize_compute_config() before self.region is
assigned, raising AttributeError inside the spec-resolution chain that
a bare 'except Exception: pass' swallows, so every no-compute build
falls back to ml.m5.large instead of the spec default. Affects public
hub deploys too.

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

Tests (mirrors master-v2's approach, unit-only with mocking, no E2E):
- Fixture is a real JumpStartModelSpecs built from master-v2
  constants.py BASE_SPEC; JumpStartModelsAccessor.get_model_specs is
  patched to return it. The spec default is set to ml.g5.12xlarge and
  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 on current master: 8 fail on
  baseline (ml.m5.large != ml.g5.12xlarge), 10/10 pass with the fix.
  Private hub unit suite (17 tests) unaffected.
Eli Davidson
test(serve): pin user instance-type override behavior end to end
c5b1d20 
Add TestUserOverrideFlowsDownstream (4 tests) closing the coverage gap
between the override-wins assertions and the downstream-forwarding
assertions, which previously only used the spec default:

- deploy() hands the user's instance type (not the spec default) to
  _deploy
- _deploy_core_endpoint places the user's instance type into the
  ProductionVariant for CreateEndpointConfig
- _build_for_jumpstart forwards the user's instance type to
  get_init_kwargs, which drives instance-type-specific image URI and
  env var selection
- instance_type via the Compute object behaves identically to a direct
  instance_type: honored verbatim, spec lookup skipped

These are behavior pins, not regression discriminators: they pass on
both baseline and fix (override was never broken; the bug only affected
the no-compute default path). Suite is now 14 tests: baseline 8 failed
6 passed, with fix 14/14.
@evakravi
evakravi deployed to manual-approval October 5, 2026 00:37 — with GitHub Actions Active
@evakravi
evakravi deployed to manual-approval October 5, 2026 00:37 — with GitHub Actions Active
@evakravi
evakravi deployed to manual-approval October 5, 2026 00:37 — with GitHub Actions Active
@github-actions

github-actions Bot commented Oct 6, 2026 •
edited
Loading

Copy link
Copy Markdown

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.

This branch is waiting to be deployed

1 waiting deployment
manual-approval — 0e232eac Waiting Oct 7, 2026 by evakravi via wait-for-approval #556
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@e-davidson @evakravi