Skip to content
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

[ROOT-10909] Add TMVA python dependencies to the requirements.txt #14553

Closed
vepadulano opened this issue Feb 4, 2024 · 9 comments · Fixed by #14685 or #15512
Closed

[ROOT-10909] Add TMVA python dependencies to the requirements.txt #14553

vepadulano opened this issue Feb 4, 2024 · 9 comments · Fixed by #14685 or #15512
Labels
fixathon This issue can be tackled at a ROOT fixathon improvement

Comments

@vepadulano
Copy link
Member

Explain what you would like to see improved and how.

From https://its.cern.ch/jira/browse/ROOT-10909

The PR #5408 will add a requirements.txt file to the repo, which should reflect our dependencies to python packages. The current status does not cover TMVA with xgboost, sklearn and keras as runtime dependencies, which should be added.

ROOT version

Any

Installation method

Any

Operating system

Any

Additional context

No response

@vepadulano vepadulano added improvement fixathon This issue can be tackled at a ROOT fixathon labels Feb 4, 2024
@enirolf enirolf self-assigned this Feb 13, 2024
@vepadulano
Copy link
Member Author

Our current understanding is that we lack the following Python packages for the optional runtime dependencies of TMVA

tensorflow 
keras
torch
sklearn
xgboost
sonnet
graph_nets

@dpiparo
Copy link
Member

dpiparo commented Feb 15, 2024

@lmoneta could you confirm this is the list of dependencies? It's important to have those fixed, also considering containers and setup of CI nodes.

@lmoneta
Copy link
Member

lmoneta commented Feb 15, 2024

Yes, this is the list of dependencies, but they are optional, we should not emit an error if the package is not installed

@dpiparo
Copy link
Member

dpiparo commented Feb 15, 2024

but what happens if they are not there?

@lmoneta
Copy link
Member

lmoneta commented Feb 15, 2024

For TMVA nothing, if they are not there the corresponding tests are not run. Everything is protected. But I think if we add in the requirements.txt and the package is not there an error in the build is emitted.
We should have a way to install the packages in the new CI, but not in all platforms, only in some of them, for example in a platform tensorflow in another torch. It could be too much work to install them in every platforms, due to some possible incompatibilities

@dpiparo
Copy link
Member

dpiparo commented Mar 16, 2024

@lmoneta I was wondering if this PR could be merged after all

@guitargeek
Copy link
Contributor

guitargeek commented Apr 20, 2024

Giving an update on this comment: our current understanding is that we lack the following Python packages for the optional runtime dependencies of TMVA:

graph_nets
sonnet
ipywidgets (for JsMVA)

@enirolf enirolf removed their assignment Apr 25, 2024
@dpiparo
Copy link
Member

dpiparo commented May 14, 2024

What is stopping us from adding them and close this item for good?

@guitargeek
Copy link
Contributor

What is stopping us from adding them and close this item for good?

That the CI will probably become red, because it would enable the GNN tests that were never run on the CI before.

But we can work around this by also always disabling the tests:
#15512

Once that PR is merged, we can close this issue, and I'll open a new one to remind the TMVA guys to re-enable the GNN tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixathon This issue can be tackled at a ROOT fixathon improvement
Projects
Status: Done
5 participants