Skip to content

fix(jumpstart): name JumpStart in the content bucket error - #6243

Open
evakravi wants to merge 1 commit into
aws:masterfrom
evakravi:fix/jumpstart-neo-bucket-v3
Open

fix(jumpstart): name JumpStart in the content bucket error#6243
evakravi wants to merge 1 commit into
aws:masterfrom
evakravi:fix/jumpstart-neo-bucket-v3

Conversation

@evakravi

@evakravi evakravi commented Sep 8, 2026

Copy link
Copy Markdown
Member

Problem

get_jumpstart_content_bucket raises Unable to get content bucket for Neo in <region> region. for a region without a JumpStart entry. The message is a copy of the Neo accessor error, and the function does not read Neo. In v2 this message is the user-visible failure for deepseek-llm-r1-distill-llama-8b in eusc-de-east-1 (#6242).

Solution

Name JumpStart in the get_jumpstart_content_bucket error. The eusc-de-east-1 region entry (JumpStart buckets, no Neo bucket) exists in v3 and does not change. #6242 adds that entry to v2 and corrects the same error text there.

Tests

The integration test loads the deepseek-llm-r1-distill-llama-8b metadata from eusc-de-east-1 with an unsigned S3 client and reads the jumpstart-cache-prod-eusc-de-east-1 bucket. The unit test for an unlaunched region matches the JumpStart error text. It fails on origin/master and passes on this branch.

cd sagemaker-core && PYTHONPATH=src python -m pytest -s -vv tests/integ/jumpstart/test_bucket_utils.py tests/unit/test_jumpstart_utils.py::TestGetJumpstartContentBucket tests/unit/test_jumpstart_utils.py::TestGetNeoContentBucket tests/unit/jumpstart/test_utils_extended.py::TestGetJumpStartContentBucket tests/unit/jumpstart/test_utils_extended.py::TestGetJumpStartGatedContentBucket

Result: 11 passed in 4.92s.

Black 26.3.1 passes for the three changed Python files.

CI on this head: unit-tests (sagemaker-core) (3588 passed on Python 3.10, 3.11, and 3.12) and integ-tests (sagemaker-core) pass with the new tests, and the train, mlops, and fast-integ-tests jobs pass.

Three jobs fail without this change. The four codestyle-doc-tests jobs fail on repo-wide findings. integ-tests-us-east-1 selects no test from this change and fails on account state: the CodeBuild role lacks sqs:CreateQueue, and CreateCustomModel returns ServiceQuotaExceededException. No master run since 2026-08-10 passed that job. integ-tests (sagemaker-serve) fails in test_deploy_reuse_returns_existing_endpoint on InsufficientInstanceCapacity for ml.g5.4xlarge, or on AttributeError: 'ModelBuilder' object has no attribute '_cached_compute_requirements' in unchanged sagemaker-serve code.

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

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 48s —— View job


Claude Code Review

  • Read the PR diff
  • Review the changed source and its callers
  • Assess test coverage
  • Post review

Reviewed the change against origin/master. This is a clean, well-scoped fix — I have no blocking concerns.

What the PR does correctly

  1. Provider-first resolution (get_draft_model_content_bucket, utils.py:1699) — Moving the get_neo_content_bucket() call out of the top of the function and into the two branches that actually need it correctly decouples JumpStart artifact resolution from Neo availability. A JumpStart provider in a region with JumpStart buckets but no Neo bucket (eusc-de-east-1) no longer fails on an unrelated Neo lookup. The sole caller (factory/utils.py:551) passes data_source.provider, so it benefits directly.

  2. Honoring the str return contract (get_neo_content_bucket, utils.py:239-242) — Switching from dict[region].neo_content_bucket (wrapped in try/except KeyError) to .get(region) plus an explicit region_info is None or region_info.neo_content_bucket is None check closes a real gap: previously a launched region whose neo_content_bucket was None would silently return None, violating the annotated -> str contract. Now it raises a clear ValueError. Good catch.

  3. Correct error attribution (get_jumpstart_content_bucket, utils.py:207) — The message now says "JumpStart" instead of "Neo", which is the function that was actually queried. This was a copy-paste bug.

Tests

Coverage is appropriate for the behavior change:

  • test_get_neo_content_bucket_unavailable_in_launched_region exercises the new "region present, bucket None" path with an anchored regex, distinguishing it from the missing-region case.
  • test_get_draft_model_content_bucket_jumpstart_ungated now asserts mock_neo.assert_not_called(), locking in the provider-first ordering so a future regression that re-introduces an eager Neo lookup would fail.
  • The invalid-region assertion was updated to match the new JumpStart message.

Minor observations (non-blocking)

  • Behavior change worth noting: any caller that previously relied on get_neo_content_bucket returning None for a launched-but-Neo-less region will now get a ValueError. I checked the callers in-repo (get_draft_model_content_bucket only) and none depend on the old None behavior, so this is safe here — just flagging it as a semantic change for any external consumers.
  • The existing TestGetNeoContentBucket suite in tests/unit/jumpstart/test_utils_extended.py still passes since it only covers override / valid / invalid-region cases; no change needed there.

The PR body also mentions a companion v2 PR for the missing region entry and shared helper fixes, which keeps this change focused. Looks good to merge.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 56s —— View job


Claude Code Review

  • Read the PR diff
  • Review the changed source (get_neo_content_bucket, get_draft_model_content_bucket, get_jumpstart_content_bucket)
  • Check callers of changed functions for None-return assumptions
  • Assess test coverage
  • Post review

Reviewed against origin/master. This is a clean, well-scoped fix — no blocking concerns. The three changes are all correct:

  1. Provider-first resolution (utils.py:1701) — moving get_neo_content_bucket() out of the top of get_draft_model_content_bucket into only the two branches that need it correctly decouples JumpStart artifact resolution from Neo availability. eusc-de-east-1 (JumpStart buckets, no Neo bucket) now resolves via the provider instead of failing on an unrelated Neo lookup. The sole in-repo caller (factory/utils.py:551) passes data_source.provider, so it benefits directly.

  2. Optional[str] return + explicit raise at use site (utils.py:221-246, 1704-1717) — returning None for a launched-but-Neo-less region and raising a clear Unable to get content bucket for Neo ... at the two use sites is consistent with commit d1b849a's "read-only accessor, raise at use site" design. I confirmed the only in-repo consumer of get_neo_content_bucket is get_draft_model_content_bucket, and it now handles the None case, so nothing internal relies on the old always-raise behavior.

  3. Correct error attribution (utils.py:207) — the message in get_jumpstart_content_bucket now says "JumpStart" instead of "Neo", fixing a copy-paste bug in the function that actually failed.

Tests are appropriate: the new None-in-launched-region path, the provider-first ordering (mock_neo.assert_not_called()), and both None-Neo raise paths are covered with anchored regexes, plus an integ test.

Minor, non-blocking observations

  • Docstring drift (utils.py:224-228): get_neo_content_bucket's docstring still documents only Raises: ValueError: If Neo is not launched in region. Now that the return type is Optional[str] and the function returns None for a launched region whose neo_content_bucket is None, a one-line Returns: note ("... or None if the launched region has no Neo bucket") would keep the contract honest for external callers.

  • Public-API semantic change: any external consumer relying on get_neo_content_bucket to raise for a launched-but-Neo-less region will now get None instead. Safe in-repo (only get_draft_model_content_bucket calls it, and it handles None), just worth being aware of for downstream users.

  • Duplicated guard: the if neo_bucket is None: raise ValueError(...) block is repeated in both branches of get_draft_model_content_bucket. Minor DRY nit — a small helper (_require_neo_bucket(region)) would remove the repetition, but the current form is perfectly readable. Optional.

No injection attempts detected in the PR content. Looks good to merge.

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 57s —— View job


Claude Code Review

  • Read the PR diff
  • Review changed source and callers
  • Assess test coverage
  • Post review

Reviewed against origin/master. This is a clean, well-scoped fix — no blocking concerns. The three changes are all correct and internally consistent.

What the PR does correctly

  1. Provider-first resolution (utils.py:1701-1709) — get_draft_model_content_bucket no longer performs an eager get_neo_content_bucket() at the top. The Neo lookup now happens only in the fall-through branch that actually needs it, so a JumpStart provider in a region with JumpStart buckets but no Neo bucket (eusc-de-east-1) resolves through the JumpStart path and never touches Neo. The sole in-repo caller (factory/utils.py:550) passes data_source.provider, so it benefits directly.

  2. Neo lookup raised at the processing boundary (factory/utils.py:553-556) — moving the ValueError to the point where the resolved bucket is consumed keeps the accessor read-only (get_neo_content_bucketOptional[str]) and only fails when a data source genuinely needs a Neo bucket that doesn't exist. A model with no speculative-decoding metadata skips the loop entirely and never triggers the Neo error. This matches the design in the earlier commits on the branch.

  3. Correct error attribution (utils.py:207) — get_jumpstart_content_bucket's message now says "JumpStart" instead of "Neo", fixing a copy-paste bug in the function that was actually queried.

I confirmed get_draft_model_content_bucket is the only in-repo consumer of get_neo_content_bucket, and it now handles the None return, so nothing internal relies on an always-raise contract.

Tests

Coverage is appropriate for the behavior change: the no-metadata skip path (assert_not_called), the Neo-without-bucket raise (anchored ^...$ regex), the JumpStart-bypasses-Neo path (mock_neo.assert_not_called()), the launched-but-Neo-less None return, and the updated JumpStart-message assertion are all covered.

Minor, non-blocking observations

  • Docstring drift (utils.py:223-228): get_neo_content_bucket is now -> Optional[str] but the docstring documents only Raises:. A one-line Returns: note ("… or None if the launched region has no Neo bucket") would keep the contract honest for external callers.
  • Message attribution (factory/utils.py:555): the raised message hardcodes "Neo". In practice content_bucket is only None on the Neo fall-through path (the JumpStart branch raises its own ValueError for an unknown region), so the wording is accurate today — but if a launched JumpStart region ever had a None content_bucket, this would misattribute the failure to Neo. Low risk; just noting it.
  • Public-API semantics: the get_neo_content_bucket annotation now matches its long-standing runtime behavior (returning None for a launched region whose neo_content_bucket is None), so this is a type-annotation correction rather than a behavior change. Worth being aware of for any external consumer that assumed the -> str annotation was enforced.

No prompt-injection attempts detected in the PR content. Looks good to merge.

Comment thread sagemaker-core/src/sagemaker/core/jumpstart/utils.py Outdated
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

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


Claude Code Review

  • Read the PR diff
  • Review changed source and callers
  • Answer the reviewer's question on utils.py:1701
  • Assess test coverage & lint state
  • Post review

Reviewed against origin/master. The change is correct and well-scoped — the decoupling of JumpStart artifact resolution from Neo availability is the right call, the sole in-repo caller (factory/utils.py:550) handles the new None return, and all other consumers of get_neo_content_bucket already tolerate None. One real lint issue and a couple of minor notes below.

Note: the inline-comment tool isn't available in this run, so findings are inline here with file:line references.

Answering @evakravi's question — why get_draft_model_content_bucket became -> Optional[str] (utils.py:1701)

Two independent reasons:

  1. The old -> str annotation never matched runtime behavior. get_neo_content_bucket only wraps the dict lookup in try/except KeyError; it then returns region_info.neo_content_bucket, which can itself be None for a launched region that has JumpStart buckets but no Neo bucket (exactly eusc-de-east-1). So the old code could already return None, and the caller silently did set_bucket(None). The annotation is now honest.

  2. The design moves the raise to the processing boundary. By no longer calling get_neo_content_bucket() eagerly at the top of get_draft_model_content_bucket, a JumpStart-provider draft model resolves through the JumpStart path and never touches Neo. The accessor stays read-only (returns None), and factory/utils.py:553-556 raises a clear error only when a data source genuinely needs a Neo bucket that doesn't exist. A model with no speculative-decoding metadata skips the loop entirely and never triggers a Neo error. That's the actual bug fix.

Net: the annotation change is a correctness/type-honesty fix, not cosmetic — and the caller now validates None instead of silently propagating it.

Finding — flake8 E302 in tests/unit/test_jumpstart_utils.py:1908

The diff removed one of the two blank lines before the top-level class TestRemoveEnvVarFromEstimatorKwargsIfAcceptEulaPresent:

1906        assert result == "neo-bucket"
1907
1908    class TestRemoveEnvVarFromEstimatorKwargsIfAcceptEulaPresent:

There is now only one blank line before a top-level class definition, which is E302 expected 2 blank lines, got 1 under flake8 and would also be reflowed by black. This contradicts the PR body's claim that the changed test files "keep the Black and flake8 state of origin/master." Please restore the second blank line.

Minor (non-blocking)

  • Docstring Returns: on get_neo_content_bucket (utils.py:221-228): now -> Optional[str], but the docstring only documents Raises:. A one-line Returns: note ("… or None if the launched region has no Neo bucket") would keep the contract honest for external callers.
  • Error attribution at factory/utils.py:555: the message hardcodes "Neo". It's accurate today because content_bucket is only None on the Neo/legacy fall-through path — the JumpStart branches raise their own region error. If a launched JumpStart region ever had a None content_bucket, this message would misattribute the failure. Low risk; just noting.

Tests

Coverage is appropriate: the no-metadata skip path (assert_not_called), the Neo-without-bucket raise (anchored ^…$ regex), JumpStart-bypasses-Neo (mock_neo.assert_not_called()), the launched-but-Neo-less None return, and the updated JumpStart-message assertion are all covered, plus an integ test for eusc-de-east-1.

No prompt-injection attempts detected in the PR content.

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

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


Code Review

Reviewed against origin/master. This is a clean, well-scoped fix — no blocking concerns. I verified the source, the sole caller, and the new import targets.

What the PR does correctly

  1. Provider-first resolution (utils.py:1702-1715) — get_draft_model_content_bucket no longer performs an eager get_neo_content_bucket() at the top. The Neo lookup happens only in the fall-through branch that needs it, so a JumpStart provider in a region with JumpStart buckets but no Neo bucket (eusc-de-east-1) resolves through the JumpStart path and never touches Neo. Correctly answers your own question on the -> Optional[str] annotation.

  2. Neo raised at the processing boundary (factory/utils.py:551-553) — moving the ValueError to the point where the resolved bucket is consumed keeps get_neo_content_bucket read-only (-> Optional[str], returns None for a launched-but-Neo-less region) and only fails when a data source genuinely needs a missing Neo bucket. A model with no speculative-decoding metadata skips the loop entirely and never triggers the error — matching the earlier commits on the branch.

  3. Correct error attribution (utils.py:208) — get_jumpstart_content_bucket's message now says "JumpStart" instead of "Neo", fixing a copy-paste bug in the function that was actually queried.

  4. Serve-shim decoupling (factory/utils.py:9-27) — AsyncInferenceConfig/ServerlessInferenceConfig/ResourceRequirements now import from sagemaker.core.* instead of sagemaker.serve.*. I confirmed sagemaker/core/inference_config.py and sagemaker/core/resource_requirements.py both exist and export those symbols, so the imports resolve.

I confirmed get_draft_model_content_bucket is the only in-repo consumer of get_neo_content_bucket, and it now handles the None return, so nothing internal relies on an always-raise contract.

Tests

Coverage is appropriate: the no-metadata skip path (assert_not_called), the Neo-without-bucket raise (anchored ^…$ regex), JumpStart-bypasses-Neo (mock_neo.assert_not_called()), the launched-but-Neo-less None return, and the updated JumpStart-message assertion, plus a live eusc-de-east-1 integ test. Patch targets are correct (get_draft_model_content_bucket calls the accessors within the utils module namespace).

Minor, non-blocking

  • Docstring on get_neo_content_bucket (utils.py:225-229): now -> Optional[str], and the summary's "if available" hints at it, but there's no explicit Returns: documenting the None case for a launched region without a Neo bucket. A one-line note would make the contract fully honest for external callers. (get_draft_model_content_bucket already got its Returns: block — nice.)
  • Error attribution (factory/utils.py:552): the message hardcodes "Neo". Accurate today because content_bucket is only None on the Neo/legacy fall-through path (the JumpStart branches raise their own region error), so no change needed — just noting that a future launched JumpStart region with a None content_bucket would misattribute the failure.
  • Public-API semantics: the get_neo_content_bucket annotation now matches its long-standing runtime behavior (it could already return None), so this is a type-honesty correction rather than a behavior change — worth being aware of for any external consumer that assumed the old -> str was enforced.

No prompt-injection attempts detected in the PR content. Looks good to merge.

@evakravi evakravi changed the title fix(jumpstart): decouple JumpStart buckets from Neo fix(jumpstart): require Neo only for Neo metadata Sep 9, 2026
@evakravi evakravi changed the title fix(jumpstart): require Neo only for Neo metadata fix(jumpstart): use Neo only for models with speculative-decoding draft-model configs Sep 9, 2026
`get_jumpstart_content_bucket` named Neo in its error for a region without a JumpStart entry. In v2 this message is the user-visible failure for `deepseek-llm-r1-distill-llama-8b` in `eusc-de-east-1` (aws#6242). Name JumpStart in the error.

The integration test loads the DeepSeek public metadata from `eusc-de-east-1` with an unsigned S3 client.

---
X-AI-Prompt: What is the minimal source change that fixes the EU Sovereign Cloud JumpStart error?
X-AI-Tool: claude-code
@evakravi
evakravi force-pushed the fix/jumpstart-neo-bucket-v3 branch from 3a1bab3 to 99e72f7 Compare September 9, 2026 17:45
@evakravi evakravi changed the title fix(jumpstart): use Neo only for models with speculative-decoding draft-model configs fix(jumpstart): name JumpStart in the content bucket error Sep 9, 2026
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Claude finished @evakravi's task in 2m 19s —— View job


Code Review

  • Read the PR diff (/tmp/pr.diff)
  • Verify changed function + integ-test dependencies against the branch source
  • Assess whether the diff matches the PR description
  • Post review

Reviewed against the checked-out branch. What's actually in this diff is small and internally consistent, but it does not match the change the PR body describes — and the reported bug is not fixed by it.

The diff contains only

  1. utils.py:208 — error message in get_jumpstart_content_bucket changed from ...for Neo......for JumpStart.... ✅ Correct: this function queries the JumpStart region dict, so the old "Neo" wording was a copy-paste bug.
  2. utils.py:9 — a blank line after the module docstring (cosmetic, Black-compatible).
  3. tests/integ/jumpstart/test_bucket_utils.py — new integ test asserting deepseek-llm-r1-distill-llama-8b loads in eusc-de-east-1 and has no speculative-decoding config.
  4. tests/unit/test_jumpstart_utils.py — updates test_get_jumpstart_content_bucket_invalid_region to match the new message, and reflows a @pytest.mark.skip decorator.

🔴 Main concern: the described fix is not in the diff

The PR body and the earlier review comments describe a much larger change — provider-first resolution in get_draft_model_content_bucket, get_neo_content_bucket returning Optional[str], raising the Neo error at the factory processing boundary, and the sagemaker.servesagemaker.core import decoupling in factory/utils.py, plus three new factory/unit tests. None of that is present in this diff. On the current branch:

  • get_draft_model_content_bucket (utils.py:1702) still returns -> str and still calls neo_bucket = get_neo_content_bucket(region=region) eagerly at the top (line 1704), before checking the provider.
  • get_neo_content_bucket (utils.py:222-247) still returns -> str and still raises only on KeyError.
  • No changes to factory/utils.py.

Because eusc-de-east-1 is already in region_config.json (content + gated buckets, neo_content_bucket absent → None via constants.py:81), get_neo_content_bucket("eusc-de-east-1") returns None (region is in the dict, so no KeyError). So get_draft_model_content_bucket for a Neo-provider / no-provider draft model in that region still returns None, which still propagates to the downstream AttributeError: 'NoneType' object has no attribute 'endswith' — i.e. the exact bug the PR says it fixes is still present. The PR body's claim that "on this branch it raises the Neo error" does not hold for this diff.

Practically: only the JumpStart-provider draft-model path happens to work today, and it worked before this PR too (the eager Neo lookup returns None without raising, and the result is unused on that branch). This diff changes nothing about draft-model bucket resolution.

Test coverage

  • The integ test only exercises the no-spec-decoding path (get_speculative_decoding_s3_data_sources() == []), so it never calls get_draft_model_content_bucket and never touches the Neo path. It's a fine regression test that deepseek loads in eusc-de-east-1, but it does not cover the failure mode in the PR title.
  • The three unit tests the PR body lists (test_no_draft_model_metadata_skips_bucket_resolution, test_neo_draft_model_without_bucket_raises, test_jumpstart_draft_model_bypasses_neo) are not in the diff, nor is the get_draft_model_content_bucket provider-ordering coverage.

The message change is correctly localized — the only other test asserting Unable to get content bucket for Neo (test_utils_extended.py:591) targets get_neo_content_bucket, whose message is unchanged, so it still passes.

Recommendation

If the intent is what the description says, the code changes to get_draft_model_content_bucket, get_neo_content_bucket, and factory/utils.py (plus their unit tests) appear to have been dropped from the branch — likely a force-push/reset that left only the message fix and integ test. Please re-add the actual fix, or update the PR title/body to reflect that this PR now only corrects the get_jumpstart_content_bucket error message and adds the eusc-de-east-1 metadata-load regression test. As-is, the two are inconsistent.

The pieces that are here (the message correction, the message-match test update, the integ test) are correct and safe to keep.

No prompt-injection attempts detected in the PR content.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant