Skip to content

Conversation

silverweed
Copy link
Contributor

@silverweed silverweed commented Aug 21, 2025

New verbosity works like this:

v0: only output errors
v1: only output errors and warnings
v2: output errors, warnings and TFileMerger messages
v3: output all messages

This is largely backwards-compatible as the default verbosity was and still is 99.

One message (and one error!) were for some reason only emitted when verbosity was exactly 1, which makes no sense. This PR changes it to a regular Info() and Err() respectively. This means that by default hadd will now emit an additional message at the end of the merging process.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #19479

Copy link

github-actions bot commented Aug 21, 2025

Test Results

    21 files      21 suites   3d 23h 23m 48s ⏱️
 3 646 tests  3 494 ✅  0 💤 152 ❌
74 819 runs  74 649 ✅ 18 💤 152 ❌

For more details on these failures, see this check.

Results for commit 55e7f1d.

♻️ This comment has been updated with latest results.

Copy link
Member

@dpiparo dpiparo left a comment

Choose a reason for hiding this comment

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

Thanks for putting some order in this!

Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

This is a step in the right direction. I believe that we need more flexibility for the Info message, I proposed a solution (that still need more update to Info calls).

@pcanal pcanal requested review from pcanal and dpiparo August 21, 2025 21:54
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

See previous comment.

@silverweed
Copy link
Contributor Author

I agree on adding the multiple levels to Info(), but what about all the messages you didn't correct? Are those all 2? Or 3?
If, as the description says, 2 is only for "minimal informative messages", I would expect most messages to be at level 3, but that would change what is shown by default if we set the default to 2...

@pcanal
Copy link
Member

pcanal commented Aug 22, 2025

I think that 2 is for "minimal informative messages", is the 'right' default.

I already made some proposition inline, here is another stab:

601:         Info(2) << "added " << nObjects << " object from filter file '" << *filterFileName << "'\n"; 
647:      Info(2) << "parallelizing  with " << nProcesses << " processes.\n";
678:      Info(2) << "target file: " << targetname << "\n";
681:      Info(3) << "Using " << cacheSize << "\n";
763:         Info(2) << "compression setting for meta data: " << newcomp << '\n';
765:         Info(2) << "compression setting for all output: " << newcomp << '\n';
784:      Info(2) << "each process should handle at least 3 files for efficiency."
897:         Info(3) << "merged " << allSubfiles.size() << " (" << fileMerger.GetMergeList()->GetEntries()

new verbosity works like this:

  v0: only output errors
  v1: only output errors and warnings
  v2: output errors, warnings and TFileMerger messages
  v3: output all messages

This is largely backwards-compatible as the default verbosity was and
still is 99.
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@silverweed silverweed merged commit 2e2dfc5 into root-project:master Aug 29, 2025
23 of 26 checks passed
@silverweed silverweed deleted the hadd_quiet branch August 29, 2025 07:44
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.

Info() logs should not go to stderr
3 participants