[Bug] deserialize_keras_object validates trainable field as boolean#22719
[Bug] deserialize_keras_object validates trainable field as boolean#22719Jah-yee wants to merge 2 commits intokeras-team:masterfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation logic for Keras object deserialization, specifically ensuring that the configuration is a dictionary and that the trainable attribute is a boolean. Review feedback identifies two critical issues: the validation logic incorrectly flags __typespec__ objects which use lists for configuration, and the trainable check can cause a TypeError when deserializing functions because it performs a substring search on a string instead of a dictionary lookup.
| if ( | ||
| inner_config is not None | ||
| and not isinstance(inner_config, dict) | ||
| and class_name != "function" | ||
| ): |
There was a problem hiding this comment.
The validation for inner_config being a dict incorrectly flags __typespec__ objects, which use a list for their configuration. This will cause deserialization of tf.TypeSpec objects to fail with a TypeError because their inner_config is a list, not a dict.
| if ( | |
| inner_config is not None | |
| and not isinstance(inner_config, dict) | |
| and class_name != "function" | |
| ): | |
| if ( | |
| inner_config is not None | |
| and not isinstance(inner_config, dict) | |
| and class_name not in ("function", "__typespec__") | |
| ): |
|
|
||
| # Validate trainable field if present in inner_config. | ||
| # trainable should always be a bool for Keras layers. | ||
| if inner_config and "trainable" in inner_config: |
There was a problem hiding this comment.
The check if inner_config and "trainable" in inner_config will crash with a TypeError when class_name is "function" if the function name contains the substring "trainable". For functions, inner_config is a string, and "trainable" in inner_config performs a substring check. If it returns True, the subsequent line inner_config["trainable"] attempts to index a string with a string key, which is invalid.
| if inner_config and "trainable" in inner_config: | |
| if isinstance(inner_config, dict) and "trainable" in inner_config: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22719 +/- ##
==========================================
+ Coverage 77.69% 83.00% +5.30%
==========================================
Files 596 596
Lines 69688 69695 +7
Branches 10869 10872 +3
==========================================
+ Hits 54147 57852 +3705
+ Misses 12789 8998 -3791
- Partials 2752 2845 +93
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
When deserialize_keras_object was called with a config dict where the 'config' field was not a dict (e.g., a list or string), the code would raise a confusing AttributeError: 'list' object has no attribute 'get'. This was because from_config() expects a dict. Now raises a clear TypeError: TypeError: Expected 'config' to be a dict, but got list: [1, 2, 3] The fix is applied only for class deserialization (not for 'class_name == function' where inner_config is a string name). Fixes keras-team#22701
Reject non-boolean trainable values (e.g. strings like 'yes', integers, None) when deserializing Keras objects. Previously, invalid trainable values were silently accepted, causing inconsistent behavior in code relying on strict boolean checks. Fixes keras-team#22699
Jah-yee
force-pushed
the
fix/22699-trainable-bool-validation
branch
from
ac7f68d to
6ff757e
Compare
|
HI @Jah-yee, Can you please sign the CLA? Thank you! |
|
This issue was addressed by #22740 |
5 participants
Reject non-boolean trainable values when deserializing Keras objects.
Fixes #22699