Skip to content

[tmva][sofie] Disentangle PyMVA and SOFIE #20355

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Nov 21, 2025
Merged

[tmva][sofie] Disentangle PyMVA and SOFIE #20355

merged 3 commits into from
Nov 21, 2025

Conversation

@guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Nov 9, 2025 •
edited
Loading

So far, the part of SOFIE that uses the C Python API was included in PyMVA for convenience, but the functionality is completely unrelated.

Moving this code to SOFIE itself means we can fully build and test SOFIE without enabling tmva-pymva.

The only subtelty is that we want to be able to disable these SOFIE features that require linking against libpython, because this is not always allowed (e.g. in the Python wheels). Therefore, this part is only built if we build also the other ROOT libraries that links against libpython besides PyMVA, wich is TPython.

@guitargeek guitargeek self-assigned this Nov 9, 2025
@guitargeek guitargeek changed the title [WIP][ci] Enable tmva-pymva to check with Python 3.14 and new torch [TMVA] Disentangle PyMVA and SOFIE Nov 9, 2025
@guitargeek guitargeek added the clean build Ask CI to do non-incremental build on PR label Nov 9, 2025
@guitargeek guitargeek changed the title [TMVA] Disentangle PyMVA and SOFIE [tmva][sofie] Disentangle PyMVA and SOFIE Nov 9, 2025
@github-actions
Copy link

github-actions bot commented Nov 9, 2025 •
edited
Loading

Test Results

    22 files      22 suites   3d 23h 6m 33s ⏱️
 3 779 tests  3 779 ✅ 0 💤 0 ❌
81 144 runs  81 144 ✅ 0 💤 0 ❌

Results for commit 4b863e2.

♻️ This comment has been updated with latest results.

@guitargeek guitargeek marked this pull request as ready for review November 10, 2025 10:24
@guitargeek guitargeek force-pushed the pymva_status branch 4 times, most recently from 4d736ec to 5b3842b Compare November 15, 2025 13:44
lmoneta
lmoneta reviewed Nov 17, 2025
View reviewed changes
Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Jonas for moving the Keras and PyTorch aprser away from PyMVA. A comment I have it is maybe to be more consistent, is to move them not in sofie but in the sofie_parsers.
Something to discuss

@guitargeek
Copy link
Contributor Author

guitargeek commented Nov 17, 2025

Thank you Jonas for moving the Keras and PyTorch aprser away from PyMVA. A comment I have it is maybe to be more consistent, is to move them not in sofie but in the sofie_parsers. Something to discuss

Sure, this can be done! I agree it's better to move it into sofie_parsers.

@guitargeek
Copy link
Contributor Author

guitargeek commented Nov 17, 2025

Hi @lmoneta, I have implemented your suggestion to moving the parser implementation to tmva/sofie_parses as you suggested. That means it's now conditional on the tmva-sofie=ON build flag, just like the ONNX parser.

I left the tests in tmva/sofie/test though, because also the tests for the ONNX parser is there for now (there is no tmva/sofie_parsers/test directory)

@guitargeek
[tmva][sofie] Disentangle PyMVA and SOFIE
2623263 
So far, the part of SOFIE that uses the C Python API was included in
PyMVA for convenience, but the functionality is completely unrelated.

Moving this code to SOFIE itself means we can fully build and test SOFIE
without enabling `tmva-pymva`.

The only subtelty is that we want to be able to disable these SOFIE
features that require linking against `libpython`, because this is not
always allowed (e.g. in the Python wheels). Therefore, this part is only
built if we build also the other ROOT libraries that links against
`libpython` besides PyMVA, wich is `TPython`.
@guitargeek
[tmva][sofie] Remove batch_size keyword arument from Dense layers
09d419d 
This keyword argument is not relevant for defining the model and will
cause errors in newer Keras versions.
@guitargeek
[tmva][sofie] Don't run Keras tests if the Keras version is too new
4b863e2
sanjibansg
sanjibansg approved these changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@sanjibansg sanjibansg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Thanks Jonas for this implementation!

@guitargeek guitargeek merged commit 10f28b6 into root-project:master Nov 21, 2025
28 of 30 checks passed
@guitargeek guitargeek deleted the pymva_status branch November 21, 2025 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lmoneta lmoneta lmoneta left review comments

@sanjibansg sanjibansg sanjibansg approved these changes

@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner

Assignees

@guitargeek guitargeek

Labels

clean build Ask CI to do non-incremental build on PR in:TMVA

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@guitargeek @lmoneta @sanjibansg