Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); fix: resolve private hub Models and aliased references for modelTrainer by tanvikab4 · Pull Request #6201 · aws/sagemaker-python-sdk · GitHub
Skip to content

fix: resolve private hub Models and aliased references for modelTrainer - #6201

Open
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer
Open

fix: resolve private hub Models and aliased references for modelTrainer#6201
tanvikab4 wants to merge 1 commit into
aws:masterfrom
tanvikab4:fix/jumpstart-private-hub-modeltrainer

Conversation

@tanvikab4

Copy link
Copy Markdown

Description of changes

Summary

ModelTrainer.from_jumpstart_config(...) could not resolve models that live in a private hub — only public
JumpStart models and (by a fragile assumption) plain private-hub references worked. This change fixes hub-content
resolution so ModelTrainer reaches parity with ModelBuilder, supporting:

  • Public JumpStart models (unchanged)
  • Model References in a private hub (pointer to a public model)
  • Aliased references — filed under a hub content name that differs from the public model_id
  • Privately-owned Models authored directly into a private hub

Root cause

sagemaker.core.jumpstart.document.get_hub_content_and_document() guessed the hub content type from the hub name:

hub_content_type = "Model" if hub_name == SAGEMAKER_PUBLIC_HUB else "ModelReference"

A private hub can hold either a Model or a ModelReference. This guess meant:

  • Privately-owned Models were looked up as ModelReference → ResourceNotFound → resolution failed.
  • The lookup used model_id and ignored hub_content_name, so aliased references were never found.

Fix (sagemaker-core/src/sagemaker/core/jumpstart/document.py)

  • Replace the guess with a probe: for a private hub, try ModelReference first, then fall back to Model; the
    public hub uses Model only. This mirrors ModelBuilder's resolution in accessors.py.
  • Honor hub_content_name (falling back to model_id) so aliased references resolve.
  • On miss, raise a combined error naming both content types attempted.

No changes were needed elsewhere: defaults.py already attaches HubAccessConfig based on hub_content_type, and
model_trainer.py / JumpStartConfig already support hub_name/hub_content_name — they become correct automatically
once the content type is resolved honestly.

Testing

Unit (sagemaker-core/tests/unit/jumpstart/test_document.py) — 5 new tests, all passing:

  • public hub resolves as Model (single lookup, no probe)
  • private-hub reference resolves on the first probe
  • private-hub Model resolves via the fallback (asserts probe order ["ModelReference", "Model"])
  • hub_content_name alias is used for lookup
  • neither type present → raises after attempting both

Integration (sagemaker-train/tests/integ/jumpstart/test_jumpstart_train.py) — 3 new tests, each creates a
temporary private hub, runs a real training job, and tears down (skips gracefully without hub permissions). All
verified passing end-to-end against AWS:

  • test_jumpstart_train_from_private_hub_reference
  • test_jumpstart_train_from_aliased_reference
  • test_jumpstart_train_from_private_owned_model

deadline = time.time() + timeout
while time.time() < deadline:
try:
resp = sm.list_hub_contents(HubName=hub_name, HubContentType=content_type)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use describe_hub_content directly?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switched _wait_for_content to use describe_hub_content directly


def _default_training_dataset(region, model_id):
"""Resolve the model's default training dataset S3 URI from JS metadata."""
from sagemaker.core.jumpstart.accessors import JumpStartModelsAccessor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's avoid the lazy imports, claude loves to add them for some reason lol

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there anything to assert or just validating nothing is thrown?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test's purpose is to verify the aliased-reference resolution path so when hub_content_name differs from the public model_id, from_jumpstart_config resolves the reference by its alias. I added these assertions to strengthen the test:

  1. _jumpstart_config.hub_content_name == ALIASED_REFERENCE_NAME (the alias was threaded through resolution)
  2. training_image is set
  3. the model channel's S3 source carries a HubAccessConfig with a hub_content_arn

model_trainer.train()


def test_jumpstart_train_from_private_hub_reference(private_hub, sagemaker_session):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a unit test for training a model reference to a gated model? there should have been a similar test in v2 so you can use that same model. There's some ModelAccessConfig/accept_eula stuff that we should verify works

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a unit test class TestJumpStartTrainDefaultsGatedModelReferenceEula in test_defaults.py using the same gated model as v2 (mocks the resolver seam and verifies the
ModelAccessConfig/accept_eula behavior)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant integ test. My bad

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might have missed it but do we have this as an integ test?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the v2 tests and there exists a gated private hub test on the ModelBuilder/deploy side, but not one for the training path. I'll add a corresponding integ test for ModelTrainer rn

HubDescription="SDK integ test JumpStart training private hub",
)
except ClientError as e:
pytest.skip(f"Cannot create private hub (missing permissions?): {e}")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipping gracefully without hub permissions is the right intent, but the implementation is wider than that: every setup step here is a bare except ClientError → skip (create_hubcreate_hub_content_referenceimport_hub_contentdescribe_hub_content, and both _wait_for_content timeouts).

The assertion itself fails loudly — from_jumpstart_config isn't wrapped. But if import_hub_content for a private-hub Model breaks service-side, test_jumpstart_train_from_private_owned_model skips and CI stays green, so the test for the core case in this fix silently stops running.

Suggest matching only the specific authorization codes you expect in a restricted account and letting everything else fail; _wait_for_content returning False should be a pytest.fail once the hub was creatable.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a _skip_if_unauthorized helper that skips only on an authorization allowlist (AccessDeniedException/AccessForbiddenException/UnauthorizedOperation) and re-raises everything else

compute=Compute(instance_type="ml.m5.xlarge"),
sagemaker_session=sagemaker_session,
)
model_trainer.train(input_data_config=[InputData(channel_name="training", data_source=dataset)])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explicit training channel bypasses the hub-aware channel construction the fix enables. _create_training_job_args merges with "method parameter taking precedence" (existing_channels[new_input.channel_name] = new_input), so this bare InputData replaces the channel built by JumpStartTrainDefaults.get_training_dataset_input — the only place ModelAccessConfig and HubAccessConfig(hub_content_arn=...) are attached (defaults.pyhub_content_type == "ModelReference" branch).

So the reference and alias tests never assert that a HubAccessConfig was derived from the resolved hub content, and with a non-gated model like catboost-regression-model they'd pass even if that plumbing were wrong.

The existing test_jumpstart_train calls train() with no arguments and relies on SDK-resolved channels — suggest the same here and dropping _default_training_dataset, which also removes the hardcoded jumpstart-cache-prod-{region} bucket.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped explicit channel and _default_training_dataset. Train() now uses SDK-resolved channels like test_jumpstart_train. Also added assertions on the resolved channels so the plumbing is guarded

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b0a2b7b to f66b654CompareAugust 27, 2026 19:54
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 27, 2026 20:06 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 23s —— View job


PR Review: private hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Review new unit tests
  • Review integ tests
  • Post findings

Overall this is a clean, well-tested fix. The root cause is correctly identified (guessing hub_content_type from the hub name) and the probe + hub_content_name alias handling are the right shape. Tests are thorough — unit tests for the probe/alias/fallback paths, mocked defaults tests for the gated EULA path, and real integ tests. Two prior approvals already; the notes below are the only things I'd raise.

(Note: the inline-comment tool wasn't available to me this run, so findings are inline below with file/line refs.)


1. Fallback only triggers on ResourceNotFound — narrower than the ModelBuilder path it claims to mirror

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The PR comment says this "mirrors ModelBuilder's resolution in accessors.py", but accessors.py:303 catches a broad Exception on the first (ModelReference) probe and falls back to Model on any failure. Here, the fallback to Model only happens if the ModelReference lookup returns exactly ResourceNotFound.

The core case this fix enables — a privately-owned Model — relies on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a hub that holds a Model under that name returns something other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model, so private-owned Models would fail to resolve.

The narrower catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception in accessors.py arguably shouldn't), so I'm not asking you to widen it blindly. But worth confirming the exact error code the service returns for a wrong-HubContentType describe, and either (a) verifying it's ResourceNotFound, or (b) adding ValidationException to the fallback set. The test_private_hub_falls_back_to_model unit test hard-codes ResourceNotFound (_not_found()), so it wouldn't catch this — the only thing exercising it is the scheduled/gated integ test.

2. Minor: unnecessary getattr defensiveness

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on the (frozen, extra="forbid") JumpStartConfig, so it always exists. The getattr(jumpstart_config, "hub_content_name", None) can be simplified to:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error when last_error could be falsy

sagemaker-core/src/sagemaker/core/jumpstart/document.py:91-98

In practice content_types_to_try is never empty and last_error is always set before hub_content is None becomes reachable, so this is safe today. Just flagging that it's an implicit invariant — raise None would produce a confusing TypeError if the list ever became empty. Not worth changing unless you want a belt-and-suspenders raise last_error or RuntimeError(...).


Nothing here blocks merge. Point #1 is the only one I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it.

@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from b5afc72 to 1d87d98CompareAugust 31, 2026 19:58
@tanvikab4
tanvikab4force-pushed the fix/jumpstart-private-hub-modeltrainer branch from 1d87d98 to f67a2abCompareAugust 31, 2026 20:06
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@tanvikab4
tanvikab4deployed to manual-approval August 31, 2026 20:44 — with GitHub Actions Active
@github-actions

github-actionsBot commented Aug 31, 2026

Copy link
Copy Markdown

Claude finished @tanvikab4's task in 1m 18s —— View job


PR Review: private-hub Models & aliased references for ModelTrainer

  • Read the PR diff
  • Review document.py change (correctness)
  • Check accessors.py for the pattern it claims to mirror
  • Review new unit & integ tests
  • Post review

The fix is well-targeted and the root-cause analysis is correct: guessing hub_content_type from the hub name is wrong because a private hub can hold either a ModelReference or an owned Model. Probing + honoring hub_content_name is the right shape, and the test coverage (unit probe/alias/fallback, mocked gated-EULA defaults, and real integ tests) is thorough. Already has two approvals; nothing below blocks merge.

(The inline-comment tool wasn't available this run, so findings are listed with file/line refs.)


1. Fallback catch is narrower than the accessors.py path it claims to mirror — could break the private-owned-Model case

sagemaker-core/src/sagemaker/core/jumpstart/document.py:85-89

exceptClientErrorase:
ife.response["Error"]["Code"] =="ResourceNotFound":
last_error=econtinueraisee

The comment says this "mirrors ModelBuilder's resolution in accessors.py," but accessors.py:303 catches a broad Exception on the ModelReference probe and falls back to Model on any failure. Here, the fallback to Model only runs when the ModelReference lookup returns exactly ResourceNotFound.

The core case this PR enables — a privately-owned Model — depends on the ModelReference probe missing so the Model probe runs. If DescribeHubContent with HubContentType=ModelReference against a name that exists only as a Model returns anything other than ResourceNotFound (e.g. ValidationException), this re-raises immediately and never tries Model. test_private_hub_falls_back_to_model hard-codes _not_found() (ResourceNotFound), so it wouldn't catch that — only the gated/scheduled integ test would, and that doesn't run on PR CI.

The narrow catch is defensible (it avoids swallowing throttling/auth errors, which the broad except Exception arguably shouldn't). But please either (a) confirm the service actually returns ResourceNotFound for a wrong-HubContentType describe, or (b) add ValidationException to the fallback set — and either way, drop or soften the "mirrors accessors.py" comment since the behavior deliberately differs.

2. Minor: unnecessary getattr

sagemaker-core/src/sagemaker/core/jumpstart/document.py:56-60

hub_content_name is a declared field on JumpStartConfig (configs.py:51, a frozen extra="forbid" model), so it always exists. Simplify:

hub_content_name=jumpstart_config.hub_content_nameorjumpstart_config.model_id

Non-blocking.

3. Minor: raise last_error relies on an implicit invariant

sagemaker-core/src/sagemaker/core/jumpstart/document.py:98

Safe today — content_types_to_try is never empty and last_error is always set before hub_content is None is reached. Just noting raise None would surface as a confusing TypeError if that invariant ever broke; a raise last_error or RuntimeError(...) would be belt-and-suspenders. Not worth changing on its own.


Point #1 is the only thing I'd want an answer on before relying on the private-owned-Model path in production, since PR CI won't exercise it. Everything else is optional polish.

Sign up for freeto 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.

3 participants

@tanvikab4@sahilper@Narrohag