From 9eb603537d0572de748cc07afe9927a1434cb560 Mon Sep 17 00:00:00 2001 From: Amarjeet LNU Date: Thu, 10 Sep 2026 13:50:00 -0700 Subject: [PATCH] fix(core): recognize new duplicate-name wording; actionable train() errors Telemetry shows the single largest V3 failure signature (40% of all failures, 465 accounts over 90 days) is CreatePipeline/CreateExperiment rejecting duplicate names. The service changed its error wording from '... already exists' to '... names must be unique within an AWS account ...', which silently broke every load-or-create flow that matched the old substring: Pipeline.upsert, Experiment/_Trial/ _TrialComponent._load_or_create, and _create_resource re-raised instead of loading or updating the existing resource. - Add a shared _is_resource_already_exists_error predicate in common_utils that matches all known wordings and error codes, and use it at all five call sites. - Pipeline.create: on a name collision, log guidance pointing at upsert() before re-raising. - ModelTrainer.train: log actionable remediation for terminal CreateTrainingJob failures before re-raising unchanged -- ResourceLimitExceeded (57% of ModelTrainer failures; quota name + Service Quotas link, flags that retries cannot succeed), AccessDenied (PassRole hint), NoRegionError and NoCredentialsError (setup steps). All error paths re-raise the original exception; only logging is added, so no caller contract changes. --- X-AI-Prompt: implement error-pattern action items from PySDK telemetry deep dive X-AI-Tool: Kiro --- .../src/sagemaker/core/common_utils.py | 45 +++++-- .../sagemaker/core/experiments/experiment.py | 6 +- .../src/sagemaker/core/experiments/trial.py | 6 +- .../core/experiments/trial_component.py | 6 +- .../unit/experiments/test_load_or_create.py | 83 +++++++++++++ .../tests/unit/test_common_utils.py | 92 ++++++++++++++ .../src/sagemaker/mlops/workflow/pipeline.py | 31 ++++- .../unit/workflow/test_pipeline_class.py | 113 ++++++++++++++++++ .../src/sagemaker/train/model_trainer.py | 66 +++++++++- .../tests/unit/train/test_model_trainer.py | 95 +++++++++++++++ 10 files changed, 514 insertions(+), 29 deletions(-) create mode 100644 sagemaker-core/tests/unit/experiments/test_load_or_create.py diff --git a/sagemaker-core/src/sagemaker/core/common_utils.py b/sagemaker-core/src/sagemaker/core/common_utils.py index 0c8025174c..4d36a5160a 100644 --- a/sagemaker-core/src/sagemaker/core/common_utils.py +++ b/sagemaker-core/src/sagemaker/core/common_utils.py @@ -2446,6 +2446,42 @@ def _check_job_status(job, desc, status_key_name): ) +# Error codes and message patterns the service returns when a create call collides +# with an existing resource. The service message wording has changed over time +# (e.g. CreatePipeline/CreateExperiment now return "... names must be unique within +# an AWS account ..." instead of "... already exists"), so match every known variant. +# The uniqueness pattern deliberately includes the "within an AWS account" scope so +# that other uniqueness validation errors (e.g. duplicate step names WITHIN a +# pipeline definition) are not mistaken for a resource-name collision. +_ALREADY_EXISTS_ERROR_CODES = ("ValidationException", "ResourceInUse") +_ALREADY_EXISTS_MSG_PATTERNS = ( + "Cannot create already existing", + "already exists", + "must be unique within an AWS account", +) + + +def _is_resource_already_exists_error(error) -> bool: + """Check whether a botocore ClientError means "this resource already exists". + + Use this predicate for every load-or-create / upsert flow instead of matching + a single hardcoded message substring, so that service message wording changes + do not silently break the already-exists branch. + + Args: + error (botocore.exceptions.ClientError): The error raised by a create call. + + Returns: + bool: True if the error indicates a name collision with an existing resource. + """ + error_response = getattr(error, "response", None) or {} + error_code = error_response.get("Error", {}).get("Code", "") + error_message = error_response.get("Error", {}).get("Message", "") + return error_code in _ALREADY_EXISTS_ERROR_CODES and any( + pattern in error_message for pattern in _ALREADY_EXISTS_MSG_PATTERNS + ) + + def _create_resource(create_fn): """Call create function and accepts/pass when resource already exists. @@ -2462,14 +2498,7 @@ def _create_resource(create_fn): # create function succeeded, resource does not exist already return True except ClientError as ce: - error_code = ce.response["Error"]["Code"] - error_message = ce.response["Error"]["Message"] - already_exists_exceptions = ["ValidationException", "ResourceInUse"] - already_exists_msg_patterns = ["Cannot create already existing", "already exists"] - if not ( - error_code in already_exists_exceptions - and any(p in error_message for p in already_exists_msg_patterns) - ): + if not _is_resource_already_exists_error(ce): raise ce # no new resource created as resource already exists return False diff --git a/sagemaker-core/src/sagemaker/core/experiments/experiment.py b/sagemaker-core/src/sagemaker/core/experiments/experiment.py index f555c44642..798494fa42 100644 --- a/sagemaker-core/src/sagemaker/core/experiments/experiment.py +++ b/sagemaker-core/src/sagemaker/core/experiments/experiment.py @@ -20,7 +20,7 @@ from sagemaker.core.apiutils import _base_types from sagemaker.core.experiments.trial import _Trial from sagemaker.core.experiments.trial_component import _TrialComponent -from sagemaker.core.common_utils import format_tags +from sagemaker.core.common_utils import format_tags, _is_resource_already_exists_error from sagemaker.core.telemetry.telemetry_logging import _telemetry_emitter from sagemaker.core.telemetry.constants import Feature @@ -169,9 +169,7 @@ def _load_or_create( sagemaker_session=sagemaker_session, ) except ClientError as ce: - error_code = ce.response["Error"]["Code"] - error_message = ce.response["Error"]["Message"] - if not (error_code == "ValidationException" and "already exists" in error_message): + if not _is_resource_already_exists_error(ce): raise ce # already exists experiment = Experiment.load(experiment_name, sagemaker_session) diff --git a/sagemaker-core/src/sagemaker/core/experiments/trial.py b/sagemaker-core/src/sagemaker/core/experiments/trial.py index 5b80557e1b..807437c531 100644 --- a/sagemaker-core/src/sagemaker/core/experiments/trial.py +++ b/sagemaker-core/src/sagemaker/core/experiments/trial.py @@ -18,7 +18,7 @@ from sagemaker.core.apiutils import _base_types import sagemaker.core.experiments._api_types as _api_types from sagemaker.core.experiments.trial_component import _TrialComponent -from sagemaker.core.common_utils import format_tags +from sagemaker.core.common_utils import format_tags, _is_resource_already_exists_error class _Trial(_base_types.Record): @@ -280,9 +280,7 @@ def _load_or_create( sagemaker_session=sagemaker_session, ) except ClientError as ce: - error_code = ce.response["Error"]["Code"] - error_message = ce.response["Error"]["Message"] - if not (error_code == "ValidationException" and "already exists" in error_message): + if not _is_resource_already_exists_error(ce): raise ce # already exists trial = _Trial.load(trial_name, sagemaker_session) diff --git a/sagemaker-core/src/sagemaker/core/experiments/trial_component.py b/sagemaker-core/src/sagemaker/core/experiments/trial_component.py index 29eb404d3b..00bf6aa0b2 100644 --- a/sagemaker-core/src/sagemaker/core/experiments/trial_component.py +++ b/sagemaker-core/src/sagemaker/core/experiments/trial_component.py @@ -20,7 +20,7 @@ from sagemaker.core.apiutils import _base_types import sagemaker.core.experiments._api_types as _api_types from sagemaker.core.experiments._api_types import TrialComponentSearchResult -from sagemaker.core.common_utils import format_tags +from sagemaker.core.common_utils import format_tags, _is_resource_already_exists_error class _TrialComponent(_base_types.Record): @@ -338,9 +338,7 @@ def _load_or_create( sagemaker_session=sagemaker_session, ) except ClientError as ce: - error_code = ce.response["Error"]["Code"] - error_message = ce.response["Error"]["Message"] - if not (error_code == "ValidationException" and "already exists" in error_message): + if not _is_resource_already_exists_error(ce): raise ce # already exists run_tc = _TrialComponent.load(trial_component_name, sagemaker_session) diff --git a/sagemaker-core/tests/unit/experiments/test_load_or_create.py b/sagemaker-core/tests/unit/experiments/test_load_or_create.py new file mode 100644 index 0000000000..7774e36758 --- /dev/null +++ b/sagemaker-core/tests/unit/experiments/test_load_or_create.py @@ -0,0 +1,83 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"). You +# may not use this file except in compliance with the License. A copy of +# the License is located at +# +# http://aws.amazon.com/apache2.0/ +# +# or in the "license" file accompanying this file. This file is +# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF +# ANY KIND, either express or implied. See the License for the specific +# language governing permissions and limitations under the License. +"""Regression tests for the load-or-create flows in the experiments modules. + +The service changed its duplicate-name error wording from '... already exists' +to '... names must be unique within an AWS account ...'. These tests pin the +already-exists branch against BOTH wordings so a future wording change cannot +silently turn load-or-create back into a hard failure. +""" +from __future__ import absolute_import + +import pytest +from unittest.mock import patch +from botocore.exceptions import ClientError + +from sagemaker.core.experiments.experiment import Experiment +from sagemaker.core.experiments.trial import _Trial +from sagemaker.core.experiments.trial_component import _TrialComponent + + +LEGACY_MESSAGE = "Experiment exp-1 already exists" +NEW_MESSAGE = "Experiment names must be unique within an AWS account and region" + + +def _validation_error(message): + return ClientError( + {"Error": {"Code": "ValidationException", "Message": message}}, "create" + ) + + +@pytest.mark.parametrize("message", [LEGACY_MESSAGE, NEW_MESSAGE]) +def test_experiment_load_or_create_loads_on_duplicate_name(message): + with patch.object(Experiment, "create", side_effect=_validation_error(message)): + with patch.object(Experiment, "load") as mock_load: + result = Experiment._load_or_create(experiment_name="exp-1") + + mock_load.assert_called_once_with("exp-1", None) + assert result is mock_load.return_value + + +def test_experiment_load_or_create_reraises_unrelated_validation_error(): + with patch.object( + Experiment, "create", side_effect=_validation_error("1 validation error detected") + ): + with patch.object(Experiment, "load") as mock_load: + with pytest.raises(ClientError): + Experiment._load_or_create(experiment_name="exp-1") + + mock_load.assert_not_called() + + +@pytest.mark.parametrize("message", [LEGACY_MESSAGE, NEW_MESSAGE]) +def test_trial_load_or_create_loads_on_duplicate_name(message): + with patch.object(_Trial, "create", side_effect=_validation_error(message)): + with patch.object(_Trial, "load") as mock_load: + mock_load.return_value.experiment_name = "exp-1" + result = _Trial._load_or_create(experiment_name="exp-1", trial_name="trial-1") + + mock_load.assert_called_once_with("trial-1", None) + assert result is mock_load.return_value + + +@pytest.mark.parametrize("message", [LEGACY_MESSAGE, NEW_MESSAGE]) +def test_trial_component_load_or_create_loads_on_duplicate_name(message): + with patch.object(_TrialComponent, "create", side_effect=_validation_error(message)): + with patch.object(_TrialComponent, "load") as mock_load: + result, is_existed = _TrialComponent._load_or_create( + trial_component_name="tc-1" + ) + + mock_load.assert_called_once_with("tc-1", None) + assert result is mock_load.return_value + assert is_existed is True diff --git a/sagemaker-core/tests/unit/test_common_utils.py b/sagemaker-core/tests/unit/test_common_utils.py index 64114e8d55..981d9bc78e 100644 --- a/sagemaker-core/tests/unit/test_common_utils.py +++ b/sagemaker-core/tests/unit/test_common_utils.py @@ -2911,3 +2911,95 @@ def test_save_to_default_bucket_preserves_kms(self, tmp_path): assert merged["ServerSideEncryption"] == "aws:kms" assert merged["SSEKMSKeyId"] == "kms-key-id" assert merged["ExpectedBucketOwner"] == "111111111111" + + +class TestIsResourceAlreadyExistsError: + """Test the shared already-exists predicate used by load-or-create/upsert flows.""" + + @staticmethod + def _client_error(code, message): + from botocore.exceptions import ClientError + + return ClientError({"Error": {"Code": code, "Message": message}}, "create_pipeline") + + def test_matches_legacy_already_exists_message(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error("ValidationException", "Pipeline pipe-1 already exists") + assert _is_resource_already_exists_error(error) is True + + def test_matches_new_names_must_be_unique_message(self): + """The service now returns 'names must be unique' instead of 'already exists'.""" + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error( + "ValidationException", + "Pipeline names must be unique within an AWS account and region", + ) + assert _is_resource_already_exists_error(error) is True + + def test_matches_experiment_names_must_be_unique_message(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error( + "ValidationException", + "Experiment names must be unique within an AWS account and region", + ) + assert _is_resource_already_exists_error(error) is True + + def test_matches_cannot_create_already_existing_message(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error( + "ValidationException", "Cannot create already existing endpoint configuration" + ) + assert _is_resource_already_exists_error(error) is True + + def test_matches_resource_in_use_code(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error( + "ResourceInUse", "Job name must be unique within an AWS account and region" + ) + assert _is_resource_already_exists_error(error) is True + + def test_rejects_other_validation_errors(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error("ValidationException", "1 validation error detected") + assert _is_resource_already_exists_error(error) is False + + def test_rejects_uniqueness_errors_scoped_within_a_resource(self): + """Uniqueness violations INSIDE a definition (e.g. duplicate step names) are + not name collisions -- treating them as already-exists would make upsert() + wrongly fall through to update() and mask the real validation error.""" + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error( + "ValidationException", "Step names must be unique within a pipeline" + ) + assert _is_resource_already_exists_error(error) is False + + def test_rejects_other_error_codes(self): + from sagemaker.core.common_utils import _is_resource_already_exists_error + + error = self._client_error("ResourceLimitExceeded", "names must be unique") + assert _is_resource_already_exists_error(error) is False + + def test_create_resource_accepts_names_must_be_unique(self): + """_create_resource treats the new service wording as already-exists.""" + from botocore.exceptions import ClientError + from sagemaker.core.common_utils import _create_resource + + def _raise(): + raise ClientError( + { + "Error": { + "Code": "ValidationException", + "Message": "Pipeline names must be unique within an AWS account", + } + }, + "create_pipeline", + ) + + assert _create_resource(_raise) is False diff --git a/sagemaker-mlops/src/sagemaker/mlops/workflow/pipeline.py b/sagemaker-mlops/src/sagemaker/mlops/workflow/pipeline.py index 472e1fbf0f..e7b171aed0 100644 --- a/sagemaker-mlops/src/sagemaker/mlops/workflow/pipeline.py +++ b/sagemaker-mlops/src/sagemaker/mlops/workflow/pipeline.py @@ -40,6 +40,7 @@ retry_with_backoff, format_tags, Tags, + _is_resource_already_exists_error, ) # Orchestration imports (now in mlops) @@ -180,6 +181,8 @@ def create( description: str = None, tags: Optional[Tags] = None, parallelism_config: ParallelismConfiguration = None, + *, + _log_name_collision_hint: bool = True, ) -> Dict[str, Any]: """Creates a Pipeline in the Pipelines service. @@ -219,7 +222,21 @@ def create( Tags=tags, ) # TODO: replace with sagemaker-core methods - return self.sagemaker_session.sagemaker_client.create_pipeline(**kwargs) + try: + return self.sagemaker_session.sagemaker_client.create_pipeline(**kwargs) + except ClientError as ce: + # upsert() handles the name collision itself (create-or-update), so it + # suppresses this hint -- otherwise every successful upsert of an + # existing pipeline would log a misleading ERROR. + if _log_name_collision_hint and _is_resource_already_exists_error(ce): + logger.error( + "A pipeline named '%s' already exists in this account and Region. " + "To update the existing pipeline (or create it only if missing), call " + "pipeline.upsert() instead of pipeline.create(). To keep both, choose a " + "unique pipeline name.", + self.name, + ) + raise ce def _create_args( self, role_arn: str, description: str, parallelism_config: ParallelismConfiguration @@ -353,11 +370,15 @@ def upsert( sagemaker_session=self.sagemaker_session, ) try: - response = self.create(role_arn, description, tags, parallelism_config) + response = self.create( + role_arn, + description, + tags, + parallelism_config, + _log_name_collision_hint=False, + ) except ClientError as ce: - error_code = ce.response["Error"]["Code"] - error_message = ce.response["Error"]["Message"] - if not (error_code == "ValidationException" and "already exists" in error_message): + if not _is_resource_already_exists_error(ce): raise ce # already exists response = self.update(role_arn, description, parallelism_config=parallelism_config) diff --git a/sagemaker-mlops/tests/unit/workflow/test_pipeline_class.py b/sagemaker-mlops/tests/unit/workflow/test_pipeline_class.py index c7d48502e5..22abb7d52a 100644 --- a/sagemaker-mlops/tests/unit/workflow/test_pipeline_class.py +++ b/sagemaker-mlops/tests/unit/workflow/test_pipeline_class.py @@ -429,6 +429,119 @@ def test_upsert_updates_existing_pipeline(self, mock_session): # Verify tags were merged and added mock_session.sagemaker_client.add_tags.assert_called_once() + def test_upsert_updates_existing_pipeline_with_names_must_be_unique_message( + self, mock_session + ): + """Upsert must also recognize the newer service wording 'names must be unique'.""" + error_response = { + "Error": { + "Code": "ValidationException", + "Message": "Pipeline names must be unique within an AWS account and region", + } + } + + mock_session.sagemaker_client.list_tags = Mock(return_value={"Tags": []}) + mock_session.sagemaker_client.add_tags = Mock() + + with patch.object(Pipeline, 'create') as mock_create: + with patch.object(Pipeline, 'update') as mock_update: + with patch('sagemaker.mlops.workflow.pipeline.resolve_value_from_config') as mock_resolve: + with patch('sagemaker.mlops.workflow.pipeline.resolve_and_validate_role') as mock_validate: + with patch('sagemaker.mlops.workflow.pipeline.format_tags') as mock_format: + mock_resolve.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_validate.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_format.return_value = [] + mock_create.side_effect = ClientError(error_response, "create_pipeline") + mock_update.return_value = { + "PipelineArn": "arn:aws:sagemaker:us-west-2:123:pipeline/test-pipeline" + } + + pipeline = Pipeline( + name="test-pipeline", + sagemaker_session=mock_session + ) + + result = pipeline.upsert(role_arn="arn:aws:iam::123:role/SageMakerRole") + + assert "PipelineArn" in result + mock_update.assert_called_once() + + def test_upsert_of_existing_pipeline_does_not_log_error_hint(self, mock_session, caplog): + """The create() name-collision hint must NOT fire on the successful upsert path.""" + import logging + + error_response = { + "Error": { + "Code": "ValidationException", + "Message": "Pipeline names must be unique within an AWS account and region", + } + } + mock_session.local_mode = False + mock_session.sagemaker_client.create_pipeline = Mock( + side_effect=ClientError(error_response, "create_pipeline") + ) + mock_session.sagemaker_client.list_tags = Mock(return_value={"Tags": []}) + + with patch.object(Pipeline, 'update') as mock_update: + with patch.object(Pipeline, '_create_args') as mock_args: + with patch('sagemaker.mlops.workflow.pipeline.resolve_value_from_config') as mock_resolve: + with patch('sagemaker.mlops.workflow.pipeline.resolve_and_validate_role') as mock_validate: + with patch('sagemaker.mlops.workflow.pipeline.format_tags') as mock_format: + mock_resolve.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_validate.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_format.return_value = [] + mock_args.return_value = {"PipelineName": "test-pipeline"} + mock_update.return_value = { + "PipelineArn": "arn:aws:sagemaker:us-west-2:123:pipeline/test-pipeline" + } + + pipeline = Pipeline( + name="test-pipeline", + sagemaker_session=mock_session + ) + + with caplog.at_level(logging.ERROR): + result = pipeline.upsert(role_arn="arn:aws:iam::123:role/SageMakerRole") + + assert "PipelineArn" in result + mock_update.assert_called_once() + assert "pipeline.upsert() instead of pipeline.create()" not in caplog.text + + def test_bare_create_of_existing_pipeline_logs_error_hint(self, mock_session, caplog): + """A direct create() name collision logs the remediation hint and re-raises.""" + import logging + + error_response = { + "Error": { + "Code": "ValidationException", + "Message": "Pipeline names must be unique within an AWS account and region", + } + } + mock_session.local_mode = False + mock_session.sagemaker_client.create_pipeline = Mock( + side_effect=ClientError(error_response, "create_pipeline") + ) + + with patch.object(Pipeline, '_create_args') as mock_args: + with patch('sagemaker.mlops.workflow.pipeline.resolve_value_from_config') as mock_resolve: + with patch('sagemaker.mlops.workflow.pipeline.resolve_and_validate_role') as mock_validate: + with patch('sagemaker.mlops.workflow.pipeline.format_tags') as mock_format: + mock_resolve.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_validate.return_value = "arn:aws:iam::123:role/SageMakerRole" + mock_format.return_value = [] + mock_args.return_value = {"PipelineName": "test-pipeline"} + + pipeline = Pipeline( + name="test-pipeline", + sagemaker_session=mock_session + ) + + with caplog.at_level(logging.ERROR): + with pytest.raises(ClientError): + pipeline.create(role_arn="arn:aws:iam::123:role/SageMakerRole") + + assert "pipeline.upsert() instead of pipeline.create()" in caplog.text + def test_upsert_without_role_raises_error(self, mock_session): """Test upsert without role raises ValueError.""" with patch('sagemaker.mlops.workflow.pipeline.resolve_value_from_config') as mock_resolve: diff --git a/sagemaker-train/src/sagemaker/train/model_trainer.py b/sagemaker-train/src/sagemaker/train/model_trainer.py index 241b6ac3be..d130130941 100644 --- a/sagemaker-train/src/sagemaker/train/model_trainer.py +++ b/sagemaker-train/src/sagemaker/train/model_trainer.py @@ -16,11 +16,13 @@ from enum import Enum import os import json +import re import shutil from tempfile import TemporaryDirectory from typing import Optional, List, Union, Dict, Any, ClassVar import yaml +from botocore.exceptions import ClientError, NoCredentialsError, NoRegionError from graphene.utils.str_converters import to_camel_case, to_snake_case from sagemaker.core.config.config_manager import SageMakerConfig from sagemaker.core import resources @@ -128,6 +130,42 @@ class Mode(Enum): SAGEMAKER_TRAINING_JOB = "SAGEMAKER_TRAINING_JOB" +def _log_actionable_client_error(client_error: "ClientError") -> None: + """Log remediation guidance for common CreateTrainingJob service errors. + + This never swallows the error: the caller always re-raises the original + exception. It only adds guidance so the failure is actionable without + guesswork. ResourceLimitExceeded in particular is not retryable until the + quota is raised, so blind retry loops keep failing. + + Args: + client_error (ClientError): The error raised by the CreateTrainingJob call. + """ + error = getattr(client_error, "response", None) or {} + error_code = error.get("Error", {}).get("Code", "") + error_message = error.get("Error", {}).get("Message", "") + if error_code == "ResourceLimitExceeded": + quota_match = re.search(r"service limit '([^']+)'", error_message) + quota_hint = f" ('{quota_match.group(1)}')" if quota_match else "" + logger.error( + "CreateTrainingJob was rejected because a SageMaker service quota%s " + "is insufficient in this account and Region. Retrying will keep " + "failing until the quota is raised. Request an increase in the " + "Service Quotas console: " + "https://console.aws.amazon.com/servicequotas/home/services/sagemaker/quotas " + "(or via 'aws service-quotas request-service-quota-increase " + "--service-code sagemaker'). Alternatively, use a different instance " + "type or reduce the instance count.", + quota_hint, + ) + elif error_code in ("AccessDeniedException", "AccessDenied"): + logger.error( + "CreateTrainingJob was denied by IAM. Verify that your caller identity " + "and the training role have the sagemaker:CreateTrainingJob and " + "iam:PassRole permissions for the role passed to the ModelTrainer." + ) + + class ModelTrainer(BaseModel): """Class that trains a model using AWS SageMaker. @@ -832,10 +870,30 @@ def train( self.sagemaker_session._intercept_create_request(training_request, None, "train") return - training_job = TrainingJob.create( - session=self.sagemaker_session.boto_session, - **training_request - ) + try: + training_job = TrainingJob.create( + session=self.sagemaker_session.boto_session, + **training_request + ) + except ClientError as ce: + _log_actionable_client_error(ce) + raise + except NoRegionError: + logger.error( + "No AWS Region configured. Set one before calling train(): export " + "AWS_DEFAULT_REGION=, add 'region = ' to ~/.aws/config, " + "or pass a boto session with a region to the ModelTrainer's " + "sagemaker_session." + ) + raise + except NoCredentialsError: + logger.error( + "No AWS credentials found. Configure credentials before calling train(): " + "run 'aws configure' or 'aws sso login', set the AWS_ACCESS_KEY_ID/" + "AWS_SECRET_ACCESS_KEY environment variables, or use an IAM role. See " + "https://boto3.amazonaws.com/v1/documentation/api/latest/guide/credentials.html" + ) + raise self._latest_training_job = training_job if wait: diff --git a/sagemaker-train/tests/unit/train/test_model_trainer.py b/sagemaker-train/tests/unit/train/test_model_trainer.py index ce5d208bbc..31c8abd36a 100644 --- a/sagemaker-train/tests/unit/train/test_model_trainer.py +++ b/sagemaker-train/tests/unit/train/test_model_trainer.py @@ -19,6 +19,7 @@ import os import yaml from omegaconf import OmegaConf +import logging import pytest from pydantic import ValidationError from unittest.mock import patch, MagicMock, ANY, mock_open @@ -2019,3 +2020,97 @@ def test_networking_intelligent_defaults_fills_subnets_on_existing(model_trainer assert model_trainer.networking.subnets == NETWORKING_DEFAULT_SUBNETS # pre-existing security_group_ids are preserved. assert model_trainer.networking.security_group_ids == ["sg-preexisting"] + + +# Actionable error guidance in train(). The original exception must always propagate +# unchanged; the SDK only adds remediation logging for common terminal failures +# (quota exhaustion, missing region/credentials) so users and automation stop +# blind-retrying errors that cannot succeed. + +RLE_ERROR_RESPONSE = { + "Error": { + "Code": "ResourceLimitExceeded", + "Message": ( + "The account-level service limit 'ml.g5.2xlarge for training job usage' is 0 " + "Instances, with current utilization of 0 Instances and a request delta of 1 " + "Instances. Please use AWS Service Quotas to request an increase for this quota." + ), + } +} + + +@patch("sagemaker.train.model_trainer.TrainingJob") +def test_train_resource_limit_exceeded_reraises_and_logs_guidance( + mock_training_job, model_trainer, caplog +): + from botocore.exceptions import ClientError + + mock_training_job.create.side_effect = ClientError(RLE_ERROR_RESPONSE, "CreateTrainingJob") + + with caplog.at_level(logging.ERROR): + with pytest.raises(ClientError) as raised: + model_trainer.train() + + assert raised.value.response["Error"]["Code"] == "ResourceLimitExceeded" + guidance = caplog.text + assert "Service Quotas" in guidance + assert "ml.g5.2xlarge for training job usage" in guidance + assert "Retrying will keep failing" in guidance + + +@patch("sagemaker.train.model_trainer.TrainingJob") +def test_train_no_region_error_reraises_and_logs_guidance( + mock_training_job, model_trainer, caplog +): + from botocore.exceptions import NoRegionError + + mock_training_job.create.side_effect = NoRegionError() + + with caplog.at_level(logging.ERROR): + with pytest.raises(NoRegionError): + model_trainer.train() + + assert "AWS_DEFAULT_REGION" in caplog.text + + +@patch("sagemaker.train.model_trainer.TrainingJob") +def test_train_no_credentials_error_reraises_and_logs_guidance( + mock_training_job, model_trainer, caplog +): + from botocore.exceptions import NoCredentialsError + + mock_training_job.create.side_effect = NoCredentialsError() + + with caplog.at_level(logging.ERROR): + with pytest.raises(NoCredentialsError): + model_trainer.train() + + assert "aws configure" in caplog.text + + +def test_log_actionable_client_error_access_denied(caplog): + from botocore.exceptions import ClientError + from sagemaker.train.model_trainer import _log_actionable_client_error + + error = ClientError( + {"Error": {"Code": "AccessDeniedException", "Message": "User is not authorized"}}, + "CreateTrainingJob", + ) + with caplog.at_level(logging.ERROR): + _log_actionable_client_error(error) + + assert "iam:PassRole" in caplog.text + + +def test_log_actionable_client_error_other_codes_stay_silent(caplog): + from botocore.exceptions import ClientError + from sagemaker.train.model_trainer import _log_actionable_client_error + + error = ClientError( + {"Error": {"Code": "ValidationException", "Message": "1 validation error detected"}}, + "CreateTrainingJob", + ) + with caplog.at_level(logging.ERROR): + _log_actionable_client_error(error) + + assert caplog.text == ""