feat(train): validate raw base model name exists in SageMaker Hub

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.

[jam-jee]

Problem

When a user passes a raw base model name (a plain string, not a model-package ARN or ModelPackage) to a V3 trainer, model resolution accepts the name without confirming the model actually exists in the SageMaker Hub. A misspelled or unsupported name is only caught later, during recipe resolution, where the failure is more opaque and harder to map back to the model argument.

Why it matters

Fine-tuning jobs are long-lived and expensive to set up. A fast, clear "this model isn't in the Hub" at construction time saves users from a confusing downstream error and points them at the right next step (list_supported_models()), rather than leaving them to decode a recipe-lookup failure.

Fix (symptom → root cause → change)

• Symptom: a bad raw base model name is accepted at resolve time and fails later with an unclear message.
• Root cause: _resolve_model_and_name normalizes the name and validates region, but never checks Hub availability for the raw-string case.
• Change: after the existing region check in the raw-model-name branch of _resolve_model_and_name, call a new _validate_model_in_hub(...) that issues a single DescribeHubContent against the active hub (get_sagemaker_hub_name()) via the existing _get_hub_content_metadata helper.
  • Only a definitive not-found raises a clear ValueError; transient or permission errors (Hub outage, missing DescribeHubContent permission, throttling) are logged and skipped, so a Hub hiccup never blocks an otherwise-valid job (fail-open on ambiguity, fail-closed only on a real miss).
  • A small _is_hub_content_not_found(exc) classifier distinguishes the two cases (botocore ResourceNotFound code, sagemaker-core exception class name, or message text).

Because the check lives in the shared _resolve_model_and_name path used by the trainer interfaces (SFT/DPO/RLVR/RLAIF/CPT/MTRL), it also covers the base_model_name supplied alongside an S3 checkpoint, which routes through the same resolver.

Model-package ARNs and ModelPackage objects are unchanged: the Hub check only applies to raw base model names.

Tests

• 8 new unit tests in tests/unit/train/common_utils/test_finetune_utils.py:
  • _is_hub_content_not_found classification (error code, message text, transient/permission errors that must NOT be treated as not-found).
  • _validate_model_in_hub: no-session skip, found passes, not-found raises, transient error does not block.
  • _resolve_model_and_name integration: raises for a missing model, resolves normally for a present model.
• New tests/unit/train/conftest.py with an autouse fixture that no-ops the Hub check for trainer construction tests (they build trainers with placeholder model names against mock sessions and must not reach the network). The fixture explicitly excludes the common_utils/ directory so the dedicated tests above exercise the real function.

Manual verification

N/A — unit coverage is sufficient; the new behavior is a single API call guarded by exception classification, fully exercised by mocked unit tests. Full tests/unit/train suite: 2403 passed, 19 skipped (the one remaining failure, TestWaitForMlflowAppReady::test_polls_until_ready, is pre-existing on master and unrelated to this change).

Screenshots

N/A — no user-visible UI change.

[github-actions]

Claude finished @jam-jee's task in 1m 49s —— View job

---

PR Review: validate raw base model name exists in SageMaker Hub

• Read the PR diff and changed files
• Review source change (finetune_utils.py)
• Trace callers of _resolve_model_and_name / _get_hub_content_metadata
• Review tests (unit + integ + conftest)

Overall this is a clean, well-scoped change. The fail-open-on-ambiguity / fail-closed-on-definitive-miss design is the right call, the exception classifier is reasonable, and test coverage (unit + a targeted integ test for the real error shape) is solid. A few things worth surfacing — none are blockers.

1. Behavioral change: trainer construction now makes a network call (worth documenting)

_resolve_model_and_name runs in trainer __init__ (e.g. rlaif_trainer.py:162, multi_turn_rl_trainer.py:222). Previously the raw-name branch was purely local (normalize + region check); it now issues a DescribeHubContent on every trainer construction with a raw model name. The new tests/unit/train/conftest.py autouse fixture — added precisely because trainer tests "must not reach the network" — confirms this shift.

Two implications:

• Constructing a trainer now requires the sagemaker:DescribeHubContent IAM permission and network reachability. The fail-open handling means a missing permission or outage logs a warning rather than hard-failing, so this won't break valid jobs — but it's a new expectation. Consider a one-line note in the trainer docstring / changelog.
• This is the same lookup recipe resolution performs later (_get_fine_tuning_options_and_model_arn, finetune_utils.py:723), so it's a duplicateDescribeHubContent. Minor, but if construction latency matters it's a candidate to cache/share the result. Not required.

2. Nova / open-weights parity — looks safe, please confirm

The check applies to all raw names, including Nova, whereas _validate_model_region_availability special-cases Nova. This is fine iff Nova base models resolve as HubContentType="Model" content under their normalized names — which is exactly what recipe resolution already assumes (finetune_utils.py:723 uses the identical call). So a Nova name that fine-tunes successfully today would also pass this pre-check. Worth a quick sanity check that no supported raw name (Nova or otherwise) resolves through a different path that skips hub-content lookup, since such a name would now get a spurious "not available in SageMaker Hub" error.

3. Message-text classification is a heuristic — acceptable because it fails open

_is_hub_content_not_found falls back to substring matching ("not found", "does not exist", …). This can theoretically both false-positive (a transient error whose message happens to contain "not found" → wrongly blocks) and false-negative (a real not-found the SDK wraps with different text → silently skips). The false-negative direction is safe (feature no-ops). The false-positive direction is the riskier one; it's low-probability, and the integ test (test_bogus_model_name_raises) validates the real service error shape, which is the right guard. No change needed — just noting the classifier's real contract is "botocore code / class name are authoritative; message text is best-effort."

4. Minor

• finetune_utils.py:57 — the sagemaker_session is None guard means that when a session is genuinely absent the check is skipped entirely. That's intentional and matches the region-check pattern, but it does mean local/offline construction silently gets no Hub validation. Fine as designed.
• Import ordering in the test file (_validate_model_in_hub before _is_hub_content_not_found before _validate_s3_path_exists) is cosmetic; linters will flag if it matters.

No correctness bugs found. Nice work on the fail-open design and the integ test covering what the mocks can't.

Note: I couldn't post these as inline review comments — the inline-comment tool isn't available in this run, so findings are consolidated here with file:line references.