fix: Attempting to serialize bytes type causes error (5660)#5684
fix: Attempting to serialize bytes type causes error (5660)#5684aviruthen 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 correctly fixes a bug where bytes type was not recognized as a primitive, causing serialize() to fail with a TypeError. The logic change is sound and backward compatible. However, there is an indentation issue in the modified source file that needs to 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.
Indentation error: This line has 8 spaces of indentation (two levels) but should have 4 spaces (one level) to match the original function body. This will still work in Python since it's consistently indented within the function, but it breaks the project's style conventions and will likely fail linting.
| return not isinstance(obj, (int, float, str, bool, bytes, datetime.datetime)) | |
| 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.
Same indentation error here: 8 spaces instead of 4. Please fix to match the rest of the codebase.
| return cls in (str, int, bool, float, bytes, datetime.datetime) | |
| return cls in (str, int, bool, float, bytes, datetime.datetime) |
| @@ -373,6 +373,31 @@ 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 test functions inside the same module is fine per PEP 8 for top-level definitions, but the rest of this test file uses single blank lines between test functions). Minor, but worth keeping consistent with the existing file style.
| result = serialize({"body": b"1"}) | ||
| assert result == {"body": b"1"} | ||
|
|
||
|
|
There was a problem hiding this comment.
Good test, but the comment describes an implementation detail (walrus operator in _serialize_dict) that could become stale if the implementation changes. Consider rephrasing to describe the behavior rather than the implementation:
| # Empty bytes is falsy, so serialize omits it from the result | |
| result = serialize({"body": b""}) | |
| assert result == {} |
aviruthen
deleted the
fix/attempting-to-serialize-bytes-type-causes-error-5660
branch
sagemaker-bot
left a comment
sagemaker-bot
left a comment
There was a problem hiding this comment.
🤖 AI Code Review
This PR correctly fixes a bug where bytes type was not recognized as a primitive, causing serialize() to fail with a TypeError. The code changes are minimal, backward compatible, and well-tested. A few minor observations on the tests and an extra blank line, but overall this is a clean fix.
| result = serialize({"body": b"1"}) | ||
| assert result == {"body": b"1"} | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: The comment here is helpful for understanding the behavior, but it's worth noting that this test documents a potentially surprising behavior — users might expect b"" to be preserved in the output (similar to how 0 or False might be treated). If the _serialize_dict walrus operator skips all falsy values, that could also affect 0, False, "", etc. This isn't a problem with your PR, but worth flagging as a pre-existing behavior that may warrant a follow-up discussion.
|
|
||
| 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.
Very minor: there are now two consecutive blank lines after the function body (the added blank line plus the existing one). PEP 8 expects exactly two blank lines between top-level definitions, so this results in three blank lines total. CI formatting checks (black/flake8) should catch this, but worth fixing proactively.
Same issue applies at line 290 (is_primitive_class).
|
|
||
| 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.
Same extra blank line issue here — three blank lines between is_primitive_class and class Unassigned.
3 participants
Description
The
bytestype is not included in the primitive type checks insagemaker-core/src/sagemaker/core/utils/utils.py. There are three functions that define what counts as a 'primitive':is_not_primitive(),is_primitive_list()(which delegates tois_not_primitive), andis_primitive_class(). Whenserialize()encounters abytesvalue,is_not_primitive(b'1')returnsTrue, causing it to fall into the_serialize_shape()branch which callsvars()on the bytes object, raising aTypeError. The fix is to addbytesto the primitive type tuples inis_not_primitive()andis_primitive_class().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