-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[tmva][sofie] Guard old Keras parser against Keras-3 #20791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
e5d987e to
7c914b4
Compare
guitargeek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the PR! This is a good temporary solution. Just a few comments:
- please remove the commit with the code formatting. It's not worth to format code that we plan to delete in #19692 anyway, so that's just bloating the history. You can ignore the clang-format CI errors this time.
- I think you don't need to explicitly
import kerasagain - Please drop the first commit, which comes from an unrelated development for which you have already opened a separate PR.
| PyRunString("import keras\n" | ||
| "version = keras.__version__\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| PyRunString("import keras\n" | |
| "version = keras.__version__\n" | |
| PyRunString("version = keras.__version__\n" |
keras is already imported before as tensorflow.keras, so you don't need to import it again I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the review!
I’ve removed the clang-format commit, dropped the unrelated initial commit, and removed the redundant import keras, keeping only tensorflow.keras.
The PR now contains only the intended Keras 3 runtime guard logic. Please let me know if any further changes are needed.
44c1176 to
60975ed
Compare
Test Results 18 files 18 suites 3d 3h 41m 48s ⏱️ For more details on these failures, see this check. Results for commit 60975ed. ♻️ This comment has been updated with latest results. |
This Pull request
Adds a defensive runtime guard to the TMVA SOFIE Keras parser to prevent segmentation faults when used with Keras 3 (enabled by default in TensorFlow ≥ 2.16).
The current parser is implemented against the Keras 2 API. When Keras 3 objects are encountered, undefined behavior may occur, leading to runtime crashes. This PR does not add Keras 3 support; it only detects the unsupported configuration early and fails cleanly with a clear error message.
This provides a temporary safety measure while proper Keras 3 support is being developed.
Changes or fixes:
Checklist:
This PR fixes #20591