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

Ensure ROOT is in batch mode #207

Merged
merged 6 commits into from
Sep 29, 2024
Merged

Conversation

Zehvogel
Copy link
Contributor

@Zehvogel Zehvogel commented Jul 3, 2024

BEGINRELEASENOTES

  • Ensure ROOT is in batch mode by setting gROOT.SetBatch(True) in k4run

ENDRELEASENOTES

Basically an upstream of key4hep/CLDConfig#36 as there will probably also be some other processors that are affected.

@@ -6,6 +6,7 @@ import argparse
import logging

from k4FWCore.utils import load_file
from ROOT import gROOT
Copy link
Contributor

@jmcarcell jmcarcell Jul 3, 2024

Choose a reason for hiding this comment

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

Ugh, this will make calling k4run slower, in particular the first time because of the ROOT imports:

{jcarcell@lxplus969.cern.ch} ~ % time python -c "from ROOT import gROOT"
python -c "from ROOT import gROOT"  0.59s user 0.36s system 8% cpu 11.352 total
{jcarcell@lxplus969.cern.ch} ~ % time python -c "from ROOT import gROOT"
python -c "from ROOT import gROOT"  0.54s user 0.30s system 65% cpu 1.300 total

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know 😎

Copy link
Contributor

@tmadlener tmadlener Jul 3, 2024

Choose a reason for hiding this comment

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

Can we move the import to after the dry_run check. Or alternatively, hide this behind a dedicated flag for k4run?

Alternatively, which algorithm is causing this, and can't that be fixed to not create a Canvas?

edit: Or set batch mode in the processor / algorithm directly? I don't think we should fix this at this level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we move the import to after the dry_run check

yes

Alternatively, which algorithm is causing this, and can't that be fixed to not create a Canvas?

It is https://github.com/iLCSoft/CLICPerformance/blob/master/Tracking/src/TrackChecker.cc
but the creation of the canvas is intended :(

We could also turn on batch mode there but without explicitly searching I would think it will not be the only existing processors, just the only one we use regularly...

In the end I don't care were and how we fix this, but I still think having ROOT in batch mode is the most future proof solution. Who knows when they make something else suddenly produce graphics output :)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. In that case I would hide it behind a flag for k4run (that disables batch mode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a flag that disables batch mode now but given the previous discussion isn't that the opposite of what you wanted? :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, sorry. I actually wanted the flag the other way around. Opt-in to non-batch mode (too many negatives flying around here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is opt-in to non-batch mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other way around I am not interested in as we will always have to turn it on and then I can just keep it in CLDConfig 🤷 so then we can also just close this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is opt-in to non-batch mode?

You are right. And I think this is what we want. With this flag, the default is to run in batch mode, but it gives users the possibility to turn it off and get outputs (if anything creates them).

One issue with this is that algorithms are in principle free to switch things again in their implementation, but I suppose there we simply have to tell people to not do that.

@tmadlener
Copy link
Contributor

Should we merge this? Or did someone come up with a better solution in the meantime?

@jmcarcell
Copy link
Contributor

I would like to have a second look, I remember seeing ROOT running some stuff for ipython when running k4run that I don't think it should run, also related to interactive versus batchmode.

@Zehvogel
Copy link
Contributor Author

Zehvogel commented Sep 4, 2024

I think as long as no one else runs into this issue it is of very low priority as we just apply this "fix" in CLDConfig at the moment :)

k4FWCore/scripts/k4run Outdated Show resolved Hide resolved
@@ -155,6 +155,12 @@ def main():
help="Print all the configurable components available in the framework and exit",
)
parser.add_argument("--gdb", action="store_true", help="Attach gdb debugger")
parser.add_argument(
"--disableROOTBatchMode",
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the options use dashes, maybe this could be changed to something like --enable-root-interactive? Or simply --root-interactive. I think the option in positive is easier to find in case someone wants to look at some plots.

k4FWCore/scripts/k4run Outdated Show resolved Hide resolved
k4FWCore/scripts/k4run Outdated Show resolved Hide resolved
k4FWCore/scripts/k4run Outdated Show resolved Hide resolved
@jmcarcell
Copy link
Contributor

I tested this and overall it doesn't change the runtime (as expected I guess). The only change is that loading ROOT happens earlier; now before running Gaudi while before this it would happen immediately after starting running in most cases because of podio or EDM4hep.

@jmcarcell jmcarcell enabled auto-merge (squash) September 29, 2024 20:05
@jmcarcell jmcarcell merged commit 20b341f into key4hep:main Sep 29, 2024
3 of 7 checks passed
@Zehvogel Zehvogel deleted the root-batchmode branch September 30, 2024 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants