fix: Attempting to serialize bytes type causes error (5660)#5682
fix: Attempting to serialize bytes type causes error (5660)#5682aviruthen wants to merge 2 commits intoaws:masterfrom
Conversation
mufaddal-rohawala
left a comment
mufaddal-rohawala
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
This PR fixes a legitimate bug where bytes objects cause a TypeError during serialization. The fix is correct and minimal, and the tests are well-structured. However, there is an indentation issue in the source code changes that must be fixed before merging.
|
|
||
| def is_not_primitive(obj): | ||
| return not isinstance(obj, (int, float, str, bool, datetime.datetime)) | ||
| return not isinstance(obj, (int, float, str, bool, bytes, datetime.datetime)) |
There was a problem hiding this comment.
Bug: Incorrect indentation introduced. The return statement now has 8 spaces of indentation (two levels) instead of the original 4 spaces (one level). This will cause an IndentationError at runtime since the function body was previously at 4-space indentation. Please fix:
return not isinstance(obj, (int, float, str, bool, bytes, datetime.datetime))|
|
||
| def is_primitive_class(cls): | ||
| return cls in (str, int, bool, float, datetime.datetime) | ||
| return cls in (str, int, bool, float, bytes, datetime.datetime) |
There was a problem hiding this comment.
Bug: Same incorrect indentation issue here. The return statement has 8 spaces instead of 4. Please fix:
return cls in (str, int, bool, float, bytes, datetime.datetime)| @@ -373,6 +373,30 @@ def test_serialize_method_nested_shape(): | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Nit: There's an extra blank line here (two blank lines between the previous test and this one, resulting in three blank lines total between top-level definitions). PEP 8 specifies two blank lines between top-level definitions. Minor, but CI linting may flag it.
aviruthen
deleted the
fix/attempting-to-serialize-bytes-type-causes-error-5660
branch
2 participants
Description
The
bytestype is not included in the primitive type checks insagemaker-core/src/sagemaker/core/utils/utils.py. Whenserialize()encounters abytesobject,is_not_primitive()returnsTrue(sincebytesisn't in the primitive tuple), causing the code to fall through to_serialize_shape(), which callsvars()on the bytes object and raises aTypeError. The fix is to addbytesto bothis_not_primitive()andis_primitive_class()functions.Related Issue
Fixes GitHub issue 5660
Changes Made
sagemaker-core/src/sagemaker/core/utils/utils.pysagemaker-core/tests/unit/generated/test_utils.pyAI-Generated PR
This PR was automatically generated by the PySDK Issue Agent.
Merge Checklist
prefix: descriptionformat