-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[tmva][sofie] New Keras Parser #19692
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?
[tmva][sofie] New Keras Parser #19692
Conversation
|
Thank you very much for the PR! It's indeed better to implement this in Python.
While waiting for the review by @lmoneta and @sanjibansg, I have a general question. What is the plan with the existing Keras parser in C++? If it is superseded by your new parser, can we remove it maybe in this PR as well? Like this we ensure that the users are not confused by two implementations, and also that the total maintenance cost doesn't increase. |
I agree with @guitargeek, we will remove the existing C++ parser for Keras within this PR. |
sanjibansg
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.
Did an initial review
| ROOT/_pythonization/_tmva/_utils.py | ||
| ROOT/_pythonization/_tmva/_gnn.py) | ||
| ROOT/_pythonization/_tmva/_gnn.py | ||
| ROOT/_pythonization/_tmva/_sofie/_parser/_keras/__init__.py |
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.
maybe better to have an additional CMake file for the keras parser files within their directory, so as to not add all of them here.
| @@ -0,0 +1,202 @@ | |||
| # functional_models.py | |||
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.
do we need this comment here?
| # # 1. Dropout (to test SOFIE's Identity operator) | ||
| # inp = layers.Input(shape=(10,)) | ||
| # out = layers.Dropout(0.5)(inp) | ||
| # model = models.Model(inputs=inp, outputs=out) | ||
| # train_and_save(model, "Functional_Dropout_test") |
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.
maybe we remove the commented out code if this is not used anyway?
| # 1. Dropout | ||
| # model = models.Sequential([ | ||
| # layers.Input(shape=(10,)), | ||
| # layers.Dropout(0.5) # Dropout | ||
| # ]) | ||
| # train_and_save(model, "Sequential_Dropout_test") | ||
|
|
||
| # 2. Binary Ops: Add, Subtract, Multiply are not typical in Sequential — skipping here | ||
|
|
||
| # 3. Concat (not applicable in Sequential without multi-input) |
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.
same comment as earlier for functional model
| return op | ||
| else: | ||
| raise RuntimeError( | ||
| "TMVA::SOFIE - Unsupported - Operator Gemm does not yet support input type " + fLayerDType |
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.
| "TMVA::SOFIE - Unsupported - Operator Gemm does not yet support input type " + fLayerDType | |
| "TMVA::SOFIE - Unsupported - Operator Conv does not yet support input type " + fLayerDType |
| op = gbl_namespace.TMVA.Experimental.SOFIE.ROperator_BasicBinary(float,'TMVA::Experimental::SOFIE::EBasicBinaryOperator::Mul')(fX1, fX2, fY) | ||
| else: | ||
| raise RuntimeError( | ||
| "TMVA::SOFIE - Unsupported - Operator Identity does not yet support input type " + fLayerDType |
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.
| "TMVA::SOFIE - Unsupported - Operator Identity does not yet support input type " + fLayerDType | |
| "TMVA::SOFIE - Unsupported - Operator BasicBinary does not yet support input type " + fLayerDType |
| @@ -0,0 +1,48 @@ | |||
| from cppyy import gbl as gbl_namespace | |||
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.
maybe good to check if the datatypes for the input tensors are float since we don't support the other cases?
| input = [str(i) for i in finput] | ||
| output = str(foutput[0]) | ||
| axis = int(attributes["axis"]) | ||
| op = gbl_namespace.TMVA.Experimental.SOFIE.ROperator_Concat(input, axis, 0, output) |
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.
maybe good to check the datatype for the input tensor?
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.
TensoFlow/Keras is too fragile to import unconditionally. You need to make sure it's not imported unconditionally, otherwise its presence will break several ROOT usecases, as we see in the tests. Always importing keras will also slow down importing ROOT, which is not desired
| @@ -0,0 +1,479 @@ | |||
| from ......_pythonization import pythonization | |||
| from cppyy import gbl as gbl_namespace | |||
| import keras | |||
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.
Here for example: instead of importing keras whenever the parsers are imported, you need to import it locally in the functions.
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.
hi @guitargeek, thanks for noticing this
but the reason I'm importing keras in this file is because I need the it to load the saved model using the input model file path
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.
You can still import Keras in these files, not problem! Just not globally. You have to do it in the functions that use it.
This is not allowed:
import keras
def func():
# do something with kerasThis is allowed:
def func():
import keras
# do something with kerasSame with the from keras import xyz imports.
So the refactoring should be pretty easy, because you have nicely organized everything in functions already 🙂 And these local imports don't have overhead in Python, because of the caching of imported modules. Think of it as "lazily importing" the keras module only when you actually need it.
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.
oh okay, got it. I'll make changes in the PR. Thanks
|
|
||
|
|
||
| from ._gnn import RModel_GNN, RModel_GraphIndependent | ||
| from ._sofie._parser._keras.parser import RModelParser_Keras |
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.
But here is the main culprit: this global import is the entry point for all other imports 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.
just a question, if we don't expose RModelParser_Keras.Parse class in the _tmva module, then how can we use it using pythonization?
I tried removing it but while using the parser it gave me this error

is there a way to tackle this?
@guitargeek @pcanal
Test Results 22 files 22 suites 3d 21h 11m 51s ⏱️ For more details on these failures, see this check. Results for commit 2e87835. ♻️ This comment has been updated with latest results. |
|
|
||
|
|
||
| from ._gnn import RModel_GNN, RModel_GraphIndependent | ||
| from ._sofie._parser._keras.parser import RModelParser_Keras |
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.
| from ._sofie._parser._keras.parser import RModelParser_Keras |
…ers and added tests for them. Imported Keras within the required functions. Created new CMakeLists.txt file for the keras parser. Made changes in the pythonization CMake file to build the keras parser files
|
Hi @lmoneta @sanjibansg, I have made a few changes in the PR
can you please take a look at it and let me know if you require any changes? Next steps:
|
|
Also, @guitargeek, @pcanal can you please take a look at the import statements of keras and whether they are suitable with ROOT now? |
I started the CI. If there are no seemingly unrelated test failures because of unconditionally importing keras anymore, then we know it's okay! |
|
|
||
| return keras.__version__ | ||
|
|
||
| keras_version = get_keras_version() No newline at end of file |
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.
Looks much better already!
But there is still an implicit import of Keras in this place. You do this import in _tmva/__init__.py:
from ._sofie._parser._keras.parser import RModelParser_KerasImporting a submodule implies importing all parent modules too. You you're also importing ._sofie._parser._keras. This import means running _keras/__init__.py, where you execute get_keras_version(), thus implicitly importing Keras.
You can fix this by removing the line with keras_version = get_keras_version(), and replace keras_version with get_keras_version() in the remaining code.
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.
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.
Yes
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.
hi @guitargeek i have made the changes
Can you please verify it once and start the tests?
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.
@guitargeek I found the error

here, the CI uses python 3.9 and a Keras version older than 2.13
And that's why it might be saying that tensorflow.python.framework.errors_impl.InvalidArgumentError: Graph execution error in case of AveragePool2D
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.
I added this sofie-keras-parser test to validate the parser's functionality
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.
Apologies I didn't catch this before
Can I know which keras version does the CI use?
I'll run the tests locally then
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.
The oldest Keras version in use by the CI is keras-2.13.1, on the alma8 platform. That's because this platform still uses Python 3.8.
But let's try to first try to fix the problems on the other platforms that use keras>=3.0, and then you think about if you need to support the older Keras versions later with @sanjibansg. Personally, I think it's totally valid to disable the functionality for Keras 2, as Keras 3 is out for almost 2 years.
If you need to know, you can check the version of Python packages that each platform is using by looking at the logs of the Docker container creation:
https://github.com/root-project/root-ci-images/actions
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
I'll look into it
…aced import keras_version with get_keras_version and called it in necessary files
|
We're making progress! But it seems we're still having unwanted implicit imports, as we see from the test failures: 2025-09-12T08:32:10.8961923Z 870/3621 Test #36: pyunittests-bindings-pyroot-pythonizations-pyroot-import-load-libs ................................***Failed 1.93 sec
2025-09-12T08:32:10.8962555Z test_import (import_load_libs.ImportLoadLibs.test_import)
2025-09-12T08:32:10.8962838Z Test libraries loaded after importing ROOT ... ERROR
2025-09-12T08:32:10.8963000Z
2025-09-12T08:32:10.8963078Z ======================================================================
2025-09-12T08:32:10.8963347Z ERROR: test_import (import_load_libs.ImportLoadLibs.test_import)
2025-09-12T08:32:10.8963701Z Test libraries loaded after importing ROOT
2025-09-12T08:32:10.8964392Z ----------------------------------------------------------------------
2025-09-12T08:32:10.8964636Z Traceback (most recent call last):
2025-09-12T08:32:10.8965031Z File "/github/home/ROOT-CI/src/bindings/pyroot/pythonizations/test/import_load_libs.py", line 135, in test_import
2025-09-12T08:32:10.8965419Z raise Exception(
2025-09-12T08:32:10.8965636Z Exception: Found not whitelisted libraries after importing ROOT:
2025-09-12T08:32:10.8965995Z - libscipy_openblas64_-56d6093b
2025-09-12T08:32:10.8966173Z - libgfortran-040039e1-0352e75f
2025-09-12T08:32:10.8966343Z - libquadmath-96973f99-934c22de
2025-09-12T08:32:10.8966615Z If the test fails with a library that is loaded on purpose, please add it to the whitelist.
2025-09-12T08:32:10.8966846Z
2025-09-12T08:32:10.8966948Z ----------------------------------------------------------------------
2025-09-12T08:32:10.8967160Z Ran 1 test in 1.777s
2025-09-12T08:32:10.8967253Z
2025-09-12T08:32:10.8967314Z FAILED (errors=1)
2025-09-12T08:32:10.8967580Z CMake Error at /github/home/ROOT-CI/src/cmake/modules/RootTestDriver.cmake:232 (message):
2025-09-12T08:32:10.8967859Z error code: 1
2025-09-12T08:32:10.8967945Z
2025-09-12T08:32:10.8967949Z
2025-09-12T08:32:10.8967952Z It looks like you're importing scipy somewhere in the global scope. |
|
Oh, |
| @@ -0,0 +1,544 @@ | |||
| from ......_pythonization import pythonization | |||
| from cppyy import gbl as gbl_namespace | |||
| import numpy as np | |||
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.
| import numpy as np |
Just like with Keras, numpy should also not be imported globally, because it loads the BLAS libraries. That's also not great, because it increases the time to import ROOT and also the risk of symbol collisions. Also, it makes it harder to test ROOT with Python development builds where NumPy is not yet available.
The only libraries that can be unconditionally imported are the builtin Python libraries, and ROOT libraries themselves like cppyy.
My bad, the problem was actually NumPy 🙂 See the comment above |
|
Hi @guitargeek |
|
No problem! And thank you very much for your work. I'm glad to see some C++ code that calls into Python getting replaced, because this is some of the most fragile code in ROOT that is difficult to maintain 👍 Your implementation in Python makes more sense. |
|
Hi! I really liked this PR, because it replaced some fragile C++ code that called into Python with real Python code 🙂 @PrasannaKasar, do you have a plan to move forward here? |
|
Yes @guitargeek , I am working on a few last issues left to cover, which were causing the tests to fail. Also, I am adding support to parse a couple of new Keras layers |
|
@guitargeek i know i promised to push the changes and fix the issues on this pr but i still can't figure out the exact reasons behind the failing tests
|
… directory. Used import numpy statements within the parser functions to avoid slowing down the import of ROOT.
|
@guitargeek can you please run the CI once? |
|
Hi! I have started the CI. Please also take my draft PR for some inspiration on what needs to be fixed to make things work: Especially, see the last 2 commits in my PR. Your PR is still missing the complete removal of the old C++ class and tests (your new Python implementation is not a "pythonization" of a C++ class, but just a new regular Python class). Also, as you can see in my PR, things only work for some Linux platforms. Can you please take a look, and also integrate my suggestions from my draft PR into your PR? Thanks. |
|
Hi @guitargeek I have few questions regarding the updates you made in the draft pr
|
|
Hi, thanks for the questions! Good that you are asking.
You run it in Python like
I don't understand your concern there, can yo clarify? Your parser is a Python class that calls into C++ methods via the ROOT Python interface. The parsers doesn't need to be a C++ class itself for that.
Pythonizing a C++ class in ROOT means you change the bindings for an existing C++ class, e.g. with this @pythonization("RModelParser_Keras", ns="TMVA::Experimental::SOFIE")
def pythonize_rmodelparser_keras(klass):
# Parameters:
# klass: class to be pythonized
setattr(klass, "Parse", RModelParser_Keras.Parse)The original C++ class is still there, but you replace or inject methods with more Pythonic version. In the case of the setattr(ns.Experimental.SOFIE, "RModelParser_Keras", _tmva.RModelParser_Keras)The reason why the Pythonization doesn't work in the CI is that by default, we exclude all TMVA C++ classes that call back into Python from the build ( @sanjibansg, FYI. |
Hi @guitargeek I got it. It was a misunderstanding from my end, I thought we need the parser to be used in C++ as well. Hence, I was insisting on declaring a dummy class in C++. But I'll use the Python class itself now |
Ahh, I understood the difference now. We need |
|
Yes, correct! |
|
But we should check with @sanjibansg if you want to continue supporting model parsing in C++ via the In other words, calling from C++ into Python should only happen via TPython, and not by including |
|
@guitargeek I have been getting this issue since the first CI I looked into the Keras documentation but I could find only few mentions of it in their issues section and found out that the issue is not limited to any Keras version, but it appears to be fixed in the later versions of Keras 2 |
|
I know nothing about Keras, sorry. @sanjibansg needs to help you here. My expertise is the ROOT Python interface in general. About the Keras versions on the macOS runners: I just checked which versions are installed:
|



This Pull request:
This pull request adds the support for parsing Keras models with SOFIE
Changes or fixes:
Currently, SOFIE's existing Keras parser is written in C++ and is quite old. Although it's written in C++ but its actual parsing logic is written in Python. Additionally, it lacks support for parsing layers such as MaxPool.
This new parser is natively written in Python and uses of pythnoization to access C++ methods from SOFIE. The parser support latest version of Keras (Keras 3) and also has backwards compatibility with earlier versions of Keras 2. It also supports for both types of models i.e. models built using Keras' Functional as well as Sequential API.
Layers supported by the parser:
Checklist: