diff --git a/src/sagemaker/estimator.py b/src/sagemaker/estimator.py index 2805bb73ae..180255e264 100644 --- a/src/sagemaker/estimator.py +++ b/src/sagemaker/estimator.py @@ -790,10 +790,9 @@ def __init__( sagemaker_session=sagemaker_session, ) # If customer passes True from either direct_input or sagemaker_config, we will - # create a default hook config as an empty dict which will later be populated - # with default s3_output_path from _prepare_debugger_for_training function + # create a default hook config populated with the default s3_output_path. if self.debugger_hook_config is True: - self.debugger_hook_config = {} + self.debugger_hook_config = DebuggerHookConfig() self.tensorboard_output_config = tensorboard_output_config @@ -1197,8 +1196,11 @@ def _validate_and_set_debugger_configs(self): ) if region_supports_debugger: - if self.debugger_hook_config in [None, {}]: - self.debugger_hook_config = DebuggerHookConfig(s3_output_path=self.output_path) + # Debugger is opt-in: do NOT auto-attach a DebuggerHookConfig when the customer + # did not request one (unset -> None). The hook is still created upstream in + # _prepare_debugger_for_training when debugger_rules are set, or when the customer + # explicitly passes debugger_hook_config=True / a DebuggerHookConfig instance. + pass else: if self.debugger_hook_config is not False and self.debugger_hook_config: # when user set debugger config in a unsupported region @@ -1245,11 +1247,15 @@ def _prepare_profiler_for_training(self): if self.profiler_rules: raise RuntimeError("ProfilerRule cannot be set when disable_profiler is True.") elif _region_supports_profiler(self.sagemaker_session.boto_region_name): - if self.profiler_config is None: + # Profiler is opt-in: do NOT auto-attach an active ProfilerConfig by default. + # If profiler_config is left unset it is emitted as disabled below. + # Passing a ProfilerRule is itself an opt-in, so give it a config to attach to; + # otherwise the request would carry a profiler rule with profiling disabled. + if self.profiler_rules and self.profiler_config is None: self.profiler_config = ProfilerConfig(s3_output_path=self.output_path) if self.rules is None or (self.rules and not self.profiler_rules): self.profiler_rules = [] - if self.profiler_config.profile_params: + if self.profiler_config and self.profiler_config.profile_params: self.profiler_rules.append( get_default_profiler_processing_job( instance_type=self.profiler_config.profile_params.instanceType, diff --git a/tests/integ/sagemaker/workflow/test_training_steps.py b/tests/integ/sagemaker/workflow/test_training_steps.py index 4b442c6d93..8ca842ec94 100644 --- a/tests/integ/sagemaker/workflow/test_training_steps.py +++ b/tests/integ/sagemaker/workflow/test_training_steps.py @@ -24,6 +24,7 @@ from sagemaker import TrainingInput, get_execution_role, utils, image_uris from sagemaker.debugger import ( DebuggerHookConfig, + ProfilerConfig, Rule, rule_configs, ) @@ -100,6 +101,8 @@ def test_training_job_with_debugger_and_profiler( sagemaker_session=sagemaker_session, rules=rules, debugger_hook_config=debugger_hook_config, + # Profiling is opt-in, so request it explicitly. + profiler_config=ProfilerConfig(system_monitor_interval_millis=500), # TODO: remove base_job_name once we merge # https://github.com/aws/sagemaker-python-sdk/pull/3158/files base_job_name="TestJob", diff --git a/tests/integ/test_debugger.py b/tests/integ/test_debugger.py index 372a0c6032..9fb8d5733e 100644 --- a/tests/integ/test_debugger.py +++ b/tests/integ/test_debugger.py @@ -435,6 +435,7 @@ def test_debug_hook_disabled_with_checkpointing( distribution={"smdistributed": {"dataparallel": {"enabled": True}}}, checkpoint_local_path="/opt/ml/checkpoints", checkpoint_s3_uri=os.path.join(s3_output_path, "checkpoints"), + debugger_hook_config=True, ) pt._prepare_for_training() # Debug Hook should be disabled @@ -455,6 +456,7 @@ def test_debug_hook_disabled_with_checkpointing( distribution={"smdistributed": {"modelparallel": {"enabled": True}}}, checkpoint_local_path="/opt/ml/checkpoints", checkpoint_s3_uri=os.path.join(s3_output_path, "checkpoints"), + debugger_hook_config=True, ) tf._prepare_for_training() # Debug Hook should be disabled @@ -472,6 +474,7 @@ def test_debug_hook_disabled_with_checkpointing( instance_type="ml.p3.16xlarge", sagemaker_session=sagemaker_session, # Training using SMDataParallel Distributed Training Framework + debugger_hook_config=True, ) xg._prepare_for_training() # Debug Hook should be enabled diff --git a/tests/integ/test_profiler.py b/tests/integ/test_profiler.py index 7d3fdb2d7b..a8e3f7aad5 100644 --- a/tests/integ/test_profiler.py +++ b/tests/integ/test_profiler.py @@ -74,6 +74,8 @@ def test_mxnet_with_default_profiler_config_and_profiler_rule( instance_count=1, instance_type=cpu_instance_type, sagemaker_session=sagemaker_session, + # Profiling is opt-in, so request it explicitly. + profiler_config=ProfilerConfig(system_monitor_interval_millis=500), ) train_input = mx.sagemaker_session.upload_data( @@ -422,6 +424,9 @@ def test_mxnet_profiling_with_disable_debugger_hook( instance_type=cpu_instance_type, sagemaker_session=sagemaker_session, debugger_hook_config=False, + # Profiling is opt-in, so request it explicitly. The point of this test is that + # disabling the debugger hook must not disable profiling. + profiler_config=ProfilerConfig(system_monitor_interval_millis=500), ) train_input = mx.sagemaker_session.upload_data( diff --git a/tests/unit/sagemaker/huggingface/test_estimator.py b/tests/unit/sagemaker/huggingface/test_estimator.py index dc6044b154..02b277a305 100644 --- a/tests/unit/sagemaker/huggingface/test_estimator.py +++ b/tests/unit/sagemaker/huggingface/test_estimator.py @@ -148,13 +148,8 @@ def _create_train_job(version, base_framework_version): "retry_strategy": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/sagemaker/tensorflow/test_estimator.py b/tests/unit/sagemaker/tensorflow/test_estimator.py index f07bbffb05..8b563f954e 100644 --- a/tests/unit/sagemaker/tensorflow/test_estimator.py +++ b/tests/unit/sagemaker/tensorflow/test_estimator.py @@ -146,17 +146,10 @@ def _create_train_job(tf_version, horovod=False, ps=False, py_version="py2", smd "enable_network_isolation": False, "experiment_config": None, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } - if not ps: - conf["debugger_hook_config"] = { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - } - return conf diff --git a/tests/unit/sagemaker/training_compiler/test_huggingface_pytorch_compiler.py b/tests/unit/sagemaker/training_compiler/test_huggingface_pytorch_compiler.py index 18d8f5f2d3..eb8df3df8b 100644 --- a/tests/unit/sagemaker/training_compiler/test_huggingface_pytorch_compiler.py +++ b/tests/unit/sagemaker/training_compiler/test_huggingface_pytorch_compiler.py @@ -150,13 +150,8 @@ def _create_train_job( "environment": None, "retry_strategy": None, "experiment_config": EXPERIMENT_CONFIG, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/sagemaker/training_compiler/test_huggingface_tensorflow_compiler.py b/tests/unit/sagemaker/training_compiler/test_huggingface_tensorflow_compiler.py index e99eccb2c8..278a9e25ff 100644 --- a/tests/unit/sagemaker/training_compiler/test_huggingface_tensorflow_compiler.py +++ b/tests/unit/sagemaker/training_compiler/test_huggingface_tensorflow_compiler.py @@ -147,13 +147,8 @@ def _create_train_job( "retry_strategy": None, "experiment_config": EXPERIMENT_CONFIG, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/sagemaker/training_compiler/test_pytorch_compiler.py b/tests/unit/sagemaker/training_compiler/test_pytorch_compiler.py index 903a56d403..43a76d72e6 100644 --- a/tests/unit/sagemaker/training_compiler/test_pytorch_compiler.py +++ b/tests/unit/sagemaker/training_compiler/test_pytorch_compiler.py @@ -146,13 +146,8 @@ def _create_train_job( "retry_strategy": None, "experiment_config": EXPERIMENT_CONFIG, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/sagemaker/training_compiler/test_tensorflow_compiler.py b/tests/unit/sagemaker/training_compiler/test_tensorflow_compiler.py index 5b33ec4445..b8e11299dc 100644 --- a/tests/unit/sagemaker/training_compiler/test_tensorflow_compiler.py +++ b/tests/unit/sagemaker/training_compiler/test_tensorflow_compiler.py @@ -152,13 +152,8 @@ def _create_train_job(framework_version, instance_type, training_compiler_config "retry_strategy": None, "experiment_config": EXPERIMENT_CONFIG, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/sagemaker/workflow/test_step_collections.py b/tests/unit/sagemaker/workflow/test_step_collections.py index 4cca2af50c..68b324426f 100644 --- a/tests/unit/sagemaker/workflow/test_step_collections.py +++ b/tests/unit/sagemaker/workflow/test_step_collections.py @@ -767,10 +767,6 @@ def test_register_model_with_model_repack_with_estimator( "TrainingImage": MODEL_REPACKING_IMAGE_URI, "TrainingInputMode": "File", }, - "DebugHookConfig": { - "CollectionConfigurations": [], - "S3OutputPath": f"s3://{BUCKET}/", - }, "ProfilerConfig": {"DisableProfiler": True}, "HyperParameters": { "inference_script": '"inference.py"', @@ -896,10 +892,6 @@ def test_register_model_with_model_repack_with_model(model, model_metrics, drift "TrainingImage": MODEL_REPACKING_IMAGE_URI, "TrainingInputMode": "File", }, - "DebugHookConfig": { - "CollectionConfigurations": [], - "S3OutputPath": f"s3://{BUCKET}/", - }, "ProfilerConfig": {"DisableProfiler": True}, "HyperParameters": { "inference_script": '"inference.py"', @@ -1027,10 +1019,6 @@ def test_register_model_with_model_repack_with_pipeline_model( "TrainingImage": MODEL_REPACKING_IMAGE_URI, "TrainingInputMode": "File", }, - "DebugHookConfig": { - "CollectionConfigurations": [], - "S3OutputPath": f"s3://{BUCKET}/", - }, "ProfilerConfig": {"DisableProfiler": True}, "HyperParameters": { "dependencies": "null", @@ -1254,10 +1242,6 @@ def test_estimator_transformer_with_model_repack_with_estimator(estimator, sourc "sagemaker_region": '"us-west-2"', }, "VpcConfig": {"Subnets": ["abc", "def"], "SecurityGroupIds": ["123", "456"]}, - "DebugHookConfig": { - "S3OutputPath": "s3://my-bucket/", - "CollectionConfigurations": [], - }, } elif request_dict["Type"] == "Model": assert request_dict["Name"] == "EstimatorTransformerStepCreateModelStep" diff --git a/tests/unit/sagemaker/workflow/test_steps.py b/tests/unit/sagemaker/workflow/test_steps.py index 84906ce620..60e53506c5 100644 --- a/tests/unit/sagemaker/workflow/test_steps.py +++ b/tests/unit/sagemaker/workflow/test_steps.py @@ -337,10 +337,6 @@ def test_training_step_base_estimator(sagemaker_session): }, "RoleArn": ROLE, "StoppingCondition": {"MaxRuntimeInSeconds": 86400}, - "DebugHookConfig": { - "S3OutputPath": {"Std:Join": {"On": "/", "Values": ["s3:/", "a", "b"]}}, - "CollectionConfigurations": [], - }, "ProfilerConfig": { "DisableProfiler": False, "ProfilingIntervalInMilliseconds": 500, @@ -453,7 +449,7 @@ def test_training_step_tensorflow(sagemaker_session): "sagemaker_instance_type": {"Get": "Parameters.InstanceType"}, "sagemaker_distributed_dataparallel_custom_mpi_options": '""', }, - "ProfilerConfig": {"DisableProfiler": False, "S3OutputPath": "s3://my-bucket/"}, + "ProfilerConfig": {"DisableProfiler": True}, }, "CacheConfig": {"Enabled": True, "ExpireAfter": "PT1H"}, } diff --git a/tests/unit/sagemaker/workflow/test_utils.py b/tests/unit/sagemaker/workflow/test_utils.py index b18ed71f9b..4378f3d1a8 100644 --- a/tests/unit/sagemaker/workflow/test_utils.py +++ b/tests/unit/sagemaker/workflow/test_utils.py @@ -94,7 +94,6 @@ def test_repack_model_step(estimator): "DependsOn": ["TestStep"], "Arguments": { "AlgorithmSpecification": {"TrainingInputMode": "File"}, - "DebugHookConfig": {"CollectionConfigurations": [], "S3OutputPath": "s3://my-bucket/"}, "InputDataConfig": [ { "ChannelName": "training", @@ -176,7 +175,6 @@ def test_repack_model_step_with_source_dir(estimator, source_dir): "Type": "Training", "Arguments": { "AlgorithmSpecification": {"TrainingInputMode": "File"}, - "DebugHookConfig": {"CollectionConfigurations": [], "S3OutputPath": "s3://my-bucket/"}, "InputDataConfig": [ { "ChannelName": "training", diff --git a/tests/unit/test_chainer.py b/tests/unit/test_chainer.py index fadb47d47d..37ca42b495 100644 --- a/tests/unit/test_chainer.py +++ b/tests/unit/test_chainer.py @@ -154,13 +154,8 @@ def _create_train_job(version, py_version): "enable_network_isolation": False, "environment": None, "experiment_config": None, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/test_estimator.py b/tests/unit/test_estimator.py index 7ea64ab2ea..8a674722ef 100644 --- a/tests/unit/test_estimator.py +++ b/tests/unit/test_estimator.py @@ -693,7 +693,7 @@ def test_estimator_with_debugger_hook_config_provided_as_bool_from_direct_input( base_job_name="base_job_name", debugger_hook_config=True, ) - assert estimator.debugger_hook_config == {} + assert isinstance(estimator.debugger_hook_config, DebuggerHookConfig) def test_estimator_with_debugger_hook_config_provided_as_dict_from_direct_input( @@ -1067,8 +1067,7 @@ def test_framework_with_debugger_and_built_in_rule(sagemaker_session): ], } assert args["profiler_config"] == { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, } @@ -1226,14 +1225,10 @@ def test_framework_without_debugger_and_profiler(time, sagemaker_session): f.fit("s3://mydata") sagemaker_session.train.assert_called_once() _, args = sagemaker_session.train.call_args - assert args["debugger_hook_config"] == { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - } + assert "debugger_hook_config" not in args or args["debugger_hook_config"] is None assert "debugger_rule_configs" not in args assert args["profiler_config"] == { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, } @@ -1295,6 +1290,8 @@ def test_framework_with_debugger_and_profiler_rules(sagemaker_session): } ], } + # A ProfilerRule is an opt-in, so a ProfilerConfig must be attached. CreateTrainingJob + # rejects "ProfilerRuleConfigurations" that are provided without "ProfilerConfig". assert args["profiler_config"] == { "DisableProfiler": False, "S3OutputPath": "s3://{}/".format(BUCKET_NAME), @@ -1331,6 +1328,8 @@ def test_framework_with_only_profiler_rule_specified(sagemaker_session): f.fit("s3://mydata") sagemaker_session.train.assert_called_once() _, args = sagemaker_session.train.call_args + # A ProfilerRule is an opt-in, so a ProfilerConfig must be attached. CreateTrainingJob + # rejects "ProfilerRuleConfigurations" that are provided without "ProfilerConfig". assert args["profiler_config"] == { "DisableProfiler": False, "S3OutputPath": "s3://{}/".format(BUCKET_NAME), @@ -2601,7 +2600,7 @@ def test_fit_verify_job_name(strftime, sagemaker_session): @pytest.mark.parametrize( "debugger_hook_config_direct_input, sagemaker_config, expected_debugger_hook_config_output", [ - (None, None, S3_OUTPUT_PATH_FROM_SESSION_S3_DEFAULT_CONFIG), + (None, None, None), (True, None, S3_OUTPUT_PATH_FROM_SESSION_S3_DEFAULT_CONFIG), (False, None, False), (HOOK_CONFIG, None, HOOK_CONFIG.s3_output_path), @@ -2665,6 +2664,8 @@ def test_prepare_for_training_for_debugger_hook_config_value_combinations( if expected_debugger_hook_config_output is False: assert fw.debugger_hook_config == expected_debugger_hook_config_output + elif expected_debugger_hook_config_output is None: + assert fw.debugger_hook_config is None else: assert fw.debugger_hook_config.s3_output_path == expected_debugger_hook_config_output @@ -3522,7 +3523,7 @@ def test_unsupported_type_in_dict(): "input_config": None, "input_mode": "File", "output_config": {"S3OutputPath": OUTPUT_PATH}, - "profiler_config": {"DisableProfiler": False, "S3OutputPath": OUTPUT_PATH}, + "profiler_config": {"DisableProfiler": True}, "resource_config": { "InstanceCount": INSTANCE_COUNT, "InstanceType": INSTANCE_TYPE, @@ -3802,7 +3803,7 @@ def test_generic_to_fit_no_input(time, sagemaker_session): args.pop("job_name") args.pop("role") - args.pop("debugger_hook_config") + args.pop("debugger_hook_config", None) assert args == NO_INPUT_TRAIN_CALL @@ -3827,7 +3828,7 @@ def test_generic_to_fit_no_hps(time, sagemaker_session): args.pop("job_name") args.pop("role") - args.pop("debugger_hook_config") + args.pop("debugger_hook_config", None) assert args == BASE_TRAIN_CALL @@ -3854,7 +3855,7 @@ def test_generic_to_fit_with_hps(time, sagemaker_session): args.pop("job_name") args.pop("role") - args.pop("debugger_hook_config") + args.pop("debugger_hook_config", None) assert args == HP_TRAIN_CALL @@ -3887,7 +3888,7 @@ def test_generic_to_fit_with_experiment_config(time, sagemaker_session): args.pop("job_name") args.pop("role") - args.pop("debugger_hook_config") + args.pop("debugger_hook_config", None) assert args == EXP_TRAIN_CALL @@ -4041,7 +4042,7 @@ def test_generic_to_deploy(time, sagemaker_session): args.pop("job_name") args.pop("role") - args.pop("debugger_hook_config") + args.pop("debugger_hook_config", None) assert args == HP_TRAIN_CALL diff --git a/tests/unit/test_mxnet.py b/tests/unit/test_mxnet.py index 120f90f9c6..1228966dc0 100644 --- a/tests/unit/test_mxnet.py +++ b/tests/unit/test_mxnet.py @@ -170,13 +170,8 @@ def _get_train_args(job_name): "retry_strategy": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/test_pytorch.py b/tests/unit/test_pytorch.py index e6743f3ee2..8d02b91f85 100644 --- a/tests/unit/test_pytorch.py +++ b/tests/unit/test_pytorch.py @@ -187,13 +187,8 @@ def _create_train_job(version, py_version): "retry_strategy": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/test_rl.py b/tests/unit/test_rl.py index aa56a5ef14..eaced074bc 100644 --- a/tests/unit/test_rl.py +++ b/tests/unit/test_rl.py @@ -158,13 +158,8 @@ def _create_train_job(toolkit, toolkit_version, framework): "environment": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, "retry_strategy": None, } diff --git a/tests/unit/test_sklearn.py b/tests/unit/test_sklearn.py index f94b811f14..4345166977 100644 --- a/tests/unit/test_sklearn.py +++ b/tests/unit/test_sklearn.py @@ -145,13 +145,8 @@ def _create_train_job(version): "environment": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, } diff --git a/tests/unit/test_xgboost.py b/tests/unit/test_xgboost.py index 189df1efc6..af5638cd6d 100644 --- a/tests/unit/test_xgboost.py +++ b/tests/unit/test_xgboost.py @@ -158,13 +158,8 @@ def _create_train_job(version, instance_count=1, instance_type="ml.c4.4xlarge"): "environment": None, "experiment_config": None, "enable_network_isolation": False, - "debugger_hook_config": { - "CollectionConfigurations": [], - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), - }, "profiler_config": { - "DisableProfiler": False, - "S3OutputPath": "s3://{}/".format(BUCKET_NAME), + "DisableProfiler": True, }, }