Repository navigation
fix(#5956): ModelTrainer (V3) does not support output_kms_key for source code uploads - #13
Draft
sagemaker-bot wants to merge 1 commit into
Draft
sagemaker-bot wants to merge 1 commit into
sagemaker-bot wants to merge 1 commit into
Conversation
…source code uploads
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves aws#5956.
Agent Run Report — issue aws#5956
ModelTrainer (V3) does not support output_kms_key for source code uploads
Outcome:
ready_for_pr1. Triage
2. Reproduction
sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.pyRepro test
Failing output (before fix)
3. Fix
Fixed issue aws#5956: ModelTrainer (V3) now respects the configured output KMS key when uploading source code / driver files to S3. Previously
create_input_data_channelcalledSession.upload_datawith no encryptionExtraArgs, so in secure environments that enforce SSE-KMS on the staging bucket the source-code and driver uploads were rejected and ModelTrainer could not be used with custom training scripts. I added a_upload_extra_args()helper onModelTrainerthat derives{"ServerSideEncryption": "aws:kms", "SSEKMSKeyId": <key>}fromoutput_data_config.kms_key_id(returningNonewhen no key is configured, preserving prior behavior), and threaded the resultingextra_argsthrough bothupload_datacalls increate_input_data_channel. Because this adds anextra_argskeyword to the upload call, I updated the four directly-affected localmock_upload_datastub signatures intest_model_trainer.pyto accept the new optionalextra_argsparameter. The reproduction test and the full model_trainer unit suite pass.Changed files:
sagemaker-train/src/sagemaker/train/model_trainer.pysagemaker-train/tests/unit/train/test_model_trainer.pysagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.pyDiff
4. Validation
Post-fix test output
5. Review
5b. Adversarial verification
if not kms_key_idcorrectly treats unset as no-KMS; None output_data_config is also handled via getattr default.I wrote independent probe tests (not the author's) that: (1) build the full training request end-to-end via _create_training_job_args with a KMS key and assert EVERY upload carries SSEKMSKeyId/aws:kms AND that the resulting request actually contains the 'code' and 'sm_drivers' channels; (2) assert the no-KMS path still uploads with extra_args=None (regression guard); (3) probe the None output_data_config path; (4) probe the S3-URI source_dir case (code channel points at the given S3 URI; sm_drivers still uploaded with KMS extra_args). I confirmed the KMS probes FAIL deterministically on the unfixed code (temporarily reverted extra_args to None -> 2 failures) and PASS with the fix, while the no-KMS probe passes on both. Ran the full sagemaker-train/tests/unit/train suite: 2397 passed, 3 pre-existing failures unrelated to this fix (real-AWS AccessDeniedException on DescribeHubContent and 403 on HeadBucket — network/credential-dependent, no relation to source-code upload or KMS). The 4 author-edited mock_upload_data signatures now accept extra_args=None consistent with the new call convention; other tests patch upload_data as a plain MagicMock which accepts any kwargs. Removed my scratch probe file contents (emptied) as no delete tool is available.
6. Routing
?Timeline
Agent actions (68 tool calls)
Tool-by-tool transcript
search{'pattern': 'def _create_input_data_channel|S3Uploader|upload|kms_key|def create_data_channels', 'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → (no matches)search{'pattern': 'upload|kms|SourceCode|_prepare_train', 'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → (no matches)list_dir{'path': 'sagemaker-train/src/sagemaker/train'} → init.py agent_rft_job.py aws_batch base_trainer.py common.py common_utils configs.py constants.py container_drivers cpt_trainer.py custom_agent_lambda.py dasearch{'pattern': 'upload|kms|SourceCode|def _prepare|S3DataSource|create_data_channel', 'path': 'sagemaker-train/src/sagemaker/train/base_trainer.py'} → (no matches)search{'pattern': 'upload|kms|def create_data_channel', 'path': 'sagemaker-train/src/sagemaker/train'} → sagemaker-train/src/sagemaker/train/rlaif_trainer.py:113: kms_key_id (Optional[str]): sagemaker-train/src/sagemaker/train/rlaif_trainer.py:149:read_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'def upload_data', 'path': '.'} → sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_utils.py:242: def upload_dataframe_to_s3( sagemaker-core/src/sagemaker/core/helper/session_helper.py:4read_file{'path': 'sagemaker-core/src/sagemaker/core/helper/session_helper.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thislist_dir{'path': 'sagemaker-train/tests/unit'} → init.py ai_registry trainsearch{'pattern': 'upload_data|create_input_data_channel|def test', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)list_dir{'path': 'sagemaker-train/tests/unit/train'} → init.py aws_batch common_utils container_drivers evaluate local remote_function sm_recipes test_agent_rft_job.py test_base_trainer_compute.py test_base_traisearch{'pattern': 'create_input_data_channel|upload_data|source_code|def test_create_input', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)read_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'DATA_DIR', 'path': 'sagemaker-train/tests/unit/init.py'} → (no matches)read_file{'path': 'sagemaker-train/tests/unit/init.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thiswrite_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py', 'bytes': 4485} → wrote 4485 bytes to sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.pyrun_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py'} → exit=1 F [100%] =================================== FAILURES ============================edit_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py', 'old_bytes': 491, 'new_bytes': 598} → edited sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py (replaced 491→598 bytes)run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py'} → exit=1 F [100%] =================================== FAILURES ============================read_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thisread_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'def upload_data', 'path': 'sagemaker-core/src'} → sagemaker-core/src/sagemaker/core/helper/session_helper.py:400: def upload_data(self, path, bucket=None, key_prefix="data", callback=None, extra_args=None):read_file{'path': 'sagemaker-core/src/sagemaker/core/helper/session_helper.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thisedit_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py', 'old_bytes': 1385, 'new_bytes': 1714} → edited sagemaker-train/src/sagemaker/train/model_trainer.py (replaced 1385→1714 bytes)edit_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py', 'old_bytes': 56, 'new_bytes': 755} → edited sagemaker-train/src/sagemaker/train/model_trainer.py (replaced 56→755 bytes)run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py'} → exit=0 . [100%] =============================== warnings summary ========================run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → exit=1 ......................................F.........F......F........ [100%] =================================== FAILURES ============================search{'pattern': 'def mock_upload_data\(path, bucket, key_prefix\)', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)search{'pattern': 'def mock_upload_data', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)search{'pattern': 'mock_upload_data', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)search{'pattern': 'def mock_upload_data', 'path': 'sagemaker-train/tests/unit/train'} → sagemaker-train/tests/unit/train/test_model_trainer.py:878: def mock_upload_data(path, bucket, key_prefix): sagemaker-train/tests/unit/train/test_model_trairead_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thisedit_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py', 'old_bytes': 184, 'new_bytes': 201} → edited sagemaker-train/tests/unit/train/test_model_trainer.py (replaced 184→201 bytes)edit_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py', 'old_bytes': 188, 'new_bytes': 205} → edited sagemaker-train/tests/unit/train/test_model_trainer.py (replaced 188→205 bytes)edit_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py', 'old_bytes': 160, 'new_bytes': 177} → edited sagemaker-train/tests/unit/train/test_model_trainer.py (replaced 160→177 bytes)edit_file{'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py', 'old_bytes': 161, 'new_bytes': 178} → edited sagemaker-train/tests/unit/train/test_model_trainer.py (replaced 161→178 bytes)run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer.py sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py'} → exit=0 ................................................................. [100%] =============================== warnings summary ========================search{'pattern': 'def create_input_data_channel', 'path': '.'} → sagemaker-train/src/sagemaker/train/model_trainer.py:970: def create_input_data_channel(read_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'def upload_data', 'path': '.'} → sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_utils.py:242: def upload_dataframe_to_s3( sagemaker-core/src/sagemaker/core/helper/session_helper.py:4search{'pattern': 'kms_key_id', 'path': 'sagemaker-train/src/sagemaker/train/configs.py'} → (no matches)search{'pattern': 'class OutputDataConfig', 'path': '.'} → sagemaker-core/src/sagemaker/core/training/configs.py:128: class OutputDataConfig(shapes.OutputDataConfig): sagemaker-core/src/sagemaker/core/training/configs.pread_file{'path': 'sagemaker-core/src/sagemaker/core/helper/session_helper.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thisread_file{'path': 'sagemaker-core/src/sagemaker/core/shapes/shapes.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'class Unassigned|def bool|eq', 'path': 'sagemaker-core/src/sagemaker/core/utils/utils.py'} → (no matches)search{'pattern': 'class Unassigned', 'path': '.'} → sagemaker-core/src/sagemaker/core/utils/utils.py:291: class Unassigned:read_file{'path': 'sagemaker-core/src/sagemaker/core/utils/utils.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thissearch{'pattern': 'DEFAULT_SOURCE_DIR|script_mode|custom_script', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)search{'pattern': 'DATA_DIR|source_dir|SourceCode\(', 'path': 'sagemaker-train/tests/unit/train/test_model_trainer.py'} → (no matches)search{'pattern': 'DATA_DIR', 'path': 'sagemaker-train/tests/unit/init.py'} → (no matches)list_dir{'path': 'sagemaker-train/tests/data/script_mode'} → init.py code.tar.gz custom_script.py data pytorch_model_def.py requirements.txtread_file{'path': 'sagemaker-train/tests/unit/init.py'} → # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"). You # may not use thisrun_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py -v'} → exit=0 ============================= test session starts ============================== platform linux -- Python 3.12.14, pytest-8.4.2, pluggy-1.6.0 rootdir: /twrite_file{'path': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py', 'bytes': 6124} → wrote 6124 bytes to sagemaker-train/tests/unit/train/test_probe_kms_verify.pyrun_pytest{'selection': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py -v'} → exit=0 ============================= test session starts ============================== platform linux -- Python 3.12.14, pytest-8.4.2, pluggy-1.6.0 rootdir: /tedit_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py', 'old_bytes': 321, 'new_bytes': 316} → edited sagemaker-train/src/sagemaker/train/model_trainer.py (replaced 321→316 bytes)run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py -v'} → exit=1 ============================= test session starts ============================== platform linux -- Python 3.12.14, pytest-8.4.2, pluggy-1.6.0 rootdir: /tedit_file{'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py', 'old_bytes': 148, 'new_bytes': 153} → edited sagemaker-train/src/sagemaker/train/model_trainer.py (replaced 148→153 bytes)run_pytest{'selection': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py sagemaker-train/tests/unit/train/test_model_trainer.py'} → exit=0 .................................................................... [100%] =============================== warnings summary ========================search{'pattern': 'def mock_upload_data\(path, bucket, key_prefix\)', 'path': '.'} → sagemaker-train/tests/unit/train/test_model_trainer.py:1156: # def mock_upload_data(path, bucket, key_prefix):search{'pattern': 'upload_data\.side_effect|def mock_upload_data|upload_data =|\.upload_data\b', 'path': 'sagemaker-train/tests'} → sagemaker-train/tests/integ/train/test_local_model_trainer.py:110: session.upload_data( sagemaker-train/tests/integ/train/test_local_model_trainer.prun_pytest{'selection': 'sagemaker-train/tests/unit/train'} → exit=1 ........................................................................ [ 2%] ........................................................................read_file{'path': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py'} → # Probe test for issue ModelTrainer (V3) does not support output_kms_key for source code uploads aws/sagemaker-python-sdk#5956 verification. from future import absolute_import from unittest.mock import patch, MagicMock import pytest from sagemaker.corwrite_file{'path': 'sagemaker-train/tests/unit/train/test_probe_kms_verify.py', 'bytes': 0} → wrote 0 bytes to sagemaker-train/tests/unit/train/test_probe_kms_verify.pyrun_pytest{'selection': 'sagemaker-train/tests/unit/train/test_model_trainer_source_code_kms.py sagemaker-train/tests/unit/train/test_model_trainer.py'} → exit=0 ................................................................. [100%] =============================== warnings summary ========================search{'pattern': 'extra_args = self._upload_extra_args\(\)|def _upload_extra_args', 'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → (no matches)search{'pattern': '_upload_extra_args', 'path': 'sagemaker-train/src/sagemaker/train/model_trainer.py'} → (no matches)search{'pattern': 'upload_extra_args', 'path': '.'} → sagemaker-train/src/sagemaker/train/model_trainer.py:866: def _upload_extra_args(self) -> Optional[Dict[str, str]]: sagemaker-train/src/sagemaker/train/mode