fix: [Bug] Pipeline parameters (ParameterInteger, ParameterString) fail in ModelTrain (5504)

[aviruthen]

Description

The bug is that PipelineVariable objects (ParameterInteger, ParameterString) used as hyperparameter values cause a TypeError when safe_serialize tries to fall back to str(data), because PipelineVariable.str() intentionally raises TypeError. The safe_serialize function in sagemaker-train/src/sagemaker/train/utils.py already has a PipelineVariable isinstance check that returns the PipelineVariable object directly (allowing it to pass through for pipeline serialization). However, the safe_serialize function in sagemaker-core/src/sagemaker/core/modules/utils.py does NOT have this PipelineVariable handling, making it vulnerable to the same bug. The fix needs to: (1) add PipelineVariable handling to sagemaker-core/src/sagemaker/core/modules/utils.py to match the pattern already in sagemaker-train, and (2) add unit tests for safe_serialize with PipelineVariable inputs in both packages.

Related Issue

Related issue: 5504

Changes Made

• sagemaker-core/src/sagemaker/core/modules/utils.py
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py
• sagemaker-train/tests/unit/train/test_safe_serialize.py
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py

AI-Generated PR

This PR was automatically generated by the PySDK Issue Agent.

• Confidence score: 85%
• Classification: bug
• SDK version target: V3

Merge Checklist

• Changes are backward compatible
• Commit message follows prefix: description format
• Unit tests added/updated
• Integration tests added (if applicable)
• Documentation updated (if applicable)

[sagemaker-bot]

🤖 AI Code Review

This PR fixes a bug where PipelineVariable objects (ParameterInteger, ParameterString) cause TypeError in safe_serialize when used as hyperparameter values. The fix adds PipelineVariable handling to sagemaker-core's safe_serialize to match the existing pattern in sagemaker-train. The code change is minimal and correct, and tests are comprehensive. A few minor issues worth addressing.

[aviruthen]

🤖 Iteration #1 — Review Comments Addressed

Description

Fix safe_serialize in sagemaker-core to handle PipelineVariable objects (e.g., ParameterInteger, ParameterString) when used as hyperparameter values in ModelTrainer.

Problem

When Pipeline parameters are used as hyperparameter values in ModelTrainer, the safe_serialize function in sagemaker-core/src/sagemaker/core/modules/utils.py fails because:

1. json.dumps() can't serialize PipelineVariable objects, so it falls through to the except TypeError handler
2. The fallback calls str(data), but PipelineVariable.__str__() intentionally raises TypeError
3. This causes pipeline creation (pipeline.upsert()) to fail

The safe_serialize function in sagemaker-train/src/sagemaker/train/utils.py already had the correct PipelineVariable handling, but the equivalent function in sagemaker-core did not.

Fix

• Added PipelineVariable isinstance check to safe_serialize in sagemaker-core/src/sagemaker/core/modules/utils.py, matching the pattern already established in sagemaker-train
• Guarded the PipelineVariable import with try/except ImportError to handle edge cases where the module might not be available
• Added comprehensive unit tests for both sagemaker-core and sagemaker-train safe_serialize functions
• Added negative tests documenting the root cause (PipelineVariable.__str__ raising TypeError)
• Added integration-level test verifying ParameterInteger in ModelTrainer hyperparameters doesn't raise TypeError

Changes

• sagemaker-core/src/sagemaker/core/modules/utils.py: Guard PipelineVariable import and add isinstance check in safe_serialize
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: Add tests with from __future__ import annotations, negative test for root cause
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Add tests with from __future__ import annotations, negative test for root cause
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Add explicit bug scenario test (test_hyperparameters_with_parameter_integer_does_not_raise)

Comments reviewed: 7
Files modified: sagemaker-core/src/sagemaker/core/modules/utils.py, sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py, sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py, sagemaker-train/tests/unit/train/test_safe_serialize.py

• sagemaker-core/src/sagemaker/core/modules/utils.py: Guard the PipelineVariable import with try/except to handle cases where the module might not be available, and to prevent potential circular import issues.
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: Use from __future__ import annotations, keep pytest (now used), and add negative test documenting the root cause bug.
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Add a test that explicitly documents the bug scenario - verifying no TypeError is raised during safe_serialize call path.
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Use from __future__ import annotations, add negative test for pytest usage, remove unused pytest import issue.

[sagemaker-bot]

🤖 AI Code Review

This PR fixes a bug where PipelineVariable objects (ParameterInteger, ParameterString) cause TypeError in safe_serialize within sagemaker-core. The fix mirrors the existing pattern in sagemaker-train. The code change is minimal and correct, with good test coverage. A few minor issues worth addressing.

[aviruthen]

🤖 Iteration #2 — Review Comments Addressed

Description

Fix safe_serialize in sagemaker-core to handle PipelineVariable objects (e.g., ParameterInteger, ParameterString) when used as hyperparameter values in ModelTrainer.

Problem

When Pipeline parameters are used as hyperparameter values in ModelTrainer, the safe_serialize function in sagemaker-core/src/sagemaker/core/modules/utils.py fails because:

1. json.dumps() can't serialize PipelineVariable objects, so it falls through to the except TypeError handler
2. The fallback calls str(data), but PipelineVariable.__str__() intentionally raises TypeError
3. This causes pipeline creation (pipeline.upsert()) to fail

The safe_serialize function in sagemaker-train/src/sagemaker/train/utils.py already had the correct PipelineVariable handling, but the equivalent function in sagemaker-core did not.

Fix

• Added PipelineVariable isinstance check to safe_serialize in sagemaker-core/src/sagemaker/core/modules/utils.py, matching the pattern already established in sagemaker-train
• Guarded the PipelineVariable import with try/except ImportError to handle edge cases where the module might not be available
• Added comprehensive unit tests for both sagemaker-core and sagemaker-train safe_serialize functions
• Added negative tests documenting the root cause (PipelineVariable.__str__ raising TypeError)
• Added integration-level test verifying ParameterInteger in ModelTrainer hyperparameters doesn't raise TypeError

Changes

• sagemaker-core/src/sagemaker/core/modules/utils.py: Guard PipelineVariable import with try/except ImportError and add isinstance check in safe_serialize
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: Tests with from __future__ import annotations, negative test for root cause
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Tests with from __future__ import annotations, consolidated with @pytest.mark.parametrize
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Consolidated redundant tests, removed try/except + pytest.fail() anti-pattern

Comments reviewed: 12
Files modified: sagemaker-core/src/sagemaker/core/modules/utils.py, sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py, sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py, sagemaker-train/tests/unit/train/test_safe_serialize.py

• sagemaker-core/src/sagemaker/core/modules/utils.py: The file already has the correct PipelineVariable import guard and safe_serialize implementation. No changes needed - the code already matches the desired state.
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: File already uses 'from future import annotations', has pytest imported and used in the negative test. No changes needed.
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Consolidate redundant tests: remove test_hyperparameters_with_parameter_integer_does_not_raise since test_hyperparameters_with_parameter_integer already covers the same scenario. Simplify the remainin
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Consolidate redundant PipelineVariable tests using @pytest.mark.parametrize, keep pytest import for the negative test.

[sagemaker-bot]

🤖 AI Code Review

This PR fixes a bug where PipelineVariable objects (ParameterInteger, ParameterString) cause TypeError in safe_serialize when used as hyperparameter values. The fix adds a PipelineVariable isinstance check to sagemaker-core's safe_serialize to match the existing pattern in sagemaker-train. The approach is sound and well-tested, with a few minor issues to address.

[aviruthen]

🤖 Iteration #3 — Review Comments Addressed

Description

Fix safe_serialize in sagemaker-core to handle PipelineVariable objects (e.g., ParameterInteger, ParameterString) when used as hyperparameter values in ModelTrainer.

Problem

When Pipeline parameters are used as hyperparameter values in ModelTrainer, the safe_serialize function in sagemaker-core/src/sagemaker/core/modules/utils.py fails because:

1. json.dumps() can't serialize PipelineVariable objects, so it falls through to the except TypeError handler
2. The fallback calls str(data), but PipelineVariable.__str__() intentionally raises TypeError
3. This causes pipeline creation (pipeline.upsert()) to fail

The safe_serialize function in sagemaker-train/src/sagemaker/train/utils.py already had the correct PipelineVariable handling, but the equivalent function in sagemaker-core did not.

Fix

• Added PipelineVariable isinstance check to safe_serialize in sagemaker-core/src/sagemaker/core/modules/utils.py, matching the pattern already established in sagemaker-train
• Used a direct import (no try/except ImportError guard) since PipelineVariable is within the same sagemaker-core package and always available
• Updated the docstring return type to reflect that the function can return str | PipelineVariable
• Added comprehensive unit tests for both sagemaker-core and sagemaker-train safe_serialize functions using pytest.mark.parametrize
• Added negative tests documenting the root cause (PipelineVariable.__str__ raising TypeError)
• Added integration-level test verifying ParameterInteger in ModelTrainer hyperparameters doesn't raise TypeError

Changes

• sagemaker-core/src/sagemaker/core/modules/utils.py: Direct PipelineVariable import and isinstance check in safe_serialize
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: Tests with @pytest.mark.parametrize, negative test for root cause
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Tests with @pytest.mark.parametrize, negative test for root cause
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Bug scenario test verifying no TypeError during safe_serialize call path

Comments reviewed: 20
Files modified: sagemaker-core/src/sagemaker/core/modules/utils.py, sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py, sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py, sagemaker-train/tests/unit/train/test_safe_serialize.py

• sagemaker-core/src/sagemaker/core/modules/utils.py: Remove try/except guard around PipelineVariable import (use direct import since it's within the same sagemaker-core package) and update safe_serialize to handle PipelineVariable with proper return typ
• sagemaker-core/tests/unit/core/modules/test_utils_safe_serialize.py: Use from future import annotations, use pytest.mark.parametrize for PipelineVariable tests, keep pytest for negative test.
• sagemaker-train/tests/unit/train/test_model_trainer_pipeline_variable.py: Consolidate redundant hyperparameter tests, fix long docstring line, ensure idiomatic pytest patterns.
• sagemaker-train/tests/unit/train/test_safe_serialize.py: Files already look good from iteration 2. No changes needed - already uses from future import annotations, pytest.mark.parametrize, and has the negative test.