Skip to content

fix: Add validation for empty ParameterString value in start local pipeline #4354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/sagemaker/local/entities.py
Original file line number Diff line number Diff line change
Expand Up @@ -845,6 +845,12 @@ def _initialize_and_validate_parameters(self, overridden_parameters):
"{}.".format(param_name, parameter_type.python_type, type(param_value))
)
raise ClientError(error_msg, "start_pipeline_execution")
if param_value == "":
error_msg = self._construct_validation_exception_message(
'Parameter {} value "" is too short (length: 0, '
"required minimum: 1).".format(param_name)
)
raise ClientError(error_msg, "start_pipeline_execution")
merged_parameters[param_name] = param_value
for param_name, default_parameter in default_parameters.items():
if param_name not in merged_parameters:
Expand Down
2 changes: 2 additions & 0 deletions tests/integ/test_local_mode.py
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ def else_step():
assert exe_step_result["Metadata"]["Condition"]["Outcome"] is True


@pytest.mark.local_mode
def test_local_pipeline_with_step_decorator_data_referenced_by_other_steps(
local_pipeline_session,
dummy_container,
Expand Down Expand Up @@ -987,6 +988,7 @@ def func(var: int):
assert exe_step_result["Metadata"]["Condition"]["Outcome"] is True


@pytest.mark.local_mode
def test_local_remote_function_with_additional_dependencies(
local_pipeline_session, dummy_container
):
Expand Down
26 changes: 26 additions & 0 deletions tests/unit/sagemaker/local/test_local_entities.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from botocore.exceptions import ClientError

import sagemaker.local
from sagemaker.workflow.fail_step import FailStep
from sagemaker.workflow.parameters import ParameterString
from sagemaker.workflow.pipeline import Pipeline
from sagemaker.workflow.lambda_step import LambdaStep
Expand Down Expand Up @@ -293,3 +294,28 @@ def test_start_local_pipeline_with_wrong_parameter_type(sagemaker_local_session)
f"Unexpected type for parameter '{parameter.name}'. Expected "
f"{parameter.parameter_type.python_type} but found {type(True)}." in str(error.value)
)


def test_start_local_pipeline_with_empty_parameter_string_value(
local_pipeline_session,
):
param_str_name = "MyParameterString"
param_str = ParameterString(name=param_str_name, default_value="default")
fail_step = FailStep(
name="MyFailStep",
error_message=param_str,
)

pipeline = Pipeline(
name="MyPipeline",
steps=[fail_step],
sagemaker_session=local_pipeline_session,
parameters=[param_str],
)

local_pipeline = sagemaker.local.entities._LocalPipeline(pipeline)
with pytest.raises(ClientError) as error:
local_pipeline.start(PipelineParameters={param_str_name: ""})
assert (
f'Parameter {param_str_name} value "" is too short (length: 0, required minimum: 1).'
) in str(error)