fix: Default AsyncPredictor upload prefix to endpoint name - #6337
lucasjia-aws merged 1 commit into
Conversation
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
mohamedzeidan2021
left a comment
There was a problem hiding this comment.
Approving. Reviewed against the v2 tree on its own merits rather than as a parity check. Reproduced the issue's own snippet with a real Predictor: TypeError: 'NoneType' object is not subscriptable at utils.py:131 on master, correct S3 key and Accept after. 1 new test fails on reverted source; 22 passed, plus 62 across predictor/async_inference and 189 across the model tests.
Model.deploy passes a non-None name, so that path is untouched, and .name is read nowhere else in v2. The documented layout in doc/overview.rst is unchanged. The pre-existing integ test test_async_walkthrough passed against real AWS in this PR's CI, which is good no-regression evidence for the name-provided path (though nothing exercises name=None end to end).
Two non-blocking notes:
self.name or self.endpoint_nametreatsname=""as unset;if self.name is Nonewould be exact.tests/unit/test_predictor_async.pysetsENDPOINTandBUCKET_NAMEboth to"mxnet_endpoint", so thestartswith("async-endpoint-inputs/{ENDPOINT}-")assertion can't distinguish "endpoint name was used" from "bucket name was used". It does still fail without the fix, so it earns its keep — it's just weaker than it reads. The v3 test gets this right with distinct values.
One sentence in doc/overview.rst noting the new default sub-prefix would close the loop. Note this overlaps files with #6334.
Issue
Fixes #3210
Fixes #4774
V3 counterpart: #6336
Problem
sagemaker.predictor_async.AsyncPredictor(predictor)defaultsnametoNone, but callingpredict(data=...)orpredict_async(data=...)without aninput_pathfails withTypeError: 'NoneType' object is not subscriptable. This was reported on 2.92.1 (#3210) and again on 2.224.2 (#4774).Root cause
AsyncPredictor._upload_data_to_s3builds the S3 key withname_from_base(self.name, short=True), andname_from_baseslices itsbaseargument without a None guard. The existing unit testtest_async_predict_call_with_dataonly passed because it setpredictor_async.namemanually before callingpredict_async.Fix
_upload_data_to_s3, useself.name or self.endpoint_nameas the base for the S3 key prefix. An explicitly providednameis still used unchanged, andself.nameis not modified.nameargument in theAsyncPredictor.__init__docstring.Testing
test_async_predict_call_with_data_and_no_nameandtest_async_predict_call_with_data_and_name_uses_nameintests/unit/test_predictor_async.py, using a realPredictorwith a mocked session.master-v2and passes with this change;tests/unit/test_predictor_async.pypasses 22/22.flake8passes on the changed files.