Skip to content

fix: Default AsyncPredictor upload prefix to endpoint name - #6336

Open
lucasjia-aws wants to merge 1 commit into
aws:masterfrom
lucasjia-aws:fix/3210-async-predictor-default-name
Open

lucasjia-aws wants to merge 1 commit into
aws:masterfrom
lucasjia-aws:fix/3210-async-predictor-default-name

Conversation

@lucasjia-aws

@lucasjia-aws lucasjia-aws commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Issue

Fixes #3210
Fixes #4774

V2 counterpart: #6337

Problem

AsyncPredictor(predictor) is the documented default construction (name=None), but calling predict(data=...) or predict_async(data=...) without an input_path fails with TypeError: 'NoneType' object is not subscriptable. Users have to pass name= or an explicit input_path as a workaround.

Root cause

AsyncPredictor._upload_data_to_s3 builds the S3 key with name_from_base(self.name, short=True). self.name is stored as-is from the constructor, and name_from_base slices its base argument without a None guard, so the default name=None crashes whenever input data has to be uploaded.

Fix

  • In _upload_data_to_s3, use self.name or self.endpoint_name as the base for the S3 key prefix, so the upload path works with the default constructor. An explicitly provided name is still used unchanged.
  • self.name itself is not modified, so the public attribute keeps the value the user passed.
  • Documented the name argument in the AsyncPredictor.__init__ docstring.

Testing

  • Added unit tests in sagemaker-serve/tests/unit/test_predictor_async.py: upload without name uses the endpoint name prefix, upload with name keeps using it, and predict_async(data=...) succeeds without name.
  • The two no-name regression tests fail on the current master and pass with this change; tests/unit/test_predictor_async.py passes 13/13.
  • black --check and flake8 pass on the changed files.

AsyncPredictor accepts name=None by default, but predict() and
predict_async() called name_from_base(self.name) when uploading input
data without an input_path, which raised
"TypeError: 'NoneType' object is not subscriptable".

Fall back to the wrapped predictor's endpoint name when no name is
given, and document the name argument. An explicitly provided name is
still used as the S3 key prefix.

Fixes aws#3210
Fixes aws#4774
@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

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


Code Review — Default AsyncPredictor upload prefix to endpoint name

  • Read the PR diff and changed files
  • Analyze correctness, tests, and conventions
  • Post review

This is a clean, well-targeted fix. The root-cause analysis is accurate: name_from_base (in sagemaker-core/src/sagemaker/core/common_utils.py:142) does trimmed_base = base[: max_length - len(timestamp) - 1], which raises TypeError: 'NoneType' object is not subscriptable when base is None. The default AsyncPredictor(predictor) construction leaves self.name = None, so any predict/predict_async call that has to upload input data hits this.

Correctness ✅

  • base_name = self.name or self.endpoint_name (predictor_async.py:177) is the right fix. It preserves an explicitly provided name and falls back to the endpoint name otherwise.
  • self.name is left unmodified, so the public attribute keeps the user-supplied value — good, no behavior change for consumers reading it.
  • Applies to both predict and predict_async since both route through _upload_data_to_s3.

Tests ✅

  • Covers the three meaningful cases: no-name → endpoint-name prefix, explicit name → name prefix, and the predict_async(data=...) end-to-end path. The no-name tests are genuine regressions that fail on master.

Minor observations (non-blocking, no change required):

  • self.endpoint_name (predictor_async.py:48) is predictor.endpoint_name. If a predictor were ever constructed without an endpoint name (both name and endpoint_name None), the same TypeError would return. In practice a deployed endpoint always has a name, so this is a theoretical edge only — worth being aware of, not worth guarding here.
  • The docstring addition is accurate and matches the actual fallback behavior.

Conventions: No hardcoded account IDs/roles/regions; consistent with surrounding code. No security or performance concerns — this only affects S3 key prefix construction.

No injected instructions were detected in the PR content.

LGTM. 👍

This branch was successfully deployed

1 active deployment
auto-approve — 1dda4968 Deployed Sep 25, 2026 by lucasjia-aws via wait-for-approval #1817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant