Fix: validate trainable field type in Layer.__init__#22722
Fix: validate trainable field type in Layer.__init__#22722Jah-yee wants to merge 2 commits intokeras-team:masterfrom
Conversation
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 with a clear TypeError at layer construction time instead of silently accepting them. Fixes keras-team#22699
|
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. |
There was a problem hiding this comment.
Code Review
This pull request introduces input validation for the trainable parameter in layers and the config dictionary during serialization to ensure they are of the correct types. The review feedback correctly points out that ValueError should be used instead of TypeError for these user input validations, adhering to the Keras Repository Style Guide.
| if not isinstance(trainable, bool): | ||
| raise TypeError( | ||
| f"Expected `trainable` to be a bool, but got " | ||
| f"{type(trainable).__name__}: {trainable!r}" | ||
| ) |
There was a problem hiding this comment.
The Keras Repository Style Guide (line 178) specifies that ValueError should be used for user input validation instead of TypeError. Additionally, consider that isinstance(trainable, bool) is quite strict and will reject 0 or 1, which are often used as boolean substitutes in Python. However, if strict boolean enforcement is intended to prevent common configuration errors (like the string "False" being truthy), please at least update the exception type to ValueError.
| if not isinstance(trainable, bool): | |
| raise TypeError( | |
| f"Expected `trainable` to be a bool, but got " | |
| f"{type(trainable).__name__}: {trainable!r}" | |
| ) | |
| if not isinstance(trainable, bool): | |
| raise ValueError( | |
| f"Expected `trainable` to be a bool, but got " | |
| f"{type(trainable).__name__}: {trainable!r}" | |
| ) |
References
- In general, to validate user input, always use ValueError and avoid assert. (link)
| if ( | ||
| inner_config is not None | ||
| and not isinstance(inner_config, dict) | ||
| and class_name != "function" | ||
| ): | ||
| raise TypeError( | ||
| f"Expected `config` to be a dict, but got {type(inner_config).__name__}: " | ||
| f"{inner_config}" | ||
| ) |
There was a problem hiding this comment.
Please use ValueError instead of TypeError to adhere to the Keras Repository Style Guide (line 178).
| if ( | |
| inner_config is not None | |
| and not isinstance(inner_config, dict) | |
| and class_name != "function" | |
| ): | |
| raise TypeError( | |
| f"Expected `config` to be a dict, but got {type(inner_config).__name__}: " | |
| f"{inner_config}" | |
| ) | |
| if ( | |
| inner_config is not None | |
| and not isinstance(inner_config, dict) | |
| and class_name != "function" | |
| ): | |
| raise ValueError( | |
| f"Expected `config` to be a dict, but got {type(inner_config).__name__}: " | |
| f"{inner_config}" | |
| ) |
References
- In general, to validate user input, always use ValueError and avoid assert. (link)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22722 +/- ##
==========================================
- Coverage 82.95% 82.95% -0.01%
==========================================
Files 596 596
Lines 69252 69257 +5
Branches 10814 10816 +2
==========================================
+ Hits 57451 57452 +1
- Misses 8973 8975 +2
- Partials 2828 2830 +2
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:
|
|
Hi @Jah-yee, Can you please sign the CLA? Thank you! |
|
This issue was addressed by #22740 |
Good day
Issue
deserialize_keras_object accepts non-boolean values for the
trainablefield without validation (#22699).Reproduction
Fix
Added an explicit type check in
Layer.__init__that raises a clearTypeErrorwhentrainableis not abool:This ensures validation happens at layer construction time (during deserialization), catching the bad type early with a descriptive error.
Testing
Verified that valid
boolvalues (True/False) work correctly, while invalid values like"yes",1,0, etc. now raiseTypeErroras expected.Thank you for your attention. If there are any issues or suggestions, please leave a comment and I will address them promptly.
Warmly,
RoomWithOutRoof