Conversation
|
Claude finished @jam-jee's task in 1m 20s —— View job Code review
Overall this is a solid, well-scoped set of local-mode fixes with a test per behavior change. One correctness issue is worth addressing before merge; the rest is minor. 🔴
|
cfb8c26 to
3fa7ce7
Compare
|
Claude finished @jam-jee's task in 1m 26s —— View job Code review
This is a well-scoped, cleanly-tested set of local-mode fixes — each behavior change has a unit test, the isolation fixes ( 🔴
|
Bring local mode behavior in line with the hosted SageMaker service: - Runtime invoke_endpoint now returns ResponseMetadata and InvokedProductionVariant, matching the boto3 runtime response. - LocalSagemakerClient gains a describe_user_profile passthrough so Studio role resolution works instead of raising AttributeError. - MultiRecordStrategy buffers bytes or str based on the record type, so local batch transform handles binary inputs. - Processing jobs no longer require an IAM role in local mode, where the role is never used. - Local pipeline execution state is isolated per instance and per session instead of living on class-level or client-level state. - Local pipeline execution ids are 12-char uppercase alphanumerics, matching the service format and downstream name limits. This ports aws#5283 (merged to master-v2, by aviruthen) to the V3 codebase; see aws#5269. Response headers are looked up case-insensitively (containers send Content-Type), and local/image.py imports DIR_PARAM_NAME and SAGEMAKER_OUTPUT_LOCATION from sagemaker.core.constants instead of referencing the never-imported sagemaker.serve.model_builder, which made every local endpoint fail with AttributeError unless sagemaker.serve happened to be imported first; three unit tests skipped for that reason are re-enabled. The role fix also skips expand_role() when building the create request, since expand_role(None) raised TypeError once the constructor let a missing role through. Local-mode integration tests (Docker) cover the role-less processing job and the invoke_endpoint response contract. Fixes aws#3348 Fixes aws#4417 Fixes aws#4996 Fixes aws#5562 Fixes aws#5572 Fixes aws#5604 --- X-AI-Prompt: Fix S-effort PySDK V3 bugs, local-mode theme X-AI-Tool: Kiro
3fa7ce7 to
fed4c01
Compare
|
Addressing the review on Case-sensitive While running that integ test I hit a second local-mode bug on
Integration tests run locally against the CI account + Docker: |
|
Claude finished @jam-jee's task in 1m 3s —— View job Code review
This is a clean, well-scoped set of local-mode fixes with a unit test per behavior change and two end-to-end integ tests. The ✅ The case-sensitivity blocker is resolved
response_headers = dict(r.headers)
...
lowered = {str(k).lower(): v for k, v in response_headers.items()}
request_id = lowered.get("x-amzn-requestid", "local-request-id")
...
"ContentType": lowered.get("content-type", Accept),Lowercasing every header key before lookup correctly handles the real-world ✅
|
|
Hi. I am a user and reporter of some issues related to misalignment. Can you also consider fixing #4757? Thanks for your work, really appreciated. |
Summary
Local mode diverged from the hosted SageMaker service in several small ways that broke code written against the real service. This PR brings the V3 local mode back in line:
invoke_endpointresponse shape --LocalSagemakerRuntimeClient.invoke_endpointnow returnsResponseMetadataandInvokedProductionVariantalongsideBody/ContentType, matching the boto3 runtime response.ContentTypenow comes from the container's response header rather than echoing the requestAccept.describe_user_profile--LocalSagemakerClientgains a passthrough so Studio role resolution works in local mode instead of raisingAttributeError.MultiRecordStrategypicks abytesorstrbuffer from the first record instead of always assuming text.local/local_gpuinstance types, where the role is never used._LocalPipeline._executionsmoves from a class attribute to the instance, andLocalPipelineSessionowns its pipeline registry (_local_pipelines) instead of injecting_pipelinesonto the sharedsagemaker_client.master-v2) to V3; see Execution ID has different length and characters in SageMaker Local Mode vs remote execution #5269.Issues fixed
Fixes #3348
Fixes #4417
Fixes #4996
Fixes #5562
Fixes #5572
Fixes #5604
Related: #5269 (V2 fix landed in #5283; this is the V3 port).
Release note
LocalSagemakerRuntimeClient.invoke_endpointnow returnsContentTypefrom the container response header instead of echoing the request'sAcceptvalue, and includesResponseMetadata/InvokedProductionVariant. The change is additive for callers that only readBody.Testing
New unit tests, each verified to fail without the source change and pass with it:
sagemaker-core/tests/unit/local/test_data.py:test_multi_record_strategy_pad_bytes,test_multi_record_strategy_pad_empty,test_multi_record_strategy_pad_strsagemaker-core/tests/unit/local/test_local_session.py:test_invoke_endpoint_response_shape,test_invoke_endpoint_default_variant,test_describe_user_profile_passthroughsagemaker-core/tests/unit/test_processing.py:test_role_not_required_for_local_instance,test_role_not_required_for_local_gpu_instance,test_role_still_required_for_managed_instancesagemaker-mlops/tests/unit/local/test_local_pipeline_session.py:test_local_pipeline_session_registry_isolated_per_sessionsagemaker-mlops/tests/unit/local/test_pipeline_entities.py:test_executions_isolated_per_instance,test_start_uses_service_like_execution_idExisting modules for the touched files pass: 165 (sagemaker-core) + 48 (sagemaker-mlops).
black -l 100andflake8clean on changed files.Integration tests
New
sagemaker-core/tests/integ/local_mode/test_local_session_fixes.py(markerslocal_mode,serial; needs Docker, which thesagemaker-core-integ-testsCodeBuild project has):test_local_processor_runs_without_role_5562: a real local processing job (sklearn image,instance_type="local",role=None) reachesCompleted. Writing this test found that the constructor fix alone was not enough:_get_process_argsstill calledexpand_role(None)and raisedTypeError, so this revision also skips role expansion when there is no role (unit testtest_request_args_skip_role_expansion_when_no_role).test_local_endpoint_invoke_response_shape_and_binary_3348_4996: builds a tiny echo serving container, deploys it as a local endpoint, and asserts theinvoke_endpointresponse carriesBody,ContentTypefrom the container's header,InvokedProductionVariantandResponseMetadata, and that a binary payload round-trips.Judged not worth an integ test: #4417 (
describe_user_profilepassthrough; an integ test would only prove a boto client can call the service), #5604/#5572/#5269 (local pipeline registry and execution ids run in-process with no Docker or AWS involvement; the unit tests are the end-to-end proof).Both tests run green locally through Docker against the CI account. The endpoint test also exposed a
masterbug:local/image.pyreferencedsagemaker.serve.model_builder.DIR_PARAM_NAMEwithout importingsagemaker.serve, so every local endpoint (and local training volume setup) raisedAttributeErrorunlesssagemaker.servewas already imported. The constants are now imported fromsagemaker.core.constants, and the threetest_image.pyunit tests skipped for that reason are re-enabled.X-AI-Prompt: Fix S-effort PySDK V3 bugs, local-mode theme
X-AI-Tool: Kiro