Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: [Bug] Pipeline parameters (ParameterInteger, ParameterString) fail in ModelTrain (5504)#5724
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
c517f24109214a0d1195de3b2d82File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -24,6 +24,7 @@ | ||
| from sagemaker.core.shapes import Unassigned | ||
| from sagemaker.core.modules import logger | ||
| from sagemaker.core.helper.pipeline_variable import PipelineVariable | ||
| def _is_valid_s3_uri(path: str, path_type: Literal["File", "Directory", "Any"] = "Any") -> bool: | ||
| @@ -129,19 +130,24 @@ def safe_serialize(data): | ||
| This function handles the following cases: | ||
| 1. If `data` is a string, it returns the string as-is without wrapping in quotes. | ||
| 2. If `data` is serializable (e.g., a dictionary, list, int, float), it returns | ||
| 2. If `data` is of type `PipelineVariable`, it returns the PipelineVariable object | ||
| as-is for pipeline serialization. | ||
| 3. If `data` is serializable (e.g., a dictionary, list, int, float), it returns | ||
| the JSON-encoded string using `json.dumps()`. | ||
| 3. If `data` cannot be serialized (e.g., a custom object), it returns the string | ||
| 4. If `data` cannot be serialized (e.g., a custom object), it returns the string | ||
| representation of the data using `str(data)`. | ||
| Args: | ||
| data (Any): The data to serialize. | ||
| Returns: | ||
| str: The serialized JSON-compatible string or the string representation of the input. | ||
| str | PipelineVariable: The serialized JSON-compatible string, the string | ||
| representation of the input, or the PipelineVariable object as-is. | ||
| """ | ||
| if isinstance(data, str): | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return data | ||
| elif isinstance(data, PipelineVariable): | ||
| return data | ||
| try: | ||
| return json.dumps(data) | ||
| except TypeError: | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # 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. | ||
| """Tests for safe_serialize in sagemaker.core.modules.utils with PipelineVariable support. | ||
| Verifies that safe_serialize correctly handles PipelineVariable objects | ||
| (e.g., ParameterInteger, ParameterString) by returning them as-is rather | ||
| than attempting str() conversion which would raise TypeError. | ||
| See: https://github.com/aws/sagemaker-python-sdk/issues/5504 | ||
| """ | ||
| from __future__ import annotations | ||
| import pytest | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| from sagemaker.core.modules.utils import safe_serialize | ||
| from sagemaker.core.helper.pipeline_variable import PipelineVariable | ||
| from sagemaker.core.workflow.parameters import ParameterInteger, ParameterString | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| class TestSafeSerializeWithPipelineVariables: | ||
| """Test safe_serialize handles PipelineVariable objects correctly.""" | ||
| @pytest.mark.parametrize("param", [ | ||
| ParameterInteger(name="MaxDepth", default_value=5), | ||
| ParameterString(name="Algorithm", default_value="xgboost"), | ||
| ]) | ||
| def test_safe_serialize_returns_pipeline_variable_as_is(self, param): | ||
| """PipelineVariable objects should be returned as-is (identity preserved).""" | ||
| result = safe_serialize(param) | ||
| assert result is param | ||
| assert isinstance(result, PipelineVariable) | ||
| def test_pipeline_variable_str_raises_type_error(self): | ||
| """Confirm PipelineVariable.__str__ raises TypeError (the root cause of the bug).""" | ||
| param = ParameterInteger(name="TestParam", default_value=10) | ||
| with pytest.raises(TypeError): | ||
| str(param) | ||
| class TestSafeSerializeBasicTypes: | ||
| """Regression tests: verify basic types still work after PipelineVariable support.""" | ||
| def test_safe_serialize_with_string(self): | ||
| """Strings should be returned as-is without JSON wrapping.""" | ||
| assert safe_serialize("hello") == "hello" | ||
| def test_safe_serialize_with_int(self): | ||
| """Integers should be JSON-serialized to string.""" | ||
| assert safe_serialize(42) == "42" | ||
| def test_safe_serialize_with_dict(self): | ||
| """Dicts should be JSON-serialized.""" | ||
| result = safe_serialize({"key": "val"}) | ||
| assert result == '{"key": "val"}' | ||
| def test_safe_serialize_with_bool(self): | ||
| """Booleans should be JSON-serialized.""" | ||
| assert safe_serialize(True) == "true" | ||
| assert safe_serialize(False) == "false" | ||
| def test_safe_serialize_with_none(self): | ||
| """None should be JSON-serialized to 'null'.""" | ||
| assert safe_serialize(None) == "null" | ||
| def test_safe_serialize_with_custom_object(self): | ||
| """Custom objects should fall back to str().""" | ||
| class CustomObj: | ||
| def __str__(self): | ||
| return "custom" | ||
| assert safe_serialize(CustomObj()) == "custom" | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -26,7 +26,7 @@ | ||
| from sagemaker.core.helper.session_helper import Session | ||
| from sagemaker.core.helper.pipeline_variable import PipelineVariable, StrPipeVar | ||
| from sagemaker.core.workflow.parameters import ParameterString | ||
| from sagemaker.core.workflow.parameters import ParameterString, ParameterInteger | ||
| from sagemaker.train.model_trainer import ModelTrainer, Mode | ||
| from sagemaker.train.configs import ( | ||
| Compute, | ||
| @@ -176,3 +176,68 @@ def test_training_image_rejects_invalid_type(self): | ||
| stopping_condition=DEFAULT_STOPPING, | ||
| output_data_config=DEFAULT_OUTPUT, | ||
| ) | ||
| class TestModelTrainerPipelineVariableHyperparameters: | ||
| """Test that PipelineVariable objects work correctly in ModelTrainer hyperparameters.""" | ||
| def test_hyperparameters_with_parameter_integer(self): | ||
| """ParameterInteger in hyperparameters should be preserved through _create_training_job_args. | ||
| This test documents the exact bug scenario from GH#5504: safe_serialize | ||
| would fall back to str(data) for PipelineVariable objects, but | ||
| PipelineVariable.__str__ intentionally raises TypeError. | ||
| Before the fix, this call would have raised TypeError. | ||
| """ | ||
| max_depth = ParameterInteger(name="MaxDepth", default_value=5) | ||
| trainer = ModelTrainer( | ||
| training_image=DEFAULT_IMAGE, | ||
| role=DEFAULT_ROLE, | ||
| compute=DEFAULT_COMPUTE, | ||
| stopping_condition=DEFAULT_STOPPING, | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| output_data_config=DEFAULT_OUTPUT, | ||
| hyperparameters={"max_depth": max_depth}, | ||
| ) | ||
| # This call would have raised TypeError before the fix (GH#5504) | ||
| args = trainer._create_training_job_args() | ||
| # PipelineVariable should be preserved as-is, not stringified | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| assert args["hyper_parameters"]["max_depth"] is max_depth | ||
| def test_hyperparameters_with_parameter_string(self): | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| """ParameterString in hyperparameters should be preserved.""" | ||
| algo = ParameterString(name="Algorithm", default_value="xgboost") | ||
| trainer = ModelTrainer( | ||
| training_image=DEFAULT_IMAGE, | ||
| role=DEFAULT_ROLE, | ||
| compute=DEFAULT_COMPUTE, | ||
| stopping_condition=DEFAULT_STOPPING, | ||
| output_data_config=DEFAULT_OUTPUT, | ||
| hyperparameters={"algorithm": algo}, | ||
| ) | ||
| args = trainer._create_training_job_args() | ||
| assert args["hyper_parameters"]["algorithm"] is algo | ||
| def test_hyperparameters_with_mixed_pipeline_and_static_values(self): | ||
| """Mixed PipelineVariable and static values should both be handled correctly.""" | ||
| max_depth = ParameterInteger(name="MaxDepth", default_value=5) | ||
| trainer = ModelTrainer( | ||
| training_image=DEFAULT_IMAGE, | ||
| role=DEFAULT_ROLE, | ||
| compute=DEFAULT_COMPUTE, | ||
| stopping_condition=DEFAULT_STOPPING, | ||
| output_data_config=DEFAULT_OUTPUT, | ||
| hyperparameters={ | ||
| "max_depth": max_depth, | ||
| "eta": 0.1, | ||
| "objective": "binary:logistic", | ||
| "num_round": 100, | ||
| }, | ||
| ) | ||
| args = trainer._create_training_job_args() | ||
| hp = args["hyper_parameters"] | ||
| # PipelineVariable preserved as-is | ||
| assert hp["max_depth"] is max_depth | ||
| # Static values serialized to strings | ||
| assert hp["eta"] == "0.1" | ||
| assert hp["objective"] == "binary:logistic" | ||
| assert hp["num_round"] == "100" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| # 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. | ||
| """Tests for safe_serialize with PipelineVariable support. | ||
| Verifies that safe_serialize in sagemaker.train.utils correctly handles | ||
| PipelineVariable objects (e.g., ParameterInteger, ParameterString) by | ||
| returning them as-is rather than attempting str() conversion which would | ||
| raise TypeError. | ||
| See: https://github.com/aws/sagemaker-python-sdk/issues/5504 | ||
| """ | ||
| from __future__ import annotations | ||
| import pytest | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| from sagemaker.train.utils import safe_serialize | ||
| from sagemaker.core.helper.pipeline_variable import PipelineVariable | ||
| from sagemaker.core.workflow.parameters import ParameterInteger, ParameterString | ||
| class TestSafeSerializeWithPipelineVariables: | ||
| """Test safe_serialize handles PipelineVariable objects correctly.""" | ||
| @pytest.mark.parametrize("param", [ | ||
aviruthen marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| ParameterInteger(name="MaxDepth", default_value=5), | ||
| ParameterString(name="Algorithm", default_value="xgboost"), | ||
| ]) | ||
| def test_safe_serialize_returns_pipeline_variable_as_is(self, param): | ||
| """PipelineVariable objects should be returned as-is (identity preserved).""" | ||
| result = safe_serialize(param) | ||
| assert result is param | ||
| assert isinstance(result, PipelineVariable) | ||
| def test_pipeline_variable_str_raises_type_error(self): | ||
| """Confirm PipelineVariable.__str__ raises TypeError (the root cause of the bug).""" | ||
| param = ParameterInteger(name="TestParam", default_value=10) | ||
| with pytest.raises(TypeError): | ||
| str(param) | ||
| class TestSafeSerializeBasicTypes: | ||
| """Regression tests: verify basic types still work after PipelineVariable support.""" | ||
| def test_safe_serialize_with_string(self): | ||
| """Strings should be returned as-is without JSON wrapping.""" | ||
| assert safe_serialize("hello") == "hello" | ||
| assert safe_serialize("12345") == "12345" | ||
| def test_safe_serialize_with_int(self): | ||
| """Integers should be JSON-serialized to string.""" | ||
| assert safe_serialize(42) == "42" | ||
| def test_safe_serialize_with_float(self): | ||
| """Floats should be JSON-serialized to string.""" | ||
| assert safe_serialize(3.14) == "3.14" | ||
| def test_safe_serialize_with_dict(self): | ||
| """Dicts should be JSON-serialized.""" | ||
| result = safe_serialize({"key": "val"}) | ||
| assert result == '{"key": "val"}' | ||
| def test_safe_serialize_with_bool(self): | ||
| """Booleans should be JSON-serialized.""" | ||
| assert safe_serialize(True) == "true" | ||
| assert safe_serialize(False) == "false" | ||
| def test_safe_serialize_with_none(self): | ||
| """None should be JSON-serialized to 'null'.""" | ||
| assert safe_serialize(None) == "null" | ||
| def test_safe_serialize_with_list(self): | ||
| """Lists should be JSON-serialized.""" | ||
| assert safe_serialize([1, 2, 3]) == "[1, 2, 3]" | ||
| def test_safe_serialize_with_custom_object(self): | ||
| """Custom objects should fall back to str().""" | ||
| class CustomObj: | ||
| def __str__(self): | ||
| return "custom" | ||
| assert safe_serialize(CustomObj()) == "custom" | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.