Fix prefer_cross_layers check#33231
Conversation
Signed-off-by: Liran Schour <lirans@il.ibm.com>
![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 fixes a bug in how a boolean configuration option was being parsed. The original implementation using bool(str(value)) would incorrectly evaluate to True even for the string "False". The fix corrects this by explicitly checking if the lowercased string value is equal to "true", which correctly handles the intended logic and ensures the feature is disabled by default. I've added one suggestion to make this boolean parsing even more robust by accounting for other common affirmative values like "1" or "yes".
| str(extra_config.get("enable_cross_layers_blocks", "False")).lower() | ||
| == "true" |
There was a problem hiding this comment.
While this correctly fixes the original bug where bool("False") evaluates to True, the boolean conversion could be more robust. The current implementation only considers "true" (case-insensitive) as affirmative. It would be better to handle other common boolean string representations like "1", "t", "y", "yes" to prevent unexpected behavior if a user configures it differently. This would make the configuration parsing more resilient, especially since kv_connector_extra_config values can be of Any type.
| str(extra_config.get("enable_cross_layers_blocks", "False")).lower() | |
| == "true" | |
| str(extra_config.get("enable_cross_layers_blocks", "False")).lower() | |
| in ("true", "1", "t", "y", "yes") |
There was a problem hiding this comment.
Can be done later, let's make sure the feature is off by default in the release first
| str(extra_config.get("enable_cross_layers_blocks", "False")).lower() | ||
| == "true" |
There was a problem hiding this comment.
Can be done later, let's make sure the feature is off by default in the release first
|
Will revert the PR |
|
Revert this PR for now |
2 participants
Fix of PR #30207
By default disable this feature
Purpose
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.