Skip to content

fix(train): Fall back to public hub when private hub lacks base model#6092

Merged
lucasjia-aws merged 2 commits into
aws:masterfrom
lucasjia-aws:fix/tests
Jul 23, 2026
Merged

fix(train): Fall back to public hub when private hub lacks base model#6092
lucasjia-aws merged 2 commits into
aws:masterfrom
lucasjia-aws:fix/tests

Conversation

@lucasjia-aws

Copy link
Copy Markdown
Collaborator

When resolving a model package's base model, the reconstructed hub-content ARN honored SAGEMAKER_HUB_NAME but never verified the base model actually existed in that private hub, and never fell back. If the private hub had not mirrored the base model (or its ModelReference was cleaned up), server-side CreateTrainingJob failed with "Hub content ... does not exist", breaking evaluation integ tests (benchmark, custom scorer, and LLM-as-judge, including custom scorer with a built-in metric).

Verify the base model exists in the configured private hub and fall back to SageMakerPublicHub when it is absent, mirroring the existing fallback in _resolve_jumpstart_model. Base models are always published to the public hub, so this keeps evaluation working regardless of private-hub contents. The public-hub path is unchanged and does no extra lookups.

Issue #, if available:

Description of changes:

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

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

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


Review: PR #6092 — Fall back to public hub when private hub lacks base model

  • Read PR diff
  • Analyze model_resolution.py changes
  • Verify the _resolve_jumpstart_model fallback pattern being mirrored
  • Check HubContent.get / get_sagemaker_hub_name signatures
  • Review test coverage

The change is correct, well-scoped, and mirrors the existing _resolve_jumpstart_model fallback faithfully. The HubContent.get(...) signature (hub_name, hub_content_type, hub_content_name, hub_content_version, session, region) matches sagemaker-core/src/sagemaker/core/resources.py:13721, and the public-hub path adds no extra lookups as claimed. Both the "private hub has it" and "private hub lacks it → fall back" paths are covered by new unit tests. Nice regression note in the test docstring.

A few minor points, none blocking:

1. Broad except Exception masks transient failures as "not found" (model_resolution.py:508)

Any exception — throttling, AccessDenied, a transient network error, or even a failed self._get_session() inside the try — is treated as "base model absent" and silently falls back to SageMakerPublicHub. Since base models are always published to the public hub, the functional outcome is still safe. But a genuine permission/throttling problem against the private hub gets logged only at info and produces a public-hub ARN that may not be what the caller intended. This matches the existing _resolve_jumpstart_model pattern, so it's consistent — just worth being aware that the fallback is unconditional rather than "not-found-specific." If you wanted to tighten it, you could narrow to ResourceNotFound / ValidationException and re-raise unexpected errors, but consistency with the existing pattern is a reasonable argument for leaving it.

2. Stray empty comment line (model_resolution.py:374)

The diff leaves a dangling # (empty comment) between the existing block comment and the new hub_name = self._resolve_base_model_hub(...) call. Harmless, but you may want to drop it for cleanliness.

3. session is fetched inside the try (model_resolution.py:498)

self._get_session() is called inside the try, so a session-creation failure also routes to the fallback. Minor, and arguably fine, but hoisting it above the try would keep the fallback scoped strictly to the HubContent.get lookup.

Overall this looks good to merge — the items above are optional polish, not correctness issues.

When resolving a model package's base model, the reconstructed hub-content
ARN honored SAGEMAKER_HUB_NAME but never verified the base model actually
existed in that private hub, and never fell back. If the private hub had
not mirrored the base model (or its ModelReference was cleaned up),
server-side CreateTrainingJob failed with "Hub content ... does not exist",
breaking evaluation integ tests (benchmark, custom scorer, and
LLM-as-judge, including custom scorer with a built-in metric).

Verify the base model exists in the configured private hub and fall back
to SageMakerPublicHub when it is absent, mirroring the existing fallback
in _resolve_jumpstart_model. Base models are always published to the
public hub, so this keeps evaluation working regardless of private-hub
contents. The public-hub path is unchanged and does no extra lookups.
@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude finished @lucasjia-aws's task in 52s —— View job


Review: PR #6092 — Fall back to public hub when private hub lacks base model

  • Read PR diff
  • Review _resolve_base_model_hub and call site
  • Verify HubContent.get signature and fallback pattern
  • Check test coverage
  • Post review

The change is correct and well-scoped. Verified against the checked-out repo:

  • HubContent.get signature matchessagemaker-core/src/sagemaker/core/resources.py:13721 accepts hub_name, hub_content_type, hub_content_name, hub_content_version, session, region. The call in _resolve_base_model_hub (model_resolution.py:499) uses them all correctly.
  • Faithfully mirrors the existing fallback in _resolve_jumpstart_model (model_resolution.py:267-289): guard on SageMakerPublicHub to skip the extra lookup, try/fall-back on failure, base models always exist in the public hub.
  • Public-hub path adds no extra lookups — the early return hub_name at line 492 short-circuits before touching HubContent.get, as claimed.
  • Both paths are covered by new unit tests (private-hub-has-it and private-hub-lacks-it → fallback), with a clear regression note.

A couple of minor, non-blocking observations:

  1. Broad except Exception treats any failure as "not found" (model_resolution.py:508). Throttling, AccessDenied, or a transient network error all route to the public-hub fallback rather than surfacing. Because base models are always published to the public hub the functional outcome is safe, and this matches the existing _resolve_jumpstart_model pattern — so consistency is a fair argument for leaving it. If you ever want to tighten it, narrowing to ResourceNotFound / ValidationException and re-raising the rest would keep the fallback scoped to genuine absence.

  2. Stray empty comment (model_resolution.py:374) — the dangling # between the block comment and the hub_name = self._resolve_base_model_hub(...) call is harmless but could be dropped for cleanliness.

  3. Session fetched inside the try (model_resolution.py:498) — a _get_session() failure also routes to the fallback. Minor; hoisting it above the try would scope the fallback strictly to the HubContent.get lookup.

None of these are correctness issues. LGTM.

@lucasjia-aws
lucasjia-aws merged commit 9664509 into aws:master Jul 23, 2026
15 of 22 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants